[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

@@ -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 \

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. *)
+         variables cannot be raced on as every thread has its own stack.
-      let own_locals_in_cpp =
+          More generally, we will never be confident that a race exists on a local/temp. *)
-        match Procdesc.get_proc_name proc_desc with
+      let own_locals =
-        | ObjC_Cpp _ | C _ ->
+        List.fold ~f:add_owned_local (Procdesc.get_locals proc_desc) ~init:OwnershipDomain.empty
-            List.fold ~f:add_owned_local (Procdesc.get_locals proc_desc)
-              ~init:OwnershipDomain.empty
-        | _ ->
-            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
+       It is difficult to ensure that a report on an access path starting with a logical
-       variable or a temporary variable. We want to skip the reports for now until we
+       variable or a temporary variable, is a race. We want to skip the reports, at least for now.*)
-       find a solution *)
     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

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

infer/tests/build_systems/racerd_dedup/issues.exp

@@ -0,0 +1,8 @@
+DeDup.java, void DeDup.colocated_read_write(), 66, THREAD_SAFETY_VIOLATION, ERROR, [<Read trace>,call to void DeDup.read_and_write(),access to `this.build_systems.threadsafety.DeDup.colocated_read`,<Write trace>,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, [<Read trace>,access to `this.build_systems.threadsafety.DeDup.field`,<Write trace>,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, [<Read trace>,access to `this.build_systems.threadsafety.DeDup.field`,<Write trace>,access to `this.build_systems.threadsafety.DeDup.field`]

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;
+  }
+}

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
 

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'

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',
     ]
 )

infer/tests/build_systems/threadsafety_dedup/issues.exp

@@ -1,8 +0,0 @@
-DeDup.java, void DeDup.colocated_read_write(), 1, THREAD_SAFETY_VIOLATION, [<Beginning of read trace>,call to void DeDup.read_and_write(),access to `build_systems.threadsafety.DeDup.colocated_read`,<Beginning of write trace>,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, [<Beginning of read trace>,access to `build_systems.threadsafety.DeDup.field`,<Beginning of write trace>,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, [<Beginning of read trace>,access to `build_systems.threadsafety.DeDup.field`,<Beginning of write trace>,access to `build_systems.threadsafety.DeDup.field`]