From 32deab86bd53e991347406f4c44d86f111f48f02 Mon Sep 17 00:00:00 2001 From: Jeremy Dubreil Date: Thu, 14 Dec 2017 20:01:41 -0800 Subject: [PATCH] [infer][nullable checker] avoid reporting the nullable violations that are already reported by the biabduction analysis Summary: This is to allow the bi-abduction analysis and the nullable checker for Clang languages to run together without stepping on each other toes. Reviewed By: sblackshear Differential Revision: D6567934 fbshipit-source-id: a318c33 --- Makefile | 3 +- infer/src/IR/Errlog.ml | 1 - infer/src/IR/Localise.ml | 8 +- infer/src/IR/Localise.mli | 3 + infer/src/backend/InferPrint.ml | 9 +- infer/src/checkers/NullabilityCheck.ml | 145 ++++++++++-------- infer/src/checkers/registerCheckers.ml | 12 +- .../codetoanalyze/cpp/conflicts/Makefile | 20 +++ .../codetoanalyze/cpp/conflicts/issues.exp | 3 + .../codetoanalyze/cpp/conflicts/test.cpp | 34 ++++ 10 files changed, 157 insertions(+), 81 deletions(-) create mode 100644 infer/tests/codetoanalyze/cpp/conflicts/Makefile create mode 100644 infer/tests/codetoanalyze/cpp/conflicts/issues.exp create mode 100644 infer/tests/codetoanalyze/cpp/conflicts/test.cpp diff --git a/Makefile b/Makefile index 1ab70e336..6abc29418 100644 --- a/Makefile +++ b/Makefile @@ -49,7 +49,8 @@ BUILD_SYSTEMS_TESTS += \ DIRECT_TESTS += \ c_biabduction c_bufferoverrun c_errors c_frontend \ - cpp_bufferoverrun cpp_errors cpp_frontend cpp_liveness cpp_quandary cpp_racerd cpp_siof cpp_uninit cpp_nullable \ + cpp_bufferoverrun cpp_errors cpp_frontend cpp_liveness cpp_quandary \ + cpp_racerd cpp_siof cpp_uninit cpp_nullable cpp_conflicts \ ifneq ($(BUCK),no) BUILD_SYSTEMS_TESTS += buck-clang-db buck_flavors buck_flavors_run buck_flavors_deterministic diff --git a/infer/src/IR/Errlog.ml b/infer/src/IR/Errlog.ml index c46bf42a6..449d997bf 100644 --- a/infer/src/IR/Errlog.ml +++ b/infer/src/IR/Errlog.ml @@ -147,7 +147,6 @@ let fold (f: err_key -> err_data -> 'a -> 'a) t acc = (fun err_key set acc -> ErrDataSet.fold (fun err_data acc -> f err_key err_data acc) set acc) t acc - (** Return the number of elements in the error log which satisfy [filter] *) let size filter (err_log: t) = let count = ref 0 in diff --git a/infer/src/IR/Localise.ml b/infer/src/IR/Localise.ml index b28d7c024..8c8dd3ca8 100644 --- a/infer/src/IR/Localise.ml +++ b/infer/src/IR/Localise.ml @@ -195,6 +195,13 @@ let error_desc_set_bucket err_desc bucket = {err_desc with descriptions; tags} +let error_desc_is_reportable_bucket err_desc = + let issue_bucket = error_desc_get_bucket err_desc in + let high_buckets = BucketLevel.([b1; b2]) in + Option.value_map issue_bucket ~default:false ~f:(fun b -> + List.mem ~equal:String.equal high_buckets b ) + + (** get the value tag, if any *) let get_value_line_tag tags = try @@ -988,4 +995,3 @@ let desc_uninitialized_dangling_pointer_deref deref expr_str loc = (at_line tags loc) in {no_desc with descriptions= [description]; tags= !tags} - diff --git a/infer/src/IR/Localise.mli b/infer/src/IR/Localise.mli index 079068ebd..341a7a18c 100644 --- a/infer/src/IR/Localise.mli +++ b/infer/src/IR/Localise.mli @@ -74,6 +74,9 @@ val error_desc_get_bucket : error_desc -> string option val error_desc_set_bucket : error_desc -> string -> error_desc (** set the bucket value of an error_desc *) +val error_desc_is_reportable_bucket : error_desc -> bool +(** check if the report is in a high confidence bucket *) + val error_desc_hash : error_desc -> int (** hash function for error_desc *) diff --git a/infer/src/backend/InferPrint.ml b/infer/src/backend/InferPrint.ml index 85beea114..d63eaafb0 100644 --- a/infer/src/backend/InferPrint.ml +++ b/infer/src/backend/InferPrint.ml @@ -199,14 +199,7 @@ let should_report (issue_kind: Exceptions.err_kind) issue_type error_desc eclass in List.mem ~equal:IssueType.equal null_deref_issue_types issue_type in - if issue_type_is_null_deref then - let issue_bucket_is_high = - let issue_bucket = Localise.error_desc_get_bucket error_desc in - let high_buckets = Localise.BucketLevel.([b1; b2]) in - Option.value_map issue_bucket ~default:false ~f:(fun b -> - List.mem ~equal:String.equal high_buckets b ) - in - issue_bucket_is_high + if issue_type_is_null_deref then Localise.error_desc_is_reportable_bucket error_desc else true diff --git a/infer/src/checkers/NullabilityCheck.ml b/infer/src/checkers/NullabilityCheck.ml index c123c045e..c394c5caa 100644 --- a/infer/src/checkers/NullabilityCheck.ml +++ b/infer/src/checkers/NullabilityCheck.ml @@ -60,78 +60,93 @@ module TransferFunctions (CFG : ProcCfg.S) = struct method_prefixes + let is_conflicting_report summary report_location = + if not Config.filtering then false + else + Errlog.fold + (fun {Errlog.err_name; err_desc; in_footprint} {Errlog.loc} found_confict -> + found_confict + || in_footprint && IssueType.equal err_name IssueType.null_dereference + && Location.equal loc report_location + && Localise.error_desc_is_reportable_bucket err_desc) + (Specs.get_err_log summary) false + + let report_nullable_dereference ap call_sites {ProcData.pdesc; extras} loc = - let pname = Procdesc.get_proc_name pdesc in - let annotation = Localise.nullable_annotation_name pname in - let call_site = - try CallSites.min_elt call_sites with Not_found -> - L.(die InternalError) - "Expecting a least one element in the set of call sites when analyzing %a" - Typ.Procname.pp pname - in - let simplified_pname = - Typ.Procname.to_simplified_string ~withclass:true (CallSite.pname call_site) - in - let is_direct_dereference = - match ap with - | (Var.LogicalVar _, _), _ -> - true - | (Var.ProgramVar pvar, _), _ -> - Pvar.is_frontend_tmp pvar - in - let message = - if is_direct_dereference then - (* direct dereference without intermediate variable *) - F.asprintf - "The return value of %s is annotated with %a and is dereferenced without being checked for null at %a" - (MF.monospaced_to_string simplified_pname) - MF.pp_monospaced annotation Location.pp loc - else - (* dereference with intermediate variable *) - F.asprintf - "Variable %a is indirectly annotated with %a (source %a) and is dereferenced without being checked for null at %a" - (MF.wrap_monospaced AccessPath.pp) - ap MF.pp_monospaced annotation (MF.wrap_monospaced CallSite.pp) call_site Location.pp loc - in - let exn = - Exceptions.Checkers (IssueType.nullable_dereference, Localise.verbatim_desc message) - in let summary = extras in - let trace = - let with_origin_site = - let callee_pname = CallSite.pname call_site in - match Specs.proc_resolve_attributes callee_pname with - | None -> - [] - | Some attributes -> - let description = - F.asprintf "definition of %s" (Typ.Procname.get_method callee_pname) - in - let trace_element = - Errlog.make_trace_element 1 attributes.ProcAttributes.loc description [] - in - [trace_element] + if is_conflicting_report summary loc then () + else + let pname = Procdesc.get_proc_name pdesc in + let annotation = Localise.nullable_annotation_name pname in + let call_site = + try CallSites.min_elt call_sites with Not_found -> + L.(die InternalError) + "Expecting a least one element in the set of call sites when analyzing %a" + Typ.Procname.pp pname + in + let simplified_pname = + Typ.Procname.to_simplified_string ~withclass:true (CallSite.pname call_site) + in + let is_direct_dereference = + match ap with + | (Var.LogicalVar _, _), _ -> + true + | (Var.ProgramVar pvar, _), _ -> + Pvar.is_frontend_tmp pvar in - let with_assignment_site = - let call_site_loc = CallSite.loc call_site in - if Location.equal call_site_loc loc then with_origin_site + let message = + if is_direct_dereference then + (* direct dereference without intermediate variable *) + F.asprintf + "The return value of %s is annotated with %a and is dereferenced without being checked for null at %a" + (MF.monospaced_to_string simplified_pname) + MF.pp_monospaced annotation Location.pp loc else - let trace_element = - Errlog.make_trace_element 0 call_site_loc "assignment of the nullable value" [] - in - trace_element :: with_origin_site + (* dereference with intermediate variable *) + F.asprintf + "Variable %a is indirectly annotated with %a (source %a) and is dereferenced without being checked for null at %a" + (MF.wrap_monospaced AccessPath.pp) + ap MF.pp_monospaced annotation (MF.wrap_monospaced CallSite.pp) call_site Location.pp + loc + in + let exn = + Exceptions.Checkers (IssueType.nullable_dereference, Localise.verbatim_desc message) in - let dereference_site = - let description = - if is_direct_dereference then - F.asprintf "dereferencing the return of %s" simplified_pname - else F.asprintf "dereference of %a" AccessPath.pp ap + let trace = + let with_origin_site = + let callee_pname = CallSite.pname call_site in + match Specs.proc_resolve_attributes callee_pname with + | None -> + [] + | Some attributes -> + let description = + F.asprintf "definition of %s" (Typ.Procname.get_method callee_pname) + in + let trace_element = + Errlog.make_trace_element 1 attributes.ProcAttributes.loc description [] + in + [trace_element] in - Errlog.make_trace_element 0 loc description [] + let with_assignment_site = + let call_site_loc = CallSite.loc call_site in + if Location.equal call_site_loc loc then with_origin_site + else + let trace_element = + Errlog.make_trace_element 0 call_site_loc "assignment of the nullable value" [] + in + trace_element :: with_origin_site + in + let dereference_site = + let description = + if is_direct_dereference then + F.asprintf "dereferencing the return of %s" simplified_pname + else F.asprintf "dereference of %a" AccessPath.pp ap + in + Errlog.make_trace_element 0 loc description [] + in + dereference_site :: with_assignment_site in - dereference_site :: with_assignment_site - in - Reporting.log_error summary ~loc ~ltr:trace exn + Reporting.log_error summary ~loc ~ltr:trace exn let add_nullable_ap ap call_sites (aps, pnames) = (NullableAP.add ap call_sites aps, pnames) diff --git a/infer/src/checkers/registerCheckers.ml b/infer/src/checkers/registerCheckers.ml index 5f28341d8..4bef55f14 100644 --- a/infer/src/checkers/registerCheckers.ml +++ b/infer/src/checkers/registerCheckers.ml @@ -27,9 +27,16 @@ type callback = callback_fun * Config.language type checker = {name: string; active: bool; callbacks: callback list} let all_checkers = + (* TODO (T24393492): the checkers should run in the order from this list. + Currently, the checkers are run in the reverse order *) [ { name= "annotation reachability" ; active= Config.annotation_reachability ; callbacks= [(Procedure AnnotationReachability.checker, Config.Java)] } + ; { name= "nullable checks" + ; active= Config.check_nullable + ; callbacks= + [ (Procedure NullabilityCheck.checker, Config.Clang) + ; (Procedure NullabilityCheck.checker, Config.Java) ] } ; { name= "biabduction" ; active= Config.biabduction ; callbacks= @@ -59,11 +66,6 @@ let all_checkers = ; { name= "printf args" ; active= Config.printf_args ; callbacks= [(Procedure PrintfArgs.callback_printf_args, Config.Java)] } - ; { name= "nullable checks" - ; active= Config.check_nullable - ; callbacks= - [ (Procedure NullabilityCheck.checker, Config.Clang) - ; (Procedure NullabilityCheck.checker, Config.Java) ] } ; { name= "nullable suggestion" ; active= Config.suggest_nullable ; callbacks= diff --git a/infer/tests/codetoanalyze/cpp/conflicts/Makefile b/infer/tests/codetoanalyze/cpp/conflicts/Makefile new file mode 100644 index 000000000..508cda22a --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/conflicts/Makefile @@ -0,0 +1,20 @@ +# Copyright (c) 2017 - 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. + + +TESTS_DIR = ../../.. + +ANALYZER = checkers +CLANG_OPTIONS = -c -x c++ -std=c++14 +INFER_OPTIONS = --no-default-checkers --biabduction --check-nullable --project-root $(TESTS_DIR) +INFERPRINT_OPTIONS = --issues-tests + +SOURCES = $(wildcard *.cpp) + +include $(TESTS_DIR)/clang.make + +infer-out/report.json: $(MAKEFILE_LIST) diff --git a/infer/tests/codetoanalyze/cpp/conflicts/issues.exp b/infer/tests/codetoanalyze/cpp/conflicts/issues.exp new file mode 100644 index 000000000..5e0dfffe4 --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/conflicts/issues.exp @@ -0,0 +1,3 @@ +codetoanalyze/cpp/conflicts/test.cpp, test1_bad, 2, NULL_DEREFERENCE, [start of procedure test1_bad(),Skipping nullableMethod(): function or method not found] +codetoanalyze/cpp/conflicts/test.cpp, test2_bad, 1, NULLABLE_DEREFERENCE, [dereferencing the return of nullableMethod(),definition of nullableMethod] +codetoanalyze/cpp/conflicts/test.cpp, test3_bad, 5, NULLABLE_DEREFERENCE, [dereference of &p,assignment of the nullable value,definition of nullableMethod] diff --git a/infer/tests/codetoanalyze/cpp/conflicts/test.cpp b/infer/tests/codetoanalyze/cpp/conflicts/test.cpp new file mode 100644 index 000000000..810d4caa8 --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/conflicts/test.cpp @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2017 - 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. + */ +class C { + private: + int f; + + public: + int get() { return f; } +}; + +C* _Nullable nullableMethod(); + +int test1_bad() { + C* p = nullableMethod(); + return p->get(); // reported by the biabduction analysis +} + +int test2_bad() { + return nullableMethod()->get(); // not reported by the biabduction analysis +} + +int test3_bad() { + C* p = nullableMethod(); + for (int i = 0; i < 10; i++) { + p = nullableMethod(); + } + return p->get(); // not reported by the biabduction analysis +}