diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index 2571b4237..51ba8996e 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -23,6 +23,17 @@ module Summary = Summary.Make (struct payload.Specs.threadsafety end) +(* we want to consider Builder classes and other safe immutablility-ensuring patterns as + thread-safe. we are overly friendly about this for now; any class whose name ends with `Builder` + is assumed to be thread-safe. in the future, we can ask for builder classes to be annotated with + @Builder and verify that annotated classes satisfy the expected invariants. *) +let is_builder_class class_name = + string_is_suffix "Builder" class_name + +let is_call_to_builder_class_method = function + | Procname.Java java_pname -> is_builder_class (Procname.java_get_class_name java_pname) + | _ -> false + let is_initializer tenv proc_name = Procname.is_constructor proc_name || Procname.is_class_initializer proc_name || @@ -92,7 +103,9 @@ module TransferFunctions (CFG : ProcCfg.S) = struct let _, read_writestate' = (* TODO (14842325): report on constructors that aren't threadsafe (e.g., constructors that access static fields) *) - if is_unprotected lockstate' && not (is_initializer tenv pn) + if is_unprotected lockstate' && + not (is_initializer tenv pn) && + not (is_call_to_builder_class_method pn) then let call_site = CallSite.make pn loc in let callee_readstate = diff --git a/infer/tests/codetoanalyze/java/threadsafety/Builders.java b/infer/tests/codetoanalyze/java/threadsafety/Builders.java new file mode 100644 index 000000000..525b74347 --- /dev/null +++ b/infer/tests/codetoanalyze/java/threadsafety/Builders.java @@ -0,0 +1,95 @@ +/* + * 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 com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableList.Builder; + +@ThreadSafe +public class Builders { + + static class Obj { + final String f; + String g; + + public Obj(String f, String g) { + this.f = f; + this.g = g; + } + + public static class Builder { + String f; + String g; + + public Builder setFromObj(Obj input) { + this.f = input.f; + this.g = input.g; + return this; + } + + public Obj build() { + return new Obj(f, g); + } + + public Builder setF(String f) { + this.f = f; + return this; + } + + public Builder setG(String g) { + this.g = g; + return this; + } + } + } + + public void guavaBuilderOk(ImmutableList.Builder builder) { + builder.add("foo"); + builder.build(); + } + + public Obj customBuilderOk1(Obj.Builder builder) { + builder.setF("f"); + builder.setG("g"); + return builder.build(); + } + + public Obj customBuilderOk2(Obj.Builder builder) { + return builder.setF("f").setG("g").build(); + } + + public Obj customBuilderOk3(Obj input) { + Obj.Builder builder = new Obj.Builder(); + return builder.setFromObj(input).build(); + } + + public Obj mutateBad(Obj o) { + o.g = ""; + return o; + } + + public Obj buildThenMutateBad(Obj input) { + Obj.Builder builder = new Obj.Builder(); + Obj output = builder.setFromObj(input).build(); + input.g = ""; + return output; + } + +} + +@ThreadSafe +class TopLevelBuilder { + public String g; + + public void setG(String g) { + this.g = g; // still want to warn if the builder is annotated ThreadSafe + } + +} diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index b74e50a74..a86791499 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -1,3 +1,6 @@ +codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.buildThenMutateBad(Builders$Obj), 3, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Builders$Obj.g] +codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.mutateBad(Builders$Obj), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Builders$Obj.g] +codetoanalyze/java/threadsafety/Builders.java, void TopLevelBuilder.setG(String), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.TopLevelBuilder.g] codetoanalyze/java/threadsafety/Locks.java, void Locks.FP_unlockOneLock(), 4, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Locks.f] codetoanalyze/java/threadsafety/Locks.java, void Locks.afterReentrantLockUnlockBad(), 3, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Locks.f] codetoanalyze/java/threadsafety/Locks.java, void Locks.afterUnlockBad(), 3, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Locks.f]