[pulse] take histories into account for all aspects of a report

Summary:
A previous change made pulse look into value histories for causes of
invalidation in case the access trace of a value already contained the
reason why that value is invalid, in order to save printing the
invalidation trace in addition to the access trace. It also made
reporting more accurate for null dereference as the source of null was
often better identified (in cases where several values are null or
zero).

But, the history is also relevant to the bug type and the error message.
Make these take histories into account too.

Also fix a bug where we didn't look inside the sub-histories contained
within function calls when looking for an invalidation along the
history.

Reviewed By: da319

Differential Revision: D28254334

fbshipit-source-id: 5ca00ee54

infer/src/pulse/PulseDiagnostic.ml

@@ -45,6 +45,18 @@ let get_location = function
       location
 
 
+let get_invalidation_in_trace trace =
+  PulseTrace.find_map trace ~f:(function
+    | ValueHistory.Invalidated (invalidation, _) ->
+        Some invalidation
+    | _ ->
+        None )
+
+
+let trace_contains_invalidation trace =
+  PulseTrace.exists trace ~f:(function ValueHistory.Invalidated _ -> true | _ -> false)
+
+
 (* whether the [calling_context + trace] starts with a call or contains only an immediate event *)
 let immediate_or_first_call calling_context (trace : Trace.t) =
   match (calling_context, trace) with
@@ -58,6 +70,12 @@ let get_message diagnostic =
   let pulse_start_msg = "Pulse found a potential" in
   match diagnostic with
   | AccessToInvalidAddress {calling_context; invalidation; invalidation_trace; access_trace} -> (
+      let invalidation, invalidation_trace =
+        get_invalidation_in_trace access_trace
+        |> Option.value_map
+             ~f:(fun invalidation -> (invalidation, access_trace))
+             ~default:(invalidation, invalidation_trace)
+      in
       match invalidation with
       | ConstantDereference i when IntLit.equal i IntLit.zero ->
           (* Special error message for nullptr dereference *)
@@ -212,10 +230,6 @@ let get_trace_calling_context calling_context errlog =
          |> fst )
 
 
-let trace_contains_invalidation trace =
-  PulseTrace.exists trace ~f:(function ValueHistory.Invalidated _ -> true | _ -> false)
-
-
 let add_invalidation_trace (invalidation : Invalidation.t) invalidation_trace access_trace errlog =
   let start_title, access_title =
     match invalidation with
@@ -275,7 +289,10 @@ let get_trace = function
 
 
 let get_issue_type = function
-  | AccessToInvalidAddress {invalidation; must_be_valid_reason} ->
+  | AccessToInvalidAddress {invalidation; must_be_valid_reason; access_trace} ->
+      let invalidation =
+        get_invalidation_in_trace access_trace |> Option.value ~default:invalidation
+      in
       Invalidation.issue_type_of_cause invalidation must_be_valid_reason
   | MemoryLeak _ ->
       IssueType.pulse_memory_leak

infer/src/pulse/PulseModels.ml

@@ -312,7 +312,7 @@ module Optional = struct
     let<+> astate =
       PulseOperations.invalidate
         (MemoryAccess {pointer; access= Dereference; hist_obj_default= snd value})
-        location Invalidation.OptionalEmpty value astate
+        location OptionalEmpty value astate
     in
     astate
 

infer/src/pulse/PulseTrace.ml

@@ -59,9 +59,9 @@ let rec add_to_errlog ?(include_value_history = true) ~nesting ~pp_immediate tra
 let rec iter trace ~f =
   match trace with
   | Immediate {history} ->
-      List.iter history ~f
+      ValueHistory.iter history ~f
   | ViaCall {history; in_call} ->
-      List.iter history ~f ;
+      ValueHistory.iter history ~f ;
       iter in_call ~f
 
 

infer/src/pulse/PulseValueHistory.ml

@@ -25,6 +25,26 @@ type event =
 
 and t = event list [@@deriving compare, equal]
 
