From e212183e7e2ced31c70930fe9cfb62bedeff7dc1 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Tue, 6 Dec 2016 14:05:24 -0800 Subject: [PATCH] [thread-safety] don't count accesses in methods of builder classes Summary: Although the Builder pattern is not actually thread-safe, Builder's are not expected to be shared between threads. Handle this by ignoring all unprotected accesses in classes the end with "Builder". We might be able to soften this heuristic in the future by ensuring rather than assuming that Builder are not shared between methods (or, ideally, between threads). Reviewed By: peterogithub Differential Revision: D4280761 fbshipit-source-id: a4e6738 --- infer/src/checkers/ThreadSafety.ml | 15 ++- .../java/threadsafety/Builders.java | 95 +++++++++++++++++++ .../java/threadsafety/issues.exp | 3 + 3 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 infer/tests/codetoanalyze/java/threadsafety/Builders.java 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]