From 9cddbd7af80c223799e263d58c3bcc6268b1571e Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Wed, 16 May 2018 05:14:28 -0700 Subject: [PATCH] [racerd] make locals in Java owned, plus stop reporting on temps/logical vars Summary: Follow C++ in having local variables owned plus silence reports on paths rooted on logical vars. We need both because when propagating ownership from right to left, the initial status of a temp var as owned is lost. Reviewed By: sblackshear Differential Revision: D7988575 fbshipit-source-id: 2e817d7 --- Makefile | 1 + infer/src/concurrency/RacerD.ml | 27 ++++++++----------- .../Makefile | 2 +- .../build_systems/racerd_dedup/issues.exp | 8 ++++++ .../src/DeDup.java | 0 .../racerd_dedup/src/Locals.java | 23 ++++++++++++++++ .../Makefile | 2 +- .../issues.exp | 0 .../module1/BUCK | 0 .../module1/Class1.java | 0 .../module2/BUCK | 2 +- .../module2/Class2.java | 0 .../module3/BUCK | 2 +- .../module3/Class3.java | 0 .../threadsafety_dedup/issues.exp | 8 ------ 15 files changed, 47 insertions(+), 28 deletions(-) rename infer/tests/build_systems/{threadsafety_dedup => racerd_dedup}/Makefile (95%) create mode 100644 infer/tests/build_systems/racerd_dedup/issues.exp rename infer/tests/build_systems/{threadsafety_dedup => racerd_dedup}/src/DeDup.java (100%) create mode 100644 infer/tests/build_systems/racerd_dedup/src/Locals.java rename infer/tests/build_systems/{threadsafety_traces => racerd_traces}/Makefile (92%) rename infer/tests/build_systems/{threadsafety_traces => racerd_traces}/issues.exp (100%) rename infer/tests/build_systems/{threadsafety_traces => racerd_traces}/module1/BUCK (100%) rename infer/tests/build_systems/{threadsafety_traces => racerd_traces}/module1/Class1.java (100%) rename infer/tests/build_systems/{threadsafety_traces => racerd_traces}/module2/BUCK (62%) rename infer/tests/build_systems/{threadsafety_traces => racerd_traces}/module2/Class2.java (100%) rename infer/tests/build_systems/{threadsafety_traces => racerd_traces}/module3/BUCK (51%) rename infer/tests/build_systems/{threadsafety_traces => racerd_traces}/module3/Class3.java (100%) delete mode 100644 infer/tests/build_systems/threadsafety_dedup/issues.exp diff --git a/Makefile b/Makefile index b9d0edcdb..9ad99ed3a 100644 --- a/Makefile +++ b/Makefile @@ -85,6 +85,7 @@ BUILD_SYSTEMS_TESTS += \ gradle \ javac \ resource_leak_exception_lines \ + racerd_dedup DIRECT_TESTS += \ java_checkers java_eradicate java_infer java_lab java_tracing java_quandary \ diff --git a/infer/src/concurrency/RacerD.ml b/infer/src/concurrency/RacerD.ml index a021121b4..b5f45d730 100644 --- a/infer/src/concurrency/RacerD.ml +++ b/infer/src/concurrency/RacerD.ml @@ -701,14 +701,10 @@ let analyze_procedure {Callbacks.proc_desc; get_proc_desc; tenv; summary} = OwnershipDomain.add (base, []) OwnershipAbstractValue.owned acc in (* Add ownership to local variables. In cpp, stack-allocated local - variables cannot be raced on as every thread has its own stack. *) - let own_locals_in_cpp = - match Procdesc.get_proc_name proc_desc with - | ObjC_Cpp _ | C _ -> - List.fold ~f:add_owned_local (Procdesc.get_locals proc_desc) - ~init:OwnershipDomain.empty - | _ -> - OwnershipDomain.empty + variables cannot be raced on as every thread has its own stack. + More generally, we will never be confident that a race exists on a local/temp. *) + let own_locals = + List.fold ~f:add_owned_local (Procdesc.get_locals proc_desc) ~init:OwnershipDomain.empty in let is_owned_formal {Annot.class_name} = (* @InjectProp allocates a fresh object to bind to the parameter *) @@ -736,10 +732,10 @@ let analyze_procedure {Callbacks.proc_desc; get_proc_desc; tenv; summary} = called via DI or using fresh parameters. *) if Annotations.pdesc_has_return_annot proc_desc Annotations.ia_is_inject then List.mapi ~f:(fun i _ -> i) (Procdesc.get_formals proc_desc) - |> List.fold ~f:add_owned_formal ~init:own_locals_in_cpp + |> List.fold ~f:add_owned_formal ~init:own_locals else (* express that the constructor owns [this] *) - let init = add_owned_formal own_locals_in_cpp 0 in + let init = add_owned_formal own_locals 0 in List.fold ~f:add_conditional_owned_formal ~init (List.filter ~f:(fun (_, index) -> not (Int.equal index 0)) @@ -752,7 +748,7 @@ let analyze_procedure {Callbacks.proc_desc; get_proc_desc; tenv; summary} = let ownership = List.fold ~f:add_conditional_owned_formal (FormalMap.get_formals_indexes formal_map) - ~init:own_locals_in_cpp + ~init:own_locals in {RacerDDomain.empty with ownership; threads} in @@ -994,17 +990,16 @@ let report_thread_safety_violation tenv pdesc ~make_description ~report_kind acc let is_full_trace = TraceElem.is_direct final_sink in let is_pvar_base initial_sink = let access_path = Access.get_access_path (PathDomain.Sink.kind initial_sink) in - Option.value_map ~default:false access_path ~f:(fun ((var, _), _) -> + Option.value_map ~default:true access_path ~f:(fun ((var, _), _) -> Var.appears_in_source_code var ) 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. - For C++ it is difficult to understand error messages when access path starts with a logical - variable or a temporary variable. We want to skip the reports for now until we - find a solution *) + It is difficult to ensure that a report on an access path starting with a logical + variable or a temporary variable, is a race. We want to skip the reports, at least for now.*) if not Config.filtering - || if Typ.Procname.is_java pname then is_full_trace else is_pvar_base initial_sink + || (is_pvar_base initial_sink && (not (Typ.Procname.is_java pname) || is_full_trace)) then let final_sink_site = PathDomain.Sink.call_site final_sink in let initial_sink_site = PathDomain.Sink.call_site initial_sink in diff --git a/infer/tests/build_systems/threadsafety_dedup/Makefile b/infer/tests/build_systems/racerd_dedup/Makefile similarity index 95% rename from infer/tests/build_systems/threadsafety_dedup/Makefile rename to infer/tests/build_systems/racerd_dedup/Makefile index f717d2bcd..06a0b0cab 100644 --- a/infer/tests/build_systems/threadsafety_dedup/Makefile +++ b/infer/tests/build_systems/racerd_dedup/Makefile @@ -12,6 +12,6 @@ ANALYZER = checkers INFER_OPTIONS = --project-root src --no-keep-going --racerd-only --filtering INFERPRINT_OPTIONS = --issues-tests -SOURCES = src/DeDup.java +SOURCES = src/*.java include $(TESTS_DIR)/javac.make diff --git a/infer/tests/build_systems/racerd_dedup/issues.exp b/infer/tests/build_systems/racerd_dedup/issues.exp new file mode 100644 index 000000000..5e93cc826 --- /dev/null +++ b/infer/tests/build_systems/racerd_dedup/issues.exp @@ -0,0 +1,8 @@ +DeDup.java, void DeDup.colocated_read_write(), 66, THREAD_SAFETY_VIOLATION, ERROR, [,call to void DeDup.read_and_write(),access to `this.build_systems.threadsafety.DeDup.colocated_read`,,access to `this.build_systems.threadsafety.DeDup.colocated_read`] +DeDup.java, void DeDup.separate_write_to_colocated_read(), 71, THREAD_SAFETY_VIOLATION, ERROR, [access to `this.build_systems.threadsafety.DeDup.colocated_read`] +DeDup.java, void DeDup.twoWritesOneInCaller(), 54, THREAD_SAFETY_VIOLATION, ERROR, [access to `this.build_systems.threadsafety.DeDup.field`] +DeDup.java, void DeDup.two_fields(), 22, THREAD_SAFETY_VIOLATION, ERROR, [call to void DeDup.foo(),access to `this.build_systems.threadsafety.DeDup.fielda`] +DeDup.java, void DeDup.two_reads(), 41, THREAD_SAFETY_VIOLATION, ERROR, [,access to `this.build_systems.threadsafety.DeDup.field`,,access to `this.build_systems.threadsafety.DeDup.field`] +DeDup.java, void DeDup.two_writes(), 34, THREAD_SAFETY_VIOLATION, ERROR, [access to `this.build_systems.threadsafety.DeDup.field`] +DeDup.java, void DeDup.write_read(), 47, THREAD_SAFETY_VIOLATION, ERROR, [access to `this.build_systems.threadsafety.DeDup.field`] +DeDup.java, void DeDup.write_read(), 48, THREAD_SAFETY_VIOLATION, ERROR, [,access to `this.build_systems.threadsafety.DeDup.field`,,access to `this.build_systems.threadsafety.DeDup.field`] diff --git a/infer/tests/build_systems/threadsafety_dedup/src/DeDup.java b/infer/tests/build_systems/racerd_dedup/src/DeDup.java similarity index 100% rename from infer/tests/build_systems/threadsafety_dedup/src/DeDup.java rename to infer/tests/build_systems/racerd_dedup/src/DeDup.java diff --git a/infer/tests/build_systems/racerd_dedup/src/Locals.java b/infer/tests/build_systems/racerd_dedup/src/Locals.java new file mode 100644 index 000000000..ef712ef9f --- /dev/null +++ b/infer/tests/build_systems/racerd_dedup/src/Locals.java @@ -0,0 +1,23 @@ +/* + * Copyright (c) 2016 - 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 codetoanalyze.java.checkers; + +import javax.annotation.concurrent.ThreadSafe; + +@ThreadSafe +public class Locals { + int f; + + static Locals id(Locals o) { return o; } + + static void FN_raceOnTemporary_bad(Locals o) { + id(o).f = 5; + } +} diff --git a/infer/tests/build_systems/threadsafety_traces/Makefile b/infer/tests/build_systems/racerd_traces/Makefile similarity index 92% rename from infer/tests/build_systems/threadsafety_traces/Makefile rename to infer/tests/build_systems/racerd_traces/Makefile index 5c84788cb..38400f8f4 100644 --- a/infer/tests/build_systems/threadsafety_traces/Makefile +++ b/infer/tests/build_systems/racerd_traces/Makefile @@ -9,7 +9,7 @@ TESTS_DIR = ../.. ROOT_DIR = $(TESTS_DIR)/../.. ANALYZER = checkers -BUCK_TARGET = //infer/tests/build_systems/threadsafety_traces/module3:module3 +BUCK_TARGET = //infer/tests/build_systems/racerd_traces/module3:module3 INFERPRINT_OPTIONS = --project-root $(ROOT_DIR) --issues-tests CLEAN_EXTRA = $(ROOT_DIR)/buck-out diff --git a/infer/tests/build_systems/threadsafety_traces/issues.exp b/infer/tests/build_systems/racerd_traces/issues.exp similarity index 100% rename from infer/tests/build_systems/threadsafety_traces/issues.exp rename to infer/tests/build_systems/racerd_traces/issues.exp diff --git a/infer/tests/build_systems/threadsafety_traces/module1/BUCK b/infer/tests/build_systems/racerd_traces/module1/BUCK similarity index 100% rename from infer/tests/build_systems/threadsafety_traces/module1/BUCK rename to infer/tests/build_systems/racerd_traces/module1/BUCK diff --git a/infer/tests/build_systems/threadsafety_traces/module1/Class1.java b/infer/tests/build_systems/racerd_traces/module1/Class1.java similarity index 100% rename from infer/tests/build_systems/threadsafety_traces/module1/Class1.java rename to infer/tests/build_systems/racerd_traces/module1/Class1.java diff --git a/infer/tests/build_systems/threadsafety_traces/module2/BUCK b/infer/tests/build_systems/racerd_traces/module2/BUCK similarity index 62% rename from infer/tests/build_systems/threadsafety_traces/module2/BUCK rename to infer/tests/build_systems/racerd_traces/module2/BUCK index 9b6224178..b4df50c6a 100644 --- a/infer/tests/build_systems/threadsafety_traces/module2/BUCK +++ b/infer/tests/build_systems/racerd_traces/module2/BUCK @@ -2,7 +2,7 @@ java_library( name='module2', srcs=['Class2.java'], deps=[ - '//infer/tests/build_systems/threadsafety_traces/module1:module1' + '//infer/tests/build_systems/racerd_traces/module1:module1' ], visibility=[ 'PUBLIC' diff --git a/infer/tests/build_systems/threadsafety_traces/module2/Class2.java b/infer/tests/build_systems/racerd_traces/module2/Class2.java similarity index 100% rename from infer/tests/build_systems/threadsafety_traces/module2/Class2.java rename to infer/tests/build_systems/racerd_traces/module2/Class2.java diff --git a/infer/tests/build_systems/threadsafety_traces/module3/BUCK b/infer/tests/build_systems/racerd_traces/module3/BUCK similarity index 51% rename from infer/tests/build_systems/threadsafety_traces/module3/BUCK rename to infer/tests/build_systems/racerd_traces/module3/BUCK index 23641400f..946b4e5ac 100644 --- a/infer/tests/build_systems/threadsafety_traces/module3/BUCK +++ b/infer/tests/build_systems/racerd_traces/module3/BUCK @@ -2,6 +2,6 @@ java_library( name='module3', srcs=['Class3.java'], deps=[ - '//infer/tests/build_systems/threadsafety_traces/module2:module2', + '//infer/tests/build_systems/racerd_traces/module2:module2', ] ) diff --git a/infer/tests/build_systems/threadsafety_traces/module3/Class3.java b/infer/tests/build_systems/racerd_traces/module3/Class3.java similarity index 100% rename from infer/tests/build_systems/threadsafety_traces/module3/Class3.java rename to infer/tests/build_systems/racerd_traces/module3/Class3.java diff --git a/infer/tests/build_systems/threadsafety_dedup/issues.exp b/infer/tests/build_systems/threadsafety_dedup/issues.exp deleted file mode 100644 index 870d53de5..000000000 --- a/infer/tests/build_systems/threadsafety_dedup/issues.exp +++ /dev/null @@ -1,8 +0,0 @@ -DeDup.java, void DeDup.colocated_read_write(), 1, THREAD_SAFETY_VIOLATION, [,call to void DeDup.read_and_write(),access to `build_systems.threadsafety.DeDup.colocated_read`,,access to `build_systems.threadsafety.DeDup.colocated_read`] -DeDup.java, void DeDup.separate_write_to_colocated_read(), 1, THREAD_SAFETY_VIOLATION, [access to `build_systems.threadsafety.DeDup.colocated_read`] -DeDup.java, void DeDup.twoWritesOneInCaller(), 2, THREAD_SAFETY_VIOLATION, [access to `build_systems.threadsafety.DeDup.field`] -DeDup.java, void DeDup.two_fields(), 1, THREAD_SAFETY_VIOLATION, [call to void DeDup.foo(),access to `build_systems.threadsafety.DeDup.fielda`] -DeDup.java, void DeDup.two_reads(), 3, THREAD_SAFETY_VIOLATION, [,access to `build_systems.threadsafety.DeDup.field`,,access to `build_systems.threadsafety.DeDup.field`] -DeDup.java, void DeDup.two_writes(), 2, THREAD_SAFETY_VIOLATION, [access to `build_systems.threadsafety.DeDup.field`] -DeDup.java, void DeDup.write_read(), 2, THREAD_SAFETY_VIOLATION, [access to `build_systems.threadsafety.DeDup.field`] -DeDup.java, void DeDup.write_read(), 3, THREAD_SAFETY_VIOLATION, [,access to `build_systems.threadsafety.DeDup.field`,,access to `build_systems.threadsafety.DeDup.field`]