+let rec iter_event event ~f =
+  f event ;
+  match event with
+  | Call {in_call} ->
+      iter in_call ~f
+  | Allocation _
+  | Assignment _
+  | Capture _
+  | Conditional _
+  | CppTemporaryCreated _
+  | FormalDeclared _
+  | Invalidated _
+  | StructFieldAddressCreated _
+  | VariableAccessed _
+  | VariableDeclared _ ->
+      ()
+
+
+and iter history ~f = List.iter history ~f:(fun event -> iter_event ~f event)
+
 let yojson_of_event = [%yojson_of: _]
 
 let yojson_of_t = [%yojson_of: _]

infer/src/pulse/PulseValueHistory.mli

@@ -27,6 +27,9 @@ val pp : F.formatter -> t -> unit
 
 val pp_fields : F.formatter -> Fieldname.t RevList.t -> unit
 
+val iter : t -> f:(event -> unit) -> unit
+(** iterate on all events, recursing into the histories inside call events *)
+
 val location_of_event : event -> Location.t
 
 val add_to_errlog : nesting:int -> t -> Errlog.loc_trace_elem list -> Errlog.loc_trace_elem list

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

@@ -64,13 +64,13 @@ codetoanalyze/cpp/pulse/nullptr.cpp, no_check_return_bad, 2, NULLPTR_DEREFERENCE
 codetoanalyze/cpp/pulse/nullptr.cpp, std_false_type_deref_bad, 3, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/nullptr.cpp, std_true_type_deref_bad, 3, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/nullptr.cpp, test_after_dereference2_latent, 1, NULLPTR_DEREFERENCE, no_bucket, ERROR, [*** LATENT ***,source of the null value part of the trace starts here,is the null pointer,null pointer dereference part of the trace starts here,parameter `x` of test_after_dereference2_latent,invalid access occurs here]
-codetoanalyze/cpp/pulse/optional.cpp, FP_smart_pointer, 2, NULLPTR_DEREFERENCE, no_bucket, ERROR, [source of the null value part of the trace starts here,when calling `Node::getShared` here,assigned,is the null pointer,null pointer dereference part of the trace starts here,passed as argument to `Node::getShared`,passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,return from call to `Node::getShared`,invalid access occurs here]
+codetoanalyze/cpp/pulse/optional.cpp, FP_smart_pointer, 2, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `Node::getShared`,passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,return from call to `Node::getShared`,invalid access occurs here]
-codetoanalyze/cpp/pulse/optional.cpp, FP_std_value_or_check_value_ok, 5, NULLPTR_DEREFERENCE, no_bucket, ERROR, [passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),is optional empty,invalid access occurs here]
+codetoanalyze/cpp/pulse/optional.cpp, FP_std_value_or_check_value_ok, 5, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),is optional empty,invalid access occurs here]
-codetoanalyze/cpp/pulse/optional.cpp, FP_value_or_check_value_ok, 5, NULLPTR_DEREFERENCE, no_bucket, ERROR, [passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,invalid access occurs here]
+codetoanalyze/cpp/pulse/optional.cpp, FP_value_or_check_value_ok, 5, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,invalid access occurs here]
-codetoanalyze/cpp/pulse/optional.cpp, assign2_bad, 4, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [invalidation part of the trace starts here,when calling `folly::Optional<int>::operator=` here,passed as argument to `folly::Optional::reset()` (modelled),return from call to `folly::Optional::reset()` (modelled),is optional empty,use-after-lifetime part of the trace starts here,passed as argument to `folly::Optional<int>::operator=`,passed as argument to `folly::Optional::reset()` (modelled),return from call to `folly::Optional::reset()` (modelled),is optional empty,return from call to `folly::Optional<int>::operator=`,invalid access occurs here]
+codetoanalyze/cpp/pulse/optional.cpp, assign2_bad, 4, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `folly::Optional<int>::operator=`,passed as argument to `folly::Optional::reset()` (modelled),return from call to `folly::Optional::reset()` (modelled),is optional empty,return from call to `folly::Optional<int>::operator=`,invalid access occurs here]
 codetoanalyze/cpp/pulse/optional.cpp, assign_bad, 5, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,passed as argument to `folly::Optional<int>::operator=`,parameter `other` of folly::Optional<int>::operator=,passed as argument to `folly::Optional::assign(folly::Optional<Value> arg)` (modelled),return from call to `folly::Optional::assign(folly::Optional<Value> arg)` (modelled),return from call to `folly::Optional<int>::operator=`,invalid access occurs here]
