From 269a1a9b93571887bd3fbe873a03546af6d4857e Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Tue, 17 Apr 2018 04:22:19 -0700 Subject: [PATCH] [starvation] treat locks in (and accessed from) inner classes properly Reviewed By: sblackshear Differential Revision: D7427659 fbshipit-source-id: 9abf1ad --- infer/src/IR/AccessPath.ml | 46 ++++++++++++++++ infer/src/IR/AccessPath.mli | 11 ++++ infer/src/IR/Pvar.ml | 8 +++ infer/src/IR/Pvar.mli | 3 + infer/src/concurrency/starvation.ml | 2 +- .../java/starvation/InnerClass.java | 55 +++++++++++++++++++ .../codetoanalyze/java/starvation/issues.exp | 2 + 7 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 infer/tests/codetoanalyze/java/starvation/InnerClass.java diff --git a/infer/src/IR/AccessPath.ml b/infer/src/IR/AccessPath.ml index cf6da80b4..e249b9212 100644 --- a/infer/src/IR/AccessPath.ml +++ b/infer/src/IR/AccessPath.ml @@ -261,3 +261,49 @@ module BaseMap = PrettyPrintable.MakePPMap (struct let pp = pp_base end) + +(* transform an access path that starts on "this" of an inner class but which breaks out to + access outer class fields to the outermost one *) +let inner_class_normalize p = + let open Typ in + let is_synthetic_this pvar = Pvar.get_simplified_name pvar |> String.is_prefix ~prefix:"this$" in + let mk_pvar_as name pvar = Pvar.get_declaring_function pvar |> Option.map ~f:(Pvar.mk name) in + let aux = function + (* (this:InnerClass* ).(this$n:OuterClassAccessor).f. ... -> (this:OuterClass* ).f . ... *) + | Some + ( ( (Var.ProgramVar pvar as root) + , ({desc= Tptr (({desc= Tstruct name} as cls), pkind)} as ptr) ) + , (FieldAccess first) :: accesses ) + when Pvar.is_this pvar && Fieldname.Java.is_outer_instance first -> + Name.Java.get_outer_class name + |> Option.map ~f:(fun outer_name -> + let outer_class = mk ~default:cls (Tstruct outer_name) in + let outer_ptr = mk ~default:ptr (Tptr (outer_class, pkind)) in + ((root, outer_ptr), accesses) ) + (* this$n.(this$m:OuterClassAccessor).f ... -> (this$m:OuterClass* ).f . ... *) + (* happens in ctrs only *) + | Some + ( (Var.ProgramVar pvar, ({desc= Tptr (({desc= Tstruct name} as cls), pkind)} as ptr)) + , (FieldAccess first) :: accesses ) + when is_synthetic_this pvar && Fieldname.Java.is_outer_instance first -> + Name.Java.get_outer_class name + |> Option.bind ~f:(fun outer_name -> + let outer_class = mk ~default:cls (Tstruct outer_name) in + let outer_ptr = mk ~default:ptr (Tptr (outer_class, pkind)) in + let varname = Fieldname.to_flat_string first |> Mangled.from_string in + mk_pvar_as varname pvar + |> Option.map ~f:(fun new_pvar -> + let base = base_of_pvar new_pvar outer_ptr in + (base, accesses) ) ) + (* this$n.f ... -> this.f . ... *) + (* happens in ctrs only *) + | Some ((Var.ProgramVar pvar, typ), all_accesses) + when is_synthetic_this pvar -> + let varname = Mangled.from_string "this" in + mk_pvar_as varname pvar + |> Option.map ~f:(fun new_pvar -> (base_of_pvar new_pvar typ, all_accesses)) + | _ -> + None + in + let rec loop path_opt = match aux path_opt with None -> path_opt | res -> loop res in + loop (Some p) |> Option.value ~default:p diff --git a/infer/src/IR/AccessPath.mli b/infer/src/IR/AccessPath.mli index 110167544..22fb8c62a 100644 --- a/infer/src/IR/AccessPath.mli +++ b/infer/src/IR/AccessPath.mli @@ -61,6 +61,17 @@ val append : t -> access list -> t val is_prefix : t -> t -> bool (** return true if [ap1] is a prefix of [ap2]. returns true for equal access paths *) +val inner_class_normalize : t -> t +(** transform an access path that starts on "this" of an inner class but which breaks out to + access outer class fields to the outermost one. + Cases handled (recursively): +- (this:InnerClass* ).(this$n:OuterClassAccessor).f. ... -> (this:OuterClass* ).f . ... +- this$n.(this$m:OuterClassAccessor).f ... -> (this$m:OuterClass* ).f . ... + (happens in ctrs only) +- this$n.f ... -> this.f . ... + (happens in ctrs only) +*) + val equal : t -> t -> bool val equal_base : base -> base -> bool diff --git a/infer/src/IR/Pvar.ml b/infer/src/IR/Pvar.ml index bdad4d1d7..a90c5c8cd 100644 --- a/infer/src/IR/Pvar.ml +++ b/infer/src/IR/Pvar.ml @@ -55,6 +55,14 @@ let compare_modulo_this x y = let equal = [%compare.equal : t] +let get_declaring_function pv = + match pv.pv_kind with + | Local_var n | Callee_var n | Abduced_retvar (n, _) | Abduced_ref_param (n, _, _) -> + Some n + | Global_var _ | Seed_var -> + None + + let pp_translation_unit fmt = function None -> () | Some fname -> SourceFile.pp fmt fname let pp_ f pv = diff --git a/infer/src/IR/Pvar.mli b/infer/src/IR/Pvar.mli index fdbea906a..50f99eeef 100644 --- a/infer/src/IR/Pvar.mli +++ b/infer/src/IR/Pvar.mli @@ -29,6 +29,9 @@ val compare_modulo_this : t -> t -> int val equal : t -> t -> bool (** Equality for pvar's *) +val get_declaring_function : t -> Typ.Procname.t option +(** if not a global, return function declaring var *) + val d : t -> unit (** Dump a program variable. *) diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml index 645ffc3ed..b1d30744c 100644 --- a/infer/src/concurrency/starvation.ml +++ b/infer/src/concurrency/starvation.ml @@ -55,7 +55,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct match AccessExpression.to_access_path access_exp with | (((Var.ProgramVar pvar, _) as base), _) as path when is_formal base || Pvar.is_global pvar -> - Some path + Some (AccessPath.inner_class_normalize path) | _ -> (* ignore paths on local or logical variables *) None ) diff --git a/infer/tests/codetoanalyze/java/starvation/InnerClass.java b/infer/tests/codetoanalyze/java/starvation/InnerClass.java new file mode 100644 index 000000000..d695f08f4 --- /dev/null +++ b/infer/tests/codetoanalyze/java/starvation/InnerClass.java @@ -0,0 +1,55 @@ +/* + * Copyright (c) 2018 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +import java.lang.Object; + +class InnerClass { + synchronized void outerInnerOk(InnerClassA a) { + a.foo(); + } + + synchronized void bar() {} + + synchronized void outerInnerBad(InnerClassA a) { + a.baz(); + } + + class InnerClassA { + void foo() { + synchronized(InnerClass.this) {} + } + + void outerInnerOk() { + synchronized(InnerClass.this) { + InnerClass.this.bar(); + } + } + + synchronized void baz() {} + + synchronized void innerOuterBad() { + InnerClass.this.bar(); + } + + // ctrs generate different access paths so test these too + // following should not be flagged + InnerClassA() { + synchronized(InnerClass.this) { + InnerClass.this.bar(); + } + } + + // following should be flagged with outer_inner_bad() + InnerClassA(Object o) { + synchronized(this) { + InnerClass.this.bar(); + } + } + } +} diff --git a/infer/tests/codetoanalyze/java/starvation/issues.exp b/infer/tests/codetoanalyze/java/starvation/issues.exp index 23c5f4cc6..f6abe3c6b 100644 --- a/infer/tests/codetoanalyze/java/starvation/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation/issues.exp @@ -1,3 +1,5 @@ +codetoanalyze/java/starvation/InnerClass.java, void InnerClass$InnerClassA.innerOuterBad(), 0, STARVATION, ERROR, [[Trace 1] Lock acquisition: locks this in class InnerClass$InnerClassA*,Method call: void InnerClass.bar(),Lock acquisition: locks this in class InnerClass*,[Trace 2] Lock acquisition: locks this in class InnerClass*,Method call: void InnerClass$InnerClassA.baz(),Lock acquisition: locks this in class InnerClass$InnerClassA*] +codetoanalyze/java/starvation/InnerClass.java, void InnerClass.outerInnerBad(InnerClass$InnerClassA), 0, STARVATION, ERROR, [[Trace 1] Lock acquisition: locks this in class InnerClass*,Method call: void InnerClass$InnerClassA.baz(),Lock acquisition: locks this in class InnerClass$InnerClassA*,[Trace 2] Method start: InnerClass$InnerClassA.(InnerClass,Object),Lock acquisition: locks this in class InnerClass$InnerClassA*,Method call: void InnerClass.bar(),Lock acquisition: locks this in class InnerClass*] codetoanalyze/java/starvation/Interclass.java, void Interclass.interclass1_bad(InterclassA), 0, STARVATION, ERROR, [[Trace 1] Lock acquisition: locks this in class Interclass*,Method call: void InterclassA.interclass1_bad(),Lock acquisition: locks this in class InterclassA*,[Trace 2] Lock acquisition: locks this in class InterclassA*,Method call: void Interclass.interclass2_bad(),Lock acquisition: locks this in class Interclass*] codetoanalyze/java/starvation/Interproc.java, void Interproc.interproc1_bad(InterprocA), 0, STARVATION, ERROR, [[Trace 1] Lock acquisition: locks this in class Interproc*,Method call: void Interproc.interproc2_bad(InterprocA),Lock acquisition: locks b in class InterprocA*,[Trace 2] Lock acquisition: locks this in class InterprocA*,Method call: void InterprocA.interproc2_bad(Interproc),Lock acquisition: locks d in class Interproc*] codetoanalyze/java/starvation/Intraproc.java, void IntraprocA.intra_bad(Intraproc), 0, STARVATION, ERROR, [[Trace 1] Method start: void IntraprocA.intra_bad(Intraproc),Lock acquisition: locks this in class IntraprocA*,Lock acquisition: locks o in class Intraproc*,[Trace 2] Method start: void Intraproc.intra_bad(IntraprocA),Lock acquisition: locks this in class Intraproc*,Lock acquisition: locks o in class IntraprocA*]