From 427937083b780a4545381d83328931376aa2b973 Mon Sep 17 00:00:00 2001 From: Jules Villard Date: Wed, 19 May 2021 08:56:01 -0700 Subject: [PATCH] [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 | 18 +++---------- infer/src/pulse/PulseReport.ml | 26 ++++++++++++++++++ infer/src/pulse/PulseTrace.ml | 12 +++++++++ infer/src/pulse/PulseTrace.mli | 8 ++++-- .../tests/codetoanalyze/cpp/pulse/issues.exp | 1 + .../codetoanalyze/cpp/pulse/issues.exp-isl | 2 +- .../tests/codetoanalyze/cpp/pulse/nullptr.cpp | 27 ++++++++++++++++++- .../codetoanalyze/objcpp/pulse/NPEBasic.mm | 2 +- .../codetoanalyze/objcpp/pulse/issues.exp | 1 - 9 files changed, 76 insertions(+), 21 deletions(-) diff --git a/infer/src/pulse/PulseDiagnostic.ml b/infer/src/pulse/PulseDiagnostic.ml index 925fd7b0c..cdd7cc429 100644 --- a/infer/src/pulse/PulseDiagnostic.ml +++ b/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 _ -> diff --git a/infer/src/pulse/PulseReport.ml b/infer/src/pulse/PulseReport.ml index b06d3104f..23dce0753 100644 --- a/infer/src/pulse/PulseReport.ml +++ b/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 diff --git a/infer/src/pulse/PulseTrace.ml b/infer/src/pulse/PulseTrace.ml index 338ca5846..b358af277 100644 --- a/infer/src/pulse/PulseTrace.ml +++ b/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) diff --git a/infer/src/pulse/PulseTrace.mli b/infer/src/pulse/PulseTrace.mli index 2c83e64f6..bf003c286 100644 --- a/infer/src/pulse/PulseTrace.mli +++ b/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 *) diff --git a/infer/tests/codetoanalyze/cpp/pulse/issues.exp b/infer/tests/codetoanalyze/cpp/pulse/issues.exp index 8a4be7fa3..c2ea89e74 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/issues.exp +++ b/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] diff --git a/infer/tests/codetoanalyze/cpp/pulse/issues.exp-isl b/infer/tests/codetoanalyze/cpp/pulse/issues.exp-isl index 0734fb27c..5c188ef81 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/issues.exp-isl +++ b/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] diff --git a/infer/tests/codetoanalyze/cpp/pulse/nullptr.cpp b/infer/tests/codetoanalyze/cpp/pulse/nullptr.cpp index d52a8757f..17bbcd26e 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/nullptr.cpp +++ b/infer/tests/codetoanalyze/cpp/pulse/nullptr.cpp @@ -5,10 +5,11 @@ * LICENSE file in the root directory of this source tree. */ -#include #include #include +#include #include +#include void assign_zero_ok() { int x[2]; @@ -250,3 +251,27 @@ std::string global_const_skipped_function_ok() { std::shared_ptr 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()); +} diff --git a/infer/tests/codetoanalyze/objcpp/pulse/NPEBasic.mm b/infer/tests/codetoanalyze/objcpp/pulse/NPEBasic.mm index 3a351a7ed..6cb78b79b 100644 --- a/infer/tests/codetoanalyze/objcpp/pulse/NPEBasic.mm +++ b/infer/tests/codetoanalyze/objcpp/pulse/NPEBasic.mm @@ -301,7 +301,7 @@ void addInDictBracketsDefault(NSMutableDictionary* mDict, mDict[key] = @"default"; } -void accessZeroElementOk_FP(NSMutableDictionary* mDict) { +void accessZeroElementOk(NSMutableDictionary* mDict) { NSArray* array = [[NSUserDefaults standardUserDefaults] arrayForKey:@"key"]; NSString* key = array[0]; diff --git a/infer/tests/codetoanalyze/objcpp/pulse/issues.exp b/infer/tests/codetoanalyze/objcpp/pulse/issues.exp index 4fbfd6159..5c6ce284f 100644 --- a/infer/tests/codetoanalyze/objcpp/pulse/issues.exp +++ b/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]