-codetoanalyze/cpp/pulse/optional.cpp, get_pointer_no_check_none_check_bad, 3, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `folly::Optional::get_pointer()` (modelled),return from call to `folly::Optional::get_pointer()` (modelled),is the null pointer,assigned,invalid access occurs here]
+codetoanalyze/cpp/pulse/optional.cpp, get_pointer_no_check_none_check_bad, 3, NULLPTR_DEREFERENCE, no_bucket, ERROR, [passed as argument to `folly::Optional::get_pointer()` (modelled),return from call to `folly::Optional::get_pointer()` (modelled),is the null pointer,assigned,invalid access occurs here]
-codetoanalyze/cpp/pulse/optional.cpp, inside_try_catch_FP, 3, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [invalidation part of the trace starts here,when calling `might_return_none` here,passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),is optional empty,use-after-lifetime part of the trace starts here,passed as argument to `might_return_none`,passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),is optional empty,return from call to `might_return_none`,invalid access occurs here]
+codetoanalyze/cpp/pulse/optional.cpp, inside_try_catch_FP, 3, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `might_return_none`,passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),is optional empty,return from call to `might_return_none`,invalid access occurs here]
 codetoanalyze/cpp/pulse/optional.cpp, none_copy_bad, 3, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,passed as argument to `folly::Optional::Optional(folly::Optional<Value> arg)` (modelled),return from call to `folly::Optional::Optional(folly::Optional<Value> arg)` (modelled),invalid access occurs here]
 codetoanalyze/cpp/pulse/optional.cpp, none_no_check_bad, 2, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,invalid access occurs here]
 codetoanalyze/cpp/pulse/optional.cpp, not_none_check_value_ok_FP, 5, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `folly::Optional::Optional(=None)` (modelled),return from call to `folly::Optional::Optional(=None)` (modelled),is optional empty,invalid access occurs here]
@@ -79,7 +79,7 @@ codetoanalyze/cpp/pulse/optional.cpp, std_assign_bad, 5, OPTIONAL_EMPTY_ACCESS,
 codetoanalyze/cpp/pulse/optional.cpp, std_none_copy_bad, 3, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),is optional empty,passed as argument to `std::optional::optional(std::optional<Value> arg)` (modelled),return from call to `std::optional::optional(std::optional<Value> arg)` (modelled),invalid access occurs here]
 codetoanalyze/cpp/pulse/optional.cpp, std_none_no_check_bad, 2, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),is optional empty,invalid access occurs here]
 codetoanalyze/cpp/pulse/optional.cpp, std_not_none_check_value_ok_FP, 5, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `std::optional::optional(=nullopt)` (modelled),return from call to `std::optional::optional(=nullopt)` (modelled),is optional empty,invalid access occurs here]
