[pulse] do not report null deref errors where the source of null is unclear

Summary:
As explained in the code comment, these reports are generally
non-actionable at best and false positives at worst:

   skip reporting for constant dereference (eg null dereference) if the source of the null value is
   not on the path of the access, otherwise the report will probably be too confusing: the actual
   source of the null value can be obscured as any value equal to 0 (or the constant) can be
   selected as the candidate for the trace, even if it has nothing to do with the error besides
   being equal to the value being dereferenced

Reviewed By: da319

Differential Revision: D28350193

fbshipit-source-id: 0cd76d252

infer/src/pulse/PulseDiagnostic.ml

@@ -45,18 +45,6 @@ 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
@@ -71,7 +59,7 @@ let get_message diagnostic =
   match diagnostic with
   | AccessToInvalidAddress {calling_context; invalidation; invalidation_trace; access_trace} -> (
       let invalidation, invalidation_trace =
-        get_invalidation_in_trace access_trace
+        Trace.get_invalidation access_trace
         |> Option.value_map
              ~f:(fun invalidation -> (invalidation, access_trace))
              ~default:(invalidation, invalidation_trace)
@@ -275,7 +263,7 @@ let add_access_trace ~include_title ~nesting invalidation access_trace errlog =
 let get_trace = function
   | AccessToInvalidAddress {calling_context; invalidation; invalidation_trace; access_trace} ->
       let in_context_nesting = List.length calling_context in
-      let should_print_invalidation_trace = not (trace_contains_invalidation access_trace) in
+      let should_print_invalidation_trace = not (Trace.has_invalidation access_trace) in
       get_trace_calling_context calling_context
       @@ ( if should_print_invalidation_trace then
            add_invalidation_trace ~nesting:in_context_nesting invalidation invalidation_trace
@@ -308,7 +296,7 @@ let get_trace = function
 let get_issue_type = function
   | AccessToInvalidAddress {invalidation; must_be_valid_reason; access_trace} ->
       let invalidation =
-        get_invalidation_in_trace access_trace |> Option.value ~default:invalidation
+        Trace.get_invalidation access_trace |> Option.value ~default:invalidation
       in
       Invalidation.issue_type_of_cause invalidation must_be_valid_reason
   | MemoryLeak _ ->

infer/src/pulse/PulseReport.ml

@@ -32,13 +32,39 @@ let report_latent_issue proc_desc err_log latent_issue =
   LatentIssue.to_diagnostic latent_issue |> report_latent_diagnostic proc_desc err_log
 
 
+(* skip reporting on Java classes annotated with [@Nullsafe] if requested *)
 let is_nullsafe_error tenv diagnostic jn =
   (not Config.pulse_nullsafe_report_npe)
   && IssueType.equal (Diagnostic.get_issue_type diagnostic) IssueType.nullptr_dereference
   && match NullsafeMode.of_java_procname tenv jn with Default -> false | Local _ | Strict -> true
 
 
+(* skip reporting for constant dereference (eg null dereference) if the source of the null value is
+   not on the path of the access, otherwise the report will probably be too confusing: the actual
+   source of the null value can be obscured as any value equal to 0 (or the constant) can be
+   selected as the candidate for the trace, even if it has nothing to do with the error besides
+   being equal to the value being dereferenced *)
+let is_constant_deref_without_invalidation (diagnostic : Diagnostic.t) =
+  match diagnostic with
+  | MemoryLeak _ | ReadUninitializedValue _ | StackVariableAddressEscape _ ->
+      false
+  | AccessToInvalidAddress {invalidation; access_trace} -> (
+    match invalidation with
+    | ConstantDereference _ ->
+        not (Trace.has_invalidation access_trace)
+    | CFree
+    | CppDelete
+    | EndIterator
+    | GoneOutOfScope _
+    | OptionalEmpty
+    | StdVector _
+    | JavaIterator _ ->
+        false )
+
+
 let is_suppressed tenv proc_desc diagnostic astate =
+  is_constant_deref_without_invalidation diagnostic
+  ||
   match Procdesc.get_proc_name proc_desc with
   | Procname.Java jn ->
       is_nullsafe_error tenv diagnostic jn

infer/src/pulse/PulseTrace.ml

@@ -7,6 +7,7 @@
 open! IStd
 module F = Format
 module CallEvent = PulseCallEvent
+module Invalidation = PulseInvalidation
 module ValueHistory = PulseValueHistory
 
 type t =
@@ -68,3 +69,14 @@ let rec iter trace ~f =
 let find_map trace ~f = Container.find_map ~iter trace ~f
 
 let exists trace ~f = Container.exists ~iter trace ~f
+
+let get_invalidation trace =
+  find_map trace ~f:(function
+    | ValueHistory.Invalidated (invalidation, _) ->
+        Some invalidation
+    | _ ->
+        None )
+
+
+let has_invalidation trace =
+  exists trace ~f:(function ValueHistory.Invalidated _ -> true | _ -> false)

infer/src/pulse/PulseTrace.mli

@@ -7,6 +7,7 @@
 open! IStd
 module F = Format
 module CallEvent = PulseCallEvent
+module Invalidation = PulseInvalidation
 module ValueHistory = PulseValueHistory
 
 type t =
@@ -38,5 +39,8 @@ val add_to_errlog :
 val find_map : t -> f:(ValueHistory.event -> 'a option) -> 'a option
 (** [find_map] applied to history events *)
 
-val exists : t -> f:(ValueHistory.event -> bool) -> bool
-(** [exists] applied to history events *)
+val get_invalidation : t -> Invalidation.t option
+(** return the first invalidation event of the trace, if any *)
+
+val has_invalidation : t -> bool
+(** whether the trace contains an invalidation event *)

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

@@ -57,6 +57,7 @@ codetoanalyze/cpp/pulse/join.cpp, invalidate_node_alias_bad, 1, USE_AFTER_DELETE
 codetoanalyze/cpp/pulse/join.cpp, invalidate_node_alias_latent, 12, NULLPTR_DEREFERENCE, no_bucket, ERROR, [*** LATENT ***,source of the null value part of the trace starts here,assigned,is the null pointer,null pointer dereference part of the trace starts here,parameter `head` of invalidate_node_alias_latent,assigned,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/join.cpp, invalidate_node_alias_latent, 12, USE_AFTER_DELETE, no_bucket, ERROR, [*** LATENT ***,invalidation part of the trace starts here,parameter `head` of invalidate_node_alias_latent,assigned,assigned,was invalidated by `delete`,use-after-lifetime part of the trace starts here,parameter `head` of invalidate_node_alias_latent,assigned,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/join.cpp, invalidate_node_alias_latent, 12, USE_AFTER_DELETE, no_bucket, ERROR, [*** LATENT ***,invalidation part of the trace starts here,parameter `head` of invalidate_node_alias_latent,assigned,assigned,assigned,was invalidated by `delete`,use-after-lifetime part of the trace starts here,parameter `head` of invalidate_node_alias_latent,assigned,assigned,assigned,invalid access occurs here]
+codetoanalyze/cpp/pulse/nullptr.cpp, SomeDerivedClass::SomeDerivedClass, 0, NULLPTR_DEREFERENCE, no_bucket, ERROR, [*** LATENT ***,source of the null value part of the trace starts here,parameter `ptr` of SomeDerivedClass::SomeDerivedClass,when calling `SomeClass::SomeClass` here,assigned,is the null pointer,null pointer dereference part of the trace starts here,parameter `ptr` of SomeDerivedClass::SomeDerivedClass,invalid access occurs here]
 codetoanalyze/cpp/pulse/nullptr.cpp, call_test_after_dereference2_bad, 1, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,when calling `test_after_dereference2_latent` here,parameter `x` of test_after_dereference2_latent,invalid access occurs here]
 codetoanalyze/cpp/pulse/nullptr.cpp, call_test_after_dereference_bad, 1, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,when calling `FN_test_after_dereference_latent` here,parameter `x` of FN_test_after_dereference_latent,invalid access occurs here]
 codetoanalyze/cpp/pulse/nullptr.cpp, deref_nullptr_bad, 2, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]

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

@@ -46,11 +46,11 @@ codetoanalyze/cpp/pulse/frontend.cpp, frontend::init_double_fields_struct_ok, 2,
 codetoanalyze/cpp/pulse/frontend.cpp, frontend::not_boolean_bad, 8, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/frontend.cpp, frontend::temp_passed_in_conditional_ok, 4, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/interprocedural.cpp, set_x_then_crash_double_latent, 4, NULLPTR_DEREFERENCE, no_bucket, ERROR, [*** LATENT ***,is the null pointer,assigned,invalid access occurs here]
-codetoanalyze/cpp/pulse/join.cpp, invalidate_node_alias_bad, 1, NULLPTR_DEREFERENCE, no_bucket, ERROR, [calling context starts here,in call to `invalidate_node_alias_latent`,source of the null value part of the trace starts here,assigned,is the null pointer,null pointer dereference part of the trace starts here,assigned,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/join.cpp, invalidate_node_alias_bad, 1, USE_AFTER_DELETE, no_bucket, ERROR, [calling context starts here,in call to `invalidate_node_alias_latent`,invalidation part of the trace starts here,assigned,assigned,was invalidated by `delete`,use-after-lifetime part of the trace starts here,assigned,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/join.cpp, invalidate_node_alias_latent, 12, NULLPTR_DEREFERENCE, no_bucket, ERROR, [*** LATENT ***,source of the null value part of the trace starts here,assigned,is the null pointer,null pointer dereference part of the trace starts here,assigned,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/join.cpp, invalidate_node_alias_latent, 12, USE_AFTER_DELETE, no_bucket, ERROR, [*** LATENT ***,invalidation part of the trace starts here,assigned,assigned,was invalidated by `delete`,use-after-lifetime part of the trace starts here,assigned,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/join.cpp, invalidate_node_alias_latent, 12, USE_AFTER_DELETE, no_bucket, ERROR, [*** LATENT ***,invalidation part of the trace starts here,assigned,assigned,assigned,was invalidated by `delete`,use-after-lifetime part of the trace starts here,assigned,assigned,assigned,invalid access occurs here]
+codetoanalyze/cpp/pulse/nullptr.cpp, SomeDerivedClass::SomeDerivedClass, 0, NULLPTR_DEREFERENCE, no_bucket, ERROR, [*** LATENT ***,source of the null value part of the trace starts here,when calling `SomeClass::SomeClass` here,assigned,is the null pointer,null pointer dereference part of the trace starts here,invalid access occurs here]
 codetoanalyze/cpp/pulse/nullptr.cpp, call_test_after_dereference2_bad, 1, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,when calling `test_after_dereference2_latent` here,invalid access occurs here]
 codetoanalyze/cpp/pulse/nullptr.cpp, call_test_after_dereference_bad, 1, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,when calling `FN_test_after_dereference_latent` here,invalid access occurs here]
 codetoanalyze/cpp/pulse/nullptr.cpp, deref_nullptr_bad, 2, NULLPTR_DEREFERENCE, no_bucket, ERROR, [is the null pointer,assigned,invalid access occurs here]

infer/tests/codetoanalyze/cpp/pulse/nullptr.cpp

@@ -5,10 +5,11 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-#include <type_traits>
 #include <atomic>
 #include <cstdlib>
+#include <memory>
 #include <string>
+#include <type_traits>
 
 void assign_zero_ok() {
   int x[2];
@@ -250,3 +251,27 @@ std::string global_const_skipped_function_ok() {
   std::shared_ptr<D> ptr(s);
   return ptr->asString();
 }
+
+struct SomeClass {
+  X* pointer_;
+  int field_init_to_zero_{0};
+  explicit SomeClass(X* ptr) : pointer_(ptr) {
+    if (pointer_) {
+    }
+  }
+};
+
+class SomeDerivedClass : public SomeClass {
+ public:
+  explicit SomeDerivedClass(X* ptr) : SomeClass(ptr) { ptr->foo(); }
+};
+
+X* unknown_function_X();
+
+// this creates a null derefer false positives with a non-sensical "forked"
+// trace (one sub-trace for where 0 came from, which in this case ends in the
+// unrelated "field_init_to_zero" assignment, and one sub-trace for the
+// dereference in ptr->foo() in the constructor), which should be ignored
+void createSomeDerivedClass_from_unknown_function_ok() {
+  SomeDerivedClass something(unknown_function_X());
+}

infer/tests/codetoanalyze/objcpp/pulse/NPEBasic.mm

@@ -301,7 +301,7 @@ void addInDictBracketsDefault(NSMutableDictionary<NSString*, NSString*>* mDict,
   mDict[key] = @"default";
 }
 
-void accessZeroElementOk_FP(NSMutableDictionary<NSString*, NSString*>* mDict) {
+void accessZeroElementOk(NSMutableDictionary<NSString*, NSString*>* mDict) {
   NSArray<NSString*>* array =
       [[NSUserDefaults standardUserDefaults] arrayForKey:@"key"];
   NSString* key = array[0];

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

@@ -1,5 +1,4 @@
 codetoanalyze/objcpp/pulse/AllocPatternMemLeak.mm, A.create_no_release_leak_bad, 2, MEMORY_LEAK, no_bucket, ERROR, [allocation part of the trace starts here,allocated by call to `ABFDataCreate` (modelled),allocation part of the trace ends here,memory becomes unreachable here]
-codetoanalyze/objcpp/pulse/NPEBasic.mm, accessZeroElementOk_FP, 4, NIL_INSERTION_INTO_COLLECTION, no_bucket, ERROR, [source of the null value part of the trace starts here,assigned,is the null pointer,null pointer dereference part of the trace starts here,assigned,in call to `NSArray.objectAtIndexedSubscript:`,parameter `self` of NSArray.objectAtIndexedSubscript:,return from call to `NSArray.objectAtIndexedSubscript:`,assigned,when calling `addInDictBracketsDefault` here,parameter `key` of addInDictBracketsDefault,in call to `mutableDictionary[someKey] = value` (modelled),invalid access occurs here]
 codetoanalyze/objcpp/pulse/NPEBasic.mm, addNilInArrayBad, 0, NIL_INSERTION_INTO_COLLECTION, no_bucket, ERROR, [is the null pointer,in call to `NSMutableArray.addObject:` (modelled),invalid access occurs here]
 codetoanalyze/objcpp/pulse/NPEBasic.mm, addNilInDictBad, 2, NIL_INSERTION_INTO_COLLECTION, no_bucket, ERROR, [is the null pointer,assigned,in call to `NSMutableDictionary.setObject:forKey:` (modelled),invalid access occurs here]
 codetoanalyze/objcpp/pulse/NPEBasic.mm, addNilKeyInDictBracketsBad, 2, NIL_INSERTION_INTO_COLLECTION, no_bucket, ERROR, [is the null pointer,assigned,in call to `mutableDictionary[someKey] = value` (modelled),invalid access occurs here]