From 47ab1a2e67d7c8e8f5444bc171fdc8fb39a2eadc Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Thu, 26 Oct 2017 12:49:07 -0700 Subject: [PATCH] [thread-safety] skip reporting on truncated traces Summary: Due to limitations in our Buck integration, the thread-safety analysis cannot create a trace that bottoms out in a Buck target that is not a direct dependency of the current target. These truncated traces are confusing and tough to act on. Until we can address these limitations, let's avoid reporting on truncated traces. Reviewed By: jeremydubreil Differential Revision: D5969840 fbshipit-source-id: 877b9de --- infer/src/concurrency/RacerD.ml | 40 +++++++++++-------- infer/src/concurrency/RacerDDomain.ml | 36 ++++++++++------- infer/src/concurrency/RacerDDomain.mli | 17 ++++---- .../threadsafety_traces/Makefile | 30 ++++++++++++++ .../threadsafety_traces/issues.exp | 0 .../threadsafety_traces/module1/BUCK | 8 ++++ .../threadsafety_traces/module1/Class1.java | 20 ++++++++++ .../threadsafety_traces/module2/BUCK | 10 +++++ .../threadsafety_traces/module2/Class2.java | 20 ++++++++++ .../threadsafety_traces/module3/BUCK | 7 ++++ .../threadsafety_traces/module3/Class3.java | 27 +++++++++++++ 11 files changed, 177 insertions(+), 38 deletions(-) create mode 100644 infer/tests/build_systems/threadsafety_traces/Makefile create mode 100644 infer/tests/build_systems/threadsafety_traces/issues.exp create mode 100644 infer/tests/build_systems/threadsafety_traces/module1/BUCK create mode 100644 infer/tests/build_systems/threadsafety_traces/module1/Class1.java create mode 100644 infer/tests/build_systems/threadsafety_traces/module2/BUCK create mode 100644 infer/tests/build_systems/threadsafety_traces/module2/Class2.java create mode 100644 infer/tests/build_systems/threadsafety_traces/module3/BUCK create mode 100644 infer/tests/build_systems/threadsafety_traces/module3/Class3.java diff --git a/infer/src/concurrency/RacerD.ml b/infer/src/concurrency/RacerD.ml index 954a1bb67..0ab37dca4 100644 --- a/infer/src/concurrency/RacerD.ml +++ b/infer/src/concurrency/RacerD.ml @@ -346,7 +346,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct then let open Domain in let pre = AccessPrecondition.make locks threads proc_data.pdesc in - AccessDomain.add_access pre (make_unannotated_call_access pname loc) attribute_map + AccessDomain.add_access pre (TraceElem.make_unannotated_call_access pname loc) attribute_map else attribute_map @@ -380,7 +380,9 @@ module TransferFunctions (CFG : ProcCfg.S) = struct || is_safe_access access prefix_path proc_data.tenv then access_acc else - AccessDomain.add_access pre (make_field_access access_path ~is_write loc) access_acc + AccessDomain.add_access pre + (TraceElem.make_field_access access_path ~is_write loc) + access_acc in add_field_accesses pre access_path access_acc' access_list' in @@ -525,7 +527,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct if is_synchronized_container callee_pname receiver_ap tenv then AccessDomain.empty else let container_access = - make_container_access receiver_ap ~is_write callee_pname callee_loc + TraceElem.make_container_access receiver_ap ~is_write callee_pname callee_loc in AccessDomain.add_access (Unprotected (IntSet.singleton 0)) container_access AccessDomain.empty @@ -1323,21 +1325,25 @@ let report_thread_safety_violation tenv pdesc issue_type ~make_description ~repo let open RacerDDomain in let pname = Procdesc.get_proc_name pdesc in let report_one_path ((_, sinks) as path) = - let initial_sink, _ = List.last_exn sinks in let final_sink, _ = List.hd_exn sinks in - let initial_sink_site = PathDomain.Sink.call_site initial_sink in - let final_sink_site = PathDomain.Sink.call_site final_sink in - let loc = CallSite.loc initial_sink_site in - let ltr = make_trace ~report_kind path pdesc in - (* what the potential bug is *) - let description = make_description pname final_sink_site initial_sink_site final_sink in - (* why we are reporting it *) - let explanation = get_reporting_explanation report_kind tenv pname thread in - let error_message = F.sprintf "%s%s" description explanation in - let exn = - Exceptions.Checkers (issue_type.IssueType.unique_id, Localise.verbatim_desc error_message) - in - Reporting.log_error_deprecated ~store_summary:true pname ~loc ~ltr exn + let is_full_trace = TraceElem.is_direct final_sink in + (* Traces can be truncated due to limitations of our Buck integration. If we have a truncated + trace, it's probably going to be too confusing to be actionable. Skip it. *) + if is_full_trace || not Config.filtering then + let final_sink_site = PathDomain.Sink.call_site final_sink in + let initial_sink, _ = List.last_exn sinks in + let initial_sink_site = PathDomain.Sink.call_site initial_sink in + let loc = CallSite.loc initial_sink_site in + let ltr = make_trace ~report_kind path pdesc in + (* what the potential bug is *) + let description = make_description pname final_sink_site initial_sink_site final_sink in + (* why we are reporting it *) + let explanation = get_reporting_explanation report_kind tenv pname thread in + let error_message = F.sprintf "%s%s" description explanation in + let exn = + Exceptions.Checkers (issue_type.IssueType.unique_id, Localise.verbatim_desc error_message) + in + Reporting.log_error_deprecated ~store_summary:true pname ~loc ~ltr exn in let trace_of_pname = trace_of_pname access pdesc in Option.iter ~f:report_one_path (PathDomain.get_reportable_sink_path access ~trace_of_pname) diff --git a/infer/src/concurrency/RacerDDomain.ml b/infer/src/concurrency/RacerDDomain.ml index acb68d887..16087e152 100644 --- a/infer/src/concurrency/RacerDDomain.ml +++ b/infer/src/concurrency/RacerDDomain.ml @@ -124,26 +124,34 @@ module TraceElem = struct let pp = pp end) -end -let make_container_access access_path pname ~is_write loc = - let site = CallSite.make Typ.Procname.empty_block loc in - let access = - if is_write then Access.ContainerWrite (access_path, pname) - else Access.ContainerRead (access_path, pname) - in - TraceElem.make access site + let dummy_pname = Typ.Procname.empty_block + + let make_dummy_site = CallSite.make dummy_pname + + (* all trace elems are created with a dummy call site. any trace elem without a dummy call site + represents a call that leads to an access rather than a direct access *) + let is_direct {site} = Typ.Procname.equal (CallSite.pname site) dummy_pname + + let make_container_access access_path pname ~is_write loc = + let site = make_dummy_site loc in + let access = + if is_write then Access.ContainerWrite (access_path, pname) + else Access.ContainerRead (access_path, pname) + in + make access site -let make_field_access access_path ~is_write loc = - let site = CallSite.make Typ.Procname.empty_block loc in - TraceElem.make (Access.make_field_access access_path ~is_write) site + let make_field_access access_path ~is_write loc = + let site = make_dummy_site loc in + make (Access.make_field_access access_path ~is_write) site -let make_unannotated_call_access pname loc = - let site = CallSite.make Typ.Procname.empty_block loc in - TraceElem.make (Access.InterfaceCall pname) site + let make_unannotated_call_access pname loc = + let site = make_dummy_site loc in + make (Access.InterfaceCall pname) site +end (* In this domain true<=false. The intended denotations [[.]] are [[true]] = the set of all states where we know according, to annotations diff --git a/infer/src/concurrency/RacerDDomain.mli b/infer/src/concurrency/RacerDDomain.mli index 1ae251428..96564683c 100644 --- a/infer/src/concurrency/RacerDDomain.mli +++ b/infer/src/concurrency/RacerDDomain.mli @@ -41,6 +41,16 @@ module TraceElem : sig val is_container_write : t -> bool val map : f:(AccessPath.t -> AccessPath.t) -> t -> t + + val is_direct : t -> bool + (** return true if the given trace elem represents a direct access, not a call that eventually + leads to an access *) + + val make_container_access : AccessPath.t -> Typ.Procname.t -> is_write:bool -> Location.t -> t + + val make_field_access : AccessPath.t -> is_write:bool -> Location.t -> t + + val make_unannotated_call_access : Typ.Procname.t -> Location.t -> t end (** A bool that is true if a lock is definitely held. Note that this is unsound because it assumes @@ -201,11 +211,4 @@ type summary = include AbstractDomain.WithBottom with type astate := astate -val make_container_access : - AccessPath.t -> Typ.Procname.t -> is_write:bool -> Location.t -> TraceElem.t - -val make_field_access : AccessPath.t -> is_write:bool -> Location.t -> TraceElem.t - -val make_unannotated_call_access : Typ.Procname.t -> Location.t -> TraceElem.t - val pp_summary : F.formatter -> summary -> unit diff --git a/infer/tests/build_systems/threadsafety_traces/Makefile b/infer/tests/build_systems/threadsafety_traces/Makefile new file mode 100644 index 000000000..1cda770ae --- /dev/null +++ b/infer/tests/build_systems/threadsafety_traces/Makefile @@ -0,0 +1,30 @@ +# 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 = ../.. +ROOT_DIR = $(TESTS_DIR)/../.. + +ANALYZER = checkers +BUCK_TARGET = //infer/tests/build_systems/threadsafety_traces/module3:module3 +INFERPRINT_OPTIONS = --project-root $(ROOT_DIR) --issues-tests +CLEAN_EXTRA = $(ROOT_DIR)/buck-out + +include $(TESTS_DIR)/infer.make + +$(OBJECTS): $(JAVA_SOURCE_FILES) + $(QUIET)cd $(ROOT_DIR) && \ + $(call silent_on_success,Compiling thread-safety trace tests,\ + INFER_BIN=$(INFER_BIN) NO_BUCKD=1 \ + $(BUCK) build --no-cache $(BUCK_TARGET)) + +infer-out/report.json: $(JAVA_DEPS) $(JAVA_SOURCE_FILES) + $(QUIET)cd $(ROOT_DIR) && \ + $(REMOVE_DIR) buck-out && \ + $(call silent_on_success,Testing thread-safety trace tests with Buck,\ + INFER_BIN=$(INFER_BIN) NO_BUCKD=1 \ + $(INFER_BIN) -a $(ANALYZER) --results-dir $(CURDIR)/infer-out -- \ + $(BUCK) build --no-cache $(BUCK_TARGET)) diff --git a/infer/tests/build_systems/threadsafety_traces/issues.exp b/infer/tests/build_systems/threadsafety_traces/issues.exp new file mode 100644 index 000000000..e69de29bb diff --git a/infer/tests/build_systems/threadsafety_traces/module1/BUCK b/infer/tests/build_systems/threadsafety_traces/module1/BUCK new file mode 100644 index 000000000..91893479d --- /dev/null +++ b/infer/tests/build_systems/threadsafety_traces/module1/BUCK @@ -0,0 +1,8 @@ +java_library( + name='module1', + srcs=['Class1.java'], + deps=[], + visibility=[ + 'PUBLIC' + ], +) diff --git a/infer/tests/build_systems/threadsafety_traces/module1/Class1.java b/infer/tests/build_systems/threadsafety_traces/module1/Class1.java new file mode 100644 index 000000000..8d1076e1a --- /dev/null +++ b/infer/tests/build_systems/threadsafety_traces/module1/Class1.java @@ -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. + */ + +package threadsafety_traces.module1; + +public class Class1 { + + static int sField; + + public static void method() { + sField++; + } + +} diff --git a/infer/tests/build_systems/threadsafety_traces/module2/BUCK b/infer/tests/build_systems/threadsafety_traces/module2/BUCK new file mode 100644 index 000000000..9b6224178 --- /dev/null +++ b/infer/tests/build_systems/threadsafety_traces/module2/BUCK @@ -0,0 +1,10 @@ +java_library( + name='module2', + srcs=['Class2.java'], + deps=[ + '//infer/tests/build_systems/threadsafety_traces/module1:module1' + ], + visibility=[ + 'PUBLIC' + ], +) diff --git a/infer/tests/build_systems/threadsafety_traces/module2/Class2.java b/infer/tests/build_systems/threadsafety_traces/module2/Class2.java new file mode 100644 index 000000000..7fedcaf5e --- /dev/null +++ b/infer/tests/build_systems/threadsafety_traces/module2/Class2.java @@ -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. + */ + +package threadsafety_traces.module2; + +import threadsafety_traces.module1.Class1; + +public class Class2 { + + public static void method() { + Class1.method(); + } + +} diff --git a/infer/tests/build_systems/threadsafety_traces/module3/BUCK b/infer/tests/build_systems/threadsafety_traces/module3/BUCK new file mode 100644 index 000000000..23641400f --- /dev/null +++ b/infer/tests/build_systems/threadsafety_traces/module3/BUCK @@ -0,0 +1,7 @@ +java_library( + name='module3', + srcs=['Class3.java'], + deps=[ + '//infer/tests/build_systems/threadsafety_traces/module2:module2', + ] +) diff --git a/infer/tests/build_systems/threadsafety_traces/module3/Class3.java b/infer/tests/build_systems/threadsafety_traces/module3/Class3.java new file mode 100644 index 000000000..84dc7f282 --- /dev/null +++ b/infer/tests/build_systems/threadsafety_traces/module3/Class3.java @@ -0,0 +1,27 @@ +/* + * 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. + */ + +package threadsafety_traces.module3; + +import threadsafety_traces.module2.Class2; + +@interface ThreadSafe { +} + +@ThreadSafe +public class Class3 { + + /** this will produce a truncated trace that should bottom out in Class1.method, but will stop + short due to limitations in our Buck integration. test that we don't report a truncated + trace in this situation. */ + public void callClass2() { + Class2.method(); + } + +}