-codetoanalyze/cpp/pulse/optional.cpp, test_trace_ref, 4, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [invalidation part of the trace starts here,when calling `folly::Optional<int>::operator=` here,passed as argument to `folly::Optional::reset()` (modelled),return from call to `folly::Optional::reset()` (modelled),is optional empty,use-after-lifetime part of the trace starts here,passed as argument to `folly::Optional<int>::operator=`,passed as argument to `folly::Optional::reset()` (modelled),return from call to `folly::Optional::reset()` (modelled),is optional empty,return from call to `folly::Optional<int>::operator=`,invalid access occurs here]
+codetoanalyze/cpp/pulse/optional.cpp, test_trace_ref, 4, OPTIONAL_EMPTY_ACCESS, no_bucket, ERROR, [passed as argument to `folly::Optional<int>::operator=`,passed as argument to `folly::Optional::reset()` (modelled),return from call to `folly::Optional::reset()` (modelled),is optional empty,return from call to `folly::Optional<int>::operator=`,invalid access occurs here]
 codetoanalyze/cpp/pulse/path.cpp, faulty_call_bad, 0, NULLPTR_DEREFERENCE, no_bucket, ERROR, [calling context starts here,in call to `only_bad_on_42_latent`,source of the null value part of the trace starts here,when calling `may_return_null` here,assigned,is the null pointer,null pointer dereference part of the trace starts here,passed as argument to `may_return_null`,return from call to `may_return_null`,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/path.cpp, only_bad_on_42_latent, 2, NULLPTR_DEREFERENCE, no_bucket, ERROR, [*** LATENT ***,source of the null value part of the trace starts here,when calling `may_return_null` here,assigned,is the null pointer,null pointer dereference part of the trace starts here,passed as argument to `may_return_null`,return from call to `may_return_null`,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/reference_wrapper.cpp, reference_wrapper_heap_bad, 2, USE_AFTER_DELETE, no_bucket, ERROR, [invalidation part of the trace starts here,when calling `getwrapperHeap` here,passed as argument to `WrapsB::WrapsB`,passed as argument to `new` (modelled),return from call to `new` (modelled),assigned,return from call to `WrapsB::WrapsB`,when calling `WrapsB::~WrapsB` here,parameter `this` of WrapsB::~WrapsB,when calling `WrapsB::__infer_inner_destructor_~WrapsB` here,parameter `this` of WrapsB::__infer_inner_destructor_~WrapsB,was invalidated by `delete`,use-after-lifetime part of the trace starts here,passed as argument to `getwrapperHeap`,passed as argument to `WrapsB::WrapsB`,passed as argument to `new` (modelled),return from call to `new` (modelled),assigned,return from call to `WrapsB::WrapsB`,passed as argument to `ReferenceWrapperHeap::ReferenceWrapperHeap`,parameter `a` of ReferenceWrapperHeap::ReferenceWrapperHeap,passed as argument to `WrapsB::getb`,return from call to `WrapsB::getb`,assigned,return from call to `ReferenceWrapperHeap::ReferenceWrapperHeap`,return from call to `getwrapperHeap`,invalid access occurs here]

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

@@ -4,7 +4,6 @@ codetoanalyze/java/pulse/AnalysisStops.java, codetoanalyze.java.infer.AnalysisSt
 codetoanalyze/java/pulse/Builtins.java, codetoanalyze.java.infer.Builtins.FP_blockErrorIntAssumeOk(java.lang.Object):void, 4, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]
 codetoanalyze/java/pulse/Builtins.java, codetoanalyze.java.infer.Builtins.FP_blockErrorOk():void, 3, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]
 codetoanalyze/java/pulse/Builtins.java, codetoanalyze.java.infer.Builtins.doNotBlockErrorBad(java.lang.Object):void, 3, NULLPTR_DEREFERENCE, no_bucket, ERROR, [*** LATENT ***,is the null pointer,assigned,invalid access occurs here]
