[pulse] suppress leaks that are not leaks due to pointer arithmetic

Summary: This seems to be a popular source of false positives.

Reviewed By: skcho

Differential Revision: D28576767

fbshipit-source-id: d8d4d60d6

infer/src/istd/ISeq.ml

@@ -1,11 +0,0 @@
-(*
- * Copyright (c) Facebook, Inc. and its affiliates.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *)
-
-open! IStd
-
-let fold_result seq ~init ~f =
-  Seq.fold_left (fun acc_result x -> Result.bind acc_result ~f:(fun acc -> f acc x)) (Ok init) seq

infer/src/istd/ISeq.mli

@@ -1,11 +0,0 @@
-(*
- * Copyright (c) Facebook, Inc. and its affiliates.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *)
-
-open! IStd
-
-val fold_result :
-  'a Seq.t -> init:'accum -> f:('accum -> 'a -> ('accum, 'e) result) -> ('accum, 'e) result

infer/src/pulse/PulseAbductiveDomain.ml

@@ -610,7 +610,32 @@ let skipped_calls_match_pattern astate =
         astate.skipped_calls )
 
 
-let check_memory_leaks unreachable_addrs astate =
+let check_memory_leaks ~live_addresses ~unreachable_addresses astate =
+  let reaches_into addr addrs astate =
+    AbstractValue.Set.mem addr addrs
+    || BaseDomain.GraphVisit.fold_from_addresses (Seq.return addr) astate ~init:()
+         ~finish:(fun () -> (* didn't find any reachable address in [addrs] *) false)
+         ~f:(fun () addr' rev_accesses ->
+           (* We want to know if [addr] is still reachable from the value [addr'] by pointer
+              arithmetic, hence detect if [addr'] is an offset away from [addr]. In particular, if
+              we go through a [Dereference] then we have lost the connexion to the original
+              address value.
+
+              TODO: this should be part of [live_addresses] since all these addresses are actually
+              live. *)
+           let can_recover_root_from_accesses =
+             List.for_all rev_accesses ~f:(fun (access : BaseMemory.Access.t) ->
+                 match access with FieldAccess _ | ArrayAccess _ -> true | _ -> false )
+           in
+           if can_recover_root_from_accesses then
+             if AbstractValue.Set.mem addr' addrs then (
+               L.d_printfln ~color:Orange "Live address %a is reachable from %a" AbstractValue.pp
+                 addr' AbstractValue.pp addr ;
+               Stop true )
+             else Continue ()
+           else Stop false )
+       |> snd
+  in
   let check_memory_leak addr attributes =
     let allocated_not_freed_opt =
       let allocated = Attributes.get_allocation attributes in
@@ -625,24 +650,41 @@ let check_memory_leaks unreachable_addrs astate =
         (* allocated but not freed => leak *)
         L.d_printfln ~color:Red "LEAK: unreachable address %a was allocated by %a" AbstractValue.pp
           addr Procname.pp procname ;
-        (* HACK: last-chance check: it could be that the value is actually equal to a reachable address so it's not a leak *)
+        (* last-chance checks: it could be that the value (or its canonical equal) is reachable via
+           pointer arithmetic from live values. This is common in libraries that do their own memory
+           management, e.g. they return a field of the malloc()'d pointer, and the latter is a fat
+           pointer with, say, a reference count. For example in C:
+
+           {[
+            struct fat_int {
+              size_t count;
+              int data;
+            };
+
+            int *alloc_int() {
+              fat_int *p = malloc(...);
+              p->count = 1;
+              return &(p->data);
+            }
+           ]}
+
+           We don't have a precise enough memory model to understand everything that
+           goes on here but we should at least not report a leak. *)
         let addr_canon = PathCondition.get_both_var_repr astate.path_condition addr in
         if
-          Container.exists
+          reaches_into addr live_addresses (astate.post :> BaseDomain.t)
-            ~iter:(fun seq ~f -> Seq.iter f seq)
+          || reaches_into addr_canon live_addresses (astate.post :> BaseDomain.t)
-            unreachable_addrs ~f:(AbstractValue.equal addr_canon)
+        then (
-        then
+          L.d_printfln ~color:Orange
+            "False alarm: address is still reachable via other means; forgetting about its \
+             allocation site to prevent leak false positives." ;
+          Ok () )
+        else
           (* if the address became unreachable at a known point use that location *)
           let location = Attributes.get_unreachable_at attributes in
           Error (location, procname, trace)
-        else (
-          L.debug Analysis Quiet
-            "HUH? Address %a was going to leak but is equal to %a which is live; not raising an \
-             error."
-            AbstractValue.pp addr AbstractValue.pp addr_canon ;
-          Ok () )
   in
-  ISeq.fold_result unreachable_addrs ~init:() ~f:(fun () addr ->
+  List.fold_result unreachable_addresses ~init:() ~f:(fun () addr ->
       match AddressAttributes.find_opt addr astate with
       | Some unreachable_attrs ->
           check_memory_leak addr unreachable_attrs
@@ -682,15 +724,6 @@ let discard_unreachable_ ~for_summary ({pre; post} as astate) =
   (astate, pre_addresses, post_addresses, dead_addresses)
 
 
-(* exported in .mli *)
-let discard_unreachable astate =
-  let astate, _pre_addresses, _post_addresses, _dead_addresses =
-    discard_unreachable_ ~for_summary:false astate
-  in
-  (* NOTE: [_dead_addresses] is always the empty list when [for_summary] is [false] *)
-  astate
-
-
 let get_unreachable_attributes {post} =
   let post_addresses = BaseDomain.reachable_addresses (post :> BaseDomain.t) in
   BaseAddressAttributes.fold
@@ -951,14 +984,17 @@ let filter_for_summary tenv proc_name astate0 =
     else pre_live_addresses
   in
   let live_addresses = AbstractValue.Set.union pre_live_addresses post_live_addresses in
-  let+ path_condition, new_eqs =
+  let+ path_condition, live_via_arithmetic, new_eqs =
     PathCondition.simplify tenv
       ~get_dynamic_type:
         (BaseAddressAttributes.get_dynamic_type (astate_before_filter.post :> BaseDomain.t).attrs)
       ~can_be_pruned ~keep:live_addresses astate.path_condition
   in
+  let live_addresses = AbstractValue.Set.union live_addresses live_via_arithmetic in
   ( {astate with path_condition; topl= PulseTopl.simplify ~keep:live_addresses astate.topl}
-  , dead_addresses
+  , live_addresses
+  , (* we could filter out the [live_addresses] if needed; right now they might overlap *)
+    dead_addresses
   , new_eqs )
 
 
@@ -976,7 +1012,7 @@ let summary_of_post tenv pdesc location astate =
   let astate = {astate with path_condition} in
   let* astate, error = incorporate_new_eqs astate new_eqs in
   let astate_before_filter = astate in
-  let* astate, dead_addresses, new_eqs =
+  let* astate, live_addresses, dead_addresses, new_eqs =
     filter_for_summary tenv (Procdesc.get_proc_name pdesc) astate
   in
   let+ astate, error =
@@ -984,7 +1020,9 @@ let summary_of_post tenv pdesc location astate =
   in
   match error with
   | None -> (
-    match check_memory_leaks (Caml.List.to_seq dead_addresses) astate_before_filter with
+    match
+      check_memory_leaks ~live_addresses ~unreachable_addresses:dead_addresses astate_before_filter
+    with
     | Ok () ->
         Ok (invalidate_locals pdesc astate)
     | Error (unreachable_location, proc_name, trace) ->

infer/src/pulse/PulseAbductiveDomain.mli

@@ -160,9 +160,6 @@ val is_local : Var.t -> t -> bool
 
 val find_post_cell_opt : AbstractValue.t -> t -> BaseDomain.cell option
 
-val discard_unreachable : t -> t
-(** garbage collect unreachable addresses in the state to make it smaller and return the new state *)
-
 val get_unreachable_attributes : t -> AbstractValue.t list
 (** collect the addresses that have attributes but are unreachable in the current post-condition *)
 

infer/src/pulse/PulseFormula.ml

@@ -1878,7 +1878,7 @@ module DeadVariables = struct
       let closed_prunable_vars = get_reachable_from var_graph can_be_pruned in
       Atom.Set.filter (fun atom -> not (Atom.has_var_notin closed_prunable_vars atom)) phi.pruned
     in
-    {known; pruned; both}
+    ({known; pruned; both}, vars_to_keep)
 end
 
 let simplify tenv ~get_dynamic_type ~can_be_pruned ~keep phi =
@@ -1890,8 +1890,8 @@ let simplify tenv ~get_dynamic_type ~can_be_pruned ~keep phi =
   let+ phi = QuantifierElimination.eliminate_vars ~keep phi in
   (* TODO: doing [QuantifierElimination.eliminate_vars; DeadVariables.eliminate] a few times may
      eliminate even more variables *)
-  let phi = DeadVariables.eliminate ~can_be_pruned ~keep phi in
+  let phi, live_vars = DeadVariables.eliminate ~can_be_pruned ~keep phi in
-  (phi, new_eqs)
+  (phi, live_vars, new_eqs)
 
 
 let is_known_zero phi v =

infer/src/pulse/PulseFormula.mli

@@ -62,7 +62,7 @@ val simplify :
   -> can_be_pruned:Var.Set.t
   -> keep:Var.Set.t
   -> t
-  -> (t * new_eqs) SatUnsat.t
+  -> (t * Var.Set.t * new_eqs) SatUnsat.t
 
 val and_fold_subst_variables :
      t

infer/src/pulse/PulseOperations.ml

@@ -665,8 +665,7 @@ let remove_vars vars location astate =
         | _ ->
             astate )
   in
-  let astate' = Stack.remove_vars vars astate in
+  Stack.remove_vars vars astate
-  if phys_equal astate' astate then astate else AbductiveDomain.discard_unreachable astate'
 
 
 let get_captured_actuals location ~captured_vars ~actual_closure astate =

infer/src/pulse/PulsePathCondition.ml

@@ -108,17 +108,20 @@ let and_eq_vars v1 v2 phi =
 
 
 let simplify tenv ~can_be_pruned ~keep ~get_dynamic_type phi =
-  let result =
+  if phi.is_unsat then Unsat
-    let+ {is_unsat; bo_itvs; citvs; formula} = phi in
+  else
-    let+| formula, new_eqs = Formula.simplify tenv ~can_be_pruned ~keep ~get_dynamic_type formula in
+    let {is_unsat; bo_itvs; citvs; formula} = phi in
+    let open SatUnsat.Import in
+    let+ formula, live_vars, new_eqs =
+      Formula.simplify tenv ~can_be_pruned ~keep ~get_dynamic_type formula
+    in
     let is_in_keep v _ = AbstractValue.Set.mem v keep in
     ( { is_unsat
       ; bo_itvs= BoItvs.filter is_in_keep bo_itvs
       ; citvs= CItvs.filter is_in_keep citvs
       ; formula }
+    , live_vars
     , new_eqs )
-  in
-  if (fst result).is_unsat then Unsat else Sat result
 
 
 let subst_find_or_new subst addr_callee =

infer/src/pulse/PulsePathCondition.mli

@@ -40,7 +40,7 @@ val simplify :
   -> keep:AbstractValue.Set.t
   -> get_dynamic_type:(AbstractValue.t -> Typ.t option)
   -> t
-  -> (t * new_eqs) SatUnsat.t
+  -> (t * AbstractValue.Set.t * new_eqs) SatUnsat.t
 (** [simplify ~can_be_pruned ~keep phi] attempts to get rid of as many variables in [fv phi] but not
     in [keep] as possible, and tries to eliminate variables not in [can_be_pruned] from the "pruned"
     part of the formula *)

infer/src/pulse/unit/PulseFormulaTest.ml

@@ -119,21 +119,24 @@ let normalized_pp fmt = function
 
 let test ~f phi =
   AbstractValue.State.set init_vars_state ;
-  phi ttrue >>= f >>| fst |> F.printf "%a" normalized_pp
+  phi ttrue >>= f |> F.printf "%a" normalized_pp
 
 
 let dummy_tenv = Tenv.create ()
 
 let dummy_get_dynamic_type _ = None
 
-let normalize phi = test ~f:(normalize dummy_tenv ~get_dynamic_type:dummy_get_dynamic_type) phi
+let normalize phi =
+  test ~f:(fun phi -> normalize dummy_tenv ~get_dynamic_type:dummy_get_dynamic_type phi >>| fst) phi
+
 
 let simplify ~keep phi =
   let keep = AbstractValue.Set.of_list keep in
   test phi ~f:(fun phi ->
       (* keep variables as if they were in the pre-condition, which makes [simplify] keeps the most
          facts (eg atoms in [pruned] may be discarded if their variables are not in the pre) *)
-      simplify dummy_tenv ~get_dynamic_type:dummy_get_dynamic_type ~can_be_pruned:keep ~keep phi )
+      simplify dummy_tenv ~get_dynamic_type:dummy_get_dynamic_type ~can_be_pruned:keep ~keep phi
+      >>| fst3 )
 
 
 let%test_module "normalization" =

infer/tests/codetoanalyze/c/pulse/issues.exp

@@ -14,8 +14,6 @@ codetoanalyze/c/pulse/latent.c, latent_use_after_free, 4, NULLPTR_DEREFERENCE, n
 codetoanalyze/c/pulse/latent.c, latent_use_after_free, 4, USE_AFTER_FREE, no_bucket, ERROR, [*** LATENT ***,invalidation part of the trace starts here,parameter `x` of latent_use_after_free,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,parameter `x` of latent_use_after_free,invalid access occurs here]
 codetoanalyze/c/pulse/latent.c, main, 3, USE_AFTER_FREE, no_bucket, ERROR, [calling context starts here,in call to `latent_use_after_free`,invalidation part of the trace starts here,parameter `x` of latent_use_after_free,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,parameter `x` of latent_use_after_free,invalid access occurs here]
 codetoanalyze/c/pulse/latent.c, manifest_use_after_free, 0, USE_AFTER_FREE, no_bucket, ERROR, [calling context starts here,in call to `latent_use_after_free`,invalidation part of the trace starts here,parameter `x` of latent_use_after_free,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,parameter `x` of latent_use_after_free,invalid access occurs here]
-codetoanalyze/c/pulse/memory_leak.c, FP_alloc_ref_counted_arith_ok, 4, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
-codetoanalyze/c/pulse/memory_leak.c, FP_alloc_ref_counted_ok, 5, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, alloc_ref_counted_bad, 6, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, malloc_formal_leak_bad, 0, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, malloc_interproc_no_free_bad, 0, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,when calling `create_p` here,allocated by `malloc` here,memory becomes unreachable here]
@@ -28,6 +26,7 @@ codetoanalyze/c/pulse/memory_leak.c, malloc_ptr_no_check_leak_bad, 2, NULLPTR_DE
 codetoanalyze/c/pulse/memory_leak.c, realloc_no_check_bad, 3, NULLPTR_DEREFERENCE, no_bucket, ERROR, [in call to `malloc` (modelled),is the null pointer,assigned,in call to `realloc_wrapper`,in call to `realloc` (modelled),is the null pointer,returned,return from call to `realloc_wrapper`,assigned,invalid access occurs here]
 codetoanalyze/c/pulse/memory_leak.c, realloc_no_free_bad, 2, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,when calling `realloc_wrapper` here,allocated by `realloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, report_leak_in_correct_line_bad, 2, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
+codetoanalyze/c/pulse/memory_leak.c, return_malloc_deref_bad, 4, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, test_config_options_no_free_bad, 2, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `my_realloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, user_malloc_leak_bad, 0, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `a_malloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/nullptr.c, bug_after_abduction_bad, 3, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]

infer/tests/codetoanalyze/c/pulse/issues.exp-isl

@@ -14,8 +14,6 @@ codetoanalyze/c/pulse/latent.c, latent_use_after_free, 4, NULLPTR_DEREFERENCE, n
 codetoanalyze/c/pulse/latent.c, latent_use_after_free, 4, USE_AFTER_FREE, no_bucket, ERROR, [*** LATENT ***,invalidation part of the trace starts here,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,invalid access occurs here]
 codetoanalyze/c/pulse/latent.c, main, 3, USE_AFTER_FREE, no_bucket, ERROR, [calling context starts here,in call to `latent_use_after_free`,invalidation part of the trace starts here,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,invalid access occurs here]
 codetoanalyze/c/pulse/latent.c, manifest_use_after_free, 0, USE_AFTER_FREE, no_bucket, ERROR, [calling context starts here,in call to `latent_use_after_free`,invalidation part of the trace starts here,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,invalid access occurs here]
-codetoanalyze/c/pulse/memory_leak.c, FP_alloc_ref_counted_arith_ok, 4, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
-codetoanalyze/c/pulse/memory_leak.c, FP_alloc_ref_counted_ok, 5, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, alloc_ref_counted_bad, 6, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, malloc_formal_leak_bad, 0, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, malloc_interproc_no_free_bad, 0, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,when calling `create_p` here,allocated by `malloc` here,memory becomes unreachable here]
@@ -28,6 +26,7 @@ codetoanalyze/c/pulse/memory_leak.c, malloc_ptr_no_check_leak_bad, 2, NULLPTR_DE
 codetoanalyze/c/pulse/memory_leak.c, realloc_no_check_bad, 3, NULLPTR_DEREFERENCE, no_bucket, ERROR, [in call to `malloc` (modelled),is the null pointer,assigned,in call to `realloc_wrapper`,in call to `realloc` (modelled),is the null pointer,returned,return from call to `realloc_wrapper`,assigned,invalid access occurs here]
 codetoanalyze/c/pulse/memory_leak.c, realloc_no_free_bad, 2, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,when calling `realloc_wrapper` here,allocated by `realloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, report_leak_in_correct_line_bad, 2, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
+codetoanalyze/c/pulse/memory_leak.c, return_malloc_deref_bad, 4, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `malloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, test_config_options_no_free_bad, 2, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `my_realloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/memory_leak.c, user_malloc_leak_bad, 0, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by `a_malloc` here,memory becomes unreachable here]
 codetoanalyze/c/pulse/nullptr.c, bug_after_abduction_bad, 3, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]

infer/tests/codetoanalyze/c/pulse/memory_leak.c

@@ -153,7 +153,7 @@ struct ref_counted {
 
 // the address of the malloc()'d pointer can be retrieved from the
 // return value using pointer arithmetic
-int* FP_alloc_ref_counted_ok() {
+int* alloc_ref_counted_ok() {
   struct ref_counted* p =
       (struct ref_counted*)malloc(sizeof(struct ref_counted));
   if (p) {
@@ -179,7 +179,7 @@ int alloc_ref_counted_bad() {
 
 // the address of the malloc()'d pointer can be retrieved from the
 // return value using pointer arithmetic
-void* FP_alloc_ref_counted_arith_ok(size_t size) {
+void* alloc_ref_counted_arith_ok(size_t size) {
   int* p = (int*)malloc(size + sizeof(int));
   if (p) {
     // register count = 1 and point past the ref count
@@ -187,3 +187,12 @@ void* FP_alloc_ref_counted_arith_ok(size_t size) {
   }
   return p;
 }
+
+int return_malloc_deref_bad() {
+  int* p = (int*)malloc(sizeof(int));
+  if (p) {
+    *p = 42;
+    return *p;
+  }
+  return 10;
+}