-codetoanalyze/java/pulse/Builtins.java, codetoanalyze.java.infer.Builtins.doNotBlockErrorBad(java.lang.Object):void, 3, NULLPTR_DEREFERENCE, no_bucket, ERROR, [*** LATENT ***,is the null pointer,assigned,invalid access occurs here]
 codetoanalyze/java/pulse/DefaultInInterface.java, DefaultInInterface$A.defaultCallNPE():void, 1, NULLPTR_DEREFERENCE, no_bucket, ERROR, [source of the null value part of the trace starts here,when calling `Object DefaultInInterface$I.defaultMethod1()` here,assigned,is the null pointer,null pointer dereference part of the trace starts here,passed as argument to `Object DefaultInInterface$I.defaultMethod1()`,return from call to `Object DefaultInInterface$I.defaultMethod1()`,assigned,invalid access occurs here]
 codetoanalyze/java/pulse/DefaultInInterface.java, DefaultInInterface$B.overridenCallNPE():void, 1, NULLPTR_DEREFERENCE, no_bucket, ERROR, [source of the null value part of the trace starts here,when calling `Object DefaultInInterface$B.defaultMethod2()` here,assigned,is the null pointer,null pointer dereference part of the trace starts here,passed as argument to `Object DefaultInInterface$B.defaultMethod2()`,return from call to `Object DefaultInInterface$B.defaultMethod2()`,assigned,invalid access occurs here]
 codetoanalyze/java/pulse/DefaultInInterface.java, DefaultInInterface.uncertainCallMethod1NPE_latent(int):void, 5, NULLPTR_DEREFERENCE, no_bucket, ERROR, [*** LATENT ***,source of the null value part of the trace starts here,when calling `Object DefaultInInterface$I.defaultMethod1()` here,assigned,is the null pointer,null pointer dereference part of the trace starts here,passed as argument to `Object DefaultInInterface$I.defaultMethod1()`,return from call to `Object DefaultInInterface$I.defaultMethod1()`,assigned,invalid access occurs here]
@@ -64,7 +63,7 @@ codetoanalyze/java/pulse/NullPointerExceptions.java, codetoanalyze.java.infer.Nu
 codetoanalyze/java/pulse/NullPointerExceptions.java, codetoanalyze.java.infer.NullPointerExceptions.someNPEAfterResourceLeak():void, 2, NULLPTR_DEREFERENCE, no_bucket, ERROR, [source of the null value part of the trace starts here,when calling `T CloseableAsResourceExample.sourceOfNullWithResourceLeak()` here,assigned,is the null pointer,null pointer dereference part of the trace starts here,passed as argument to `T CloseableAsResourceExample.sourceOfNullWithResourceLeak()`,return from call to `T CloseableAsResourceExample.sourceOfNullWithResourceLeak()`,assigned,invalid access occurs here]
 codetoanalyze/java/pulse/NullPointerExceptions.java, codetoanalyze.java.infer.NullPointerExceptions.stringConstantEqualsFalseNotNPE_FP():void, 10, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]
 codetoanalyze/java/pulse/NullPointerExceptions.java, codetoanalyze.java.infer.NullPointerExceptions.stringVarEqualsFalseNPE():void, 5, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]
-codetoanalyze/java/pulse/NullSafeExample.java, OtherClass.buggyMethodBad():java.lang.String, 2, NULLPTR_DEREFERENCE, no_bucket, ERROR, [source of the null value part of the trace starts here,when calling `OtherClass.<init>()` here,assigned,is the null pointer,null pointer dereference part of the trace starts here,passed as argument to `OtherClass.<init>()`,is the null pointer,assigned,return from call to `OtherClass.<init>()`,passed as argument to `OtherClass OtherClass.canReturnNull()`,return from call to `OtherClass OtherClass.canReturnNull()`,assigned,invalid access occurs here]
+codetoanalyze/java/pulse/NullSafeExample.java, OtherClass.buggyMethodBad():java.lang.String, 2, NULLPTR_DEREFERENCE, no_bucket, ERROR, [passed as argument to `OtherClass.<init>()`,is the null pointer,assigned,return from call to `OtherClass.<init>()`,passed as argument to `OtherClass OtherClass.canReturnNull()`,return from call to `OtherClass OtherClass.canReturnNull()`,assigned,invalid access occurs here]
 codetoanalyze/java/pulse/PreconditionsExample.java, codetoanalyze.java.infer.PreconditionsExample.checkArgumentSatBad():void, 4, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]
 codetoanalyze/java/pulse/PreconditionsExample.java, codetoanalyze.java.infer.PreconditionsExample.checkStateConditionSatBad():void, 5, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]
 codetoanalyze/java/pulse/PreconditionsExample.java, codetoanalyze.java.infer.PreconditionsExample.testCheckNotNullArgLatent(java.lang.Object):void, 3, NULLPTR_DEREFERENCE, no_bucket, ERROR, [*** LATENT ***,is the null pointer,assigned,invalid access occurs here]