From 14aa1edbf532675f48ba0a35a2e55d07b79b1f3c Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Fri, 27 Jul 2018 01:17:18 -0700 Subject: [PATCH] [starvation] Avoid FPs in guava Futures Summary: Guava subclasses Future in ways that make .get() calls safe from the UI thread. Treat such methods as skip. Reviewed By: jeremydubreil Differential Revision: D9013475 fbshipit-source-id: 38373aa5f --- infer/src/concurrency/RacerDConfig.ml | 9 +++++++++ infer/src/concurrency/RacerDConfig.mli | 3 +++ infer/src/concurrency/starvation.ml | 2 ++ .../codetoanalyze/java/starvation/FutureGet.java | 6 ++++++ infer/tests/codetoanalyze/java/starvation/issues.exp | 12 ++++++------ 5 files changed, 26 insertions(+), 6 deletions(-) diff --git a/infer/src/concurrency/RacerDConfig.ml b/infer/src/concurrency/RacerDConfig.ml index 368287e08..58afc4dd9 100644 --- a/infer/src/concurrency/RacerDConfig.ml +++ b/infer/src/concurrency/RacerDConfig.ml @@ -734,4 +734,13 @@ module Models = struct List.exists targets ~f:(PatternMatch.is_subtype_of_str tenv classname) | _ -> false + + + let is_futures_getdone = + is_call_of_class ["com.google.common.util.concurrent.Futures"] "getDone" |> Staged.unstage + + + let should_skip_analysis = + let matchers = [is_futures_getdone] in + fun tenv pn actuals -> List.exists matchers ~f:(fun matcher -> matcher tenv pn actuals) end diff --git a/infer/src/concurrency/RacerDConfig.mli b/infer/src/concurrency/RacerDConfig.mli index e46ca1407..527f94b96 100644 --- a/infer/src/concurrency/RacerDConfig.mli +++ b/infer/src/concurrency/RacerDConfig.mli @@ -85,4 +85,7 @@ module Models : sig val is_synchronized_library_call : Tenv.t -> Typ.Procname.t -> bool (** does the method call lock-then-unlock the underlying object? legacy Java containers like Vector do this, and can interact with explicit locking *) + + val should_skip_analysis : Tenv.t -> Typ.Procname.t -> HilExp.t list -> bool + (** should we go avoid analyzing a library method (eg in guava) to avoid FPs? *) end diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml index e3e088824..68187c4c4 100644 --- a/infer/src/concurrency/starvation.ml +++ b/infer/src/concurrency/starvation.ml @@ -96,6 +96,8 @@ module TransferFunctions (CFG : ProcCfg.S) = struct do_unlock actuals astate | LockedIfTrue -> astate + | NoEffect when Models.should_skip_analysis tenv callee actuals -> + astate | NoEffect when Models.is_synchronized_library_call tenv callee -> (* model a synchronized call without visible internal behaviour *) do_lock actuals loc astate |> do_unlock actuals diff --git a/infer/tests/codetoanalyze/java/starvation/FutureGet.java b/infer/tests/codetoanalyze/java/starvation/FutureGet.java index 21b20c177..b36823bb0 100644 --- a/infer/tests/codetoanalyze/java/starvation/FutureGet.java +++ b/infer/tests/codetoanalyze/java/starvation/FutureGet.java @@ -10,6 +10,7 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeoutException; import android.support.annotation.UiThread; import java.util.concurrent.TimeUnit; +import com.google.common.util.concurrent.Futures; class FutureGet { Future future; @@ -83,4 +84,9 @@ class FutureGet { future.get(9223372036854775807L, TimeUnit.MICROSECONDS); } catch (TimeoutException e) {} } + + @UiThread + Object getFuturesDoneOk(Future future) throws ExecutionException { + return Futures.getDone(future); + } } diff --git a/infer/tests/codetoanalyze/java/starvation/issues.exp b/infer/tests/codetoanalyze/java/starvation/issues.exp index 81fdbaac0..b534ee522 100644 --- a/infer/tests/codetoanalyze/java/starvation/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation/issues.exp @@ -12,12 +12,12 @@ codetoanalyze/java/starvation/Dedup.java, void Dedup.callMethodWithMultipleBlock codetoanalyze/java/starvation/Dedup.java, void Dedup.callMethodWithMultipleBlocksBad(), 29, STARVATION, no_bucket, ERROR, [`void Dedup.callMethodWithMultipleBlocksBad()`,calls `Object Future.get()` from `void Dedup.callMethodWithMultipleBlocksBad()`] codetoanalyze/java/starvation/Dedup.java, void Dedup.onUiThreadBad(), 21, STARVATION, no_bucket, ERROR, [`void Dedup.onUiThreadBad()`,Method call: `void Dedup.callMethodWithMultipleBlocksBad()`,calls `void CountDownLatch.await()` from `void Dedup.callMethodWithMultipleBlocksBad()`] codetoanalyze/java/starvation/Dedup.java, void Dedup.oneWayBad(), 36, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void Dedup.oneWayBad()`,locks `this.Dedup.lockA` in class `Dedup*`,locks `this.Dedup.lockB` in class `Dedup*`,[Trace 2] `void Dedup.anotherWayBad()`,locks `this.Dedup.lockB` in class `Dedup*`,locks `this.Dedup.lockA` in class `Dedup*`] -codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getDirectBad(), 20, STARVATION, no_bucket, ERROR, [`void FutureGet.getDirectBad()`,calls `Object Future.get()` from `void FutureGet.getDirectBad()`] -codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getIndirectBad(), 25, STARVATION, no_bucket, ERROR, [[Trace 1] `void FutureGet.getIndirectBad()`,locks `this.FutureGet.lock` in class `FutureGet*`,[Trace 2] `void FutureGet.getUnderLock()`,locks `this.FutureGet.lock` in class `FutureGet*`,calls `Object Future.get()` from `void FutureGet.getUnderLock()`] -codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeout50000001MicroSecondsBad(), 76, STARVATION, no_bucket, ERROR, [`void FutureGet.getTimeout50000001MicroSecondsBad()`,calls `Object Future.get(long,TimeUnit)` from `void FutureGet.getTimeout50000001MicroSecondsBad()`] -codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeout64BitsBad(), 83, STARVATION, no_bucket, ERROR, [`void FutureGet.getTimeout64BitsBad()`,calls `Object Future.get(long,TimeUnit)` from `void FutureGet.getTimeout64BitsBad()`] -codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeoutOneDayBad(), 41, STARVATION, no_bucket, ERROR, [`void FutureGet.getTimeoutOneDayBad()`,calls `Object Future.get(long,TimeUnit)` from `void FutureGet.getTimeoutOneDayBad()`] -codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeoutOneHourBad(), 55, STARVATION, no_bucket, ERROR, [`void FutureGet.getTimeoutOneHourBad()`,calls `Object Future.get(long,TimeUnit)` from `void FutureGet.getTimeoutOneHourBad()`] +codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getDirectBad(), 21, STARVATION, no_bucket, ERROR, [`void FutureGet.getDirectBad()`,calls `Object Future.get()` from `void FutureGet.getDirectBad()`] +codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getIndirectBad(), 26, STARVATION, no_bucket, ERROR, [[Trace 1] `void FutureGet.getIndirectBad()`,locks `this.FutureGet.lock` in class `FutureGet*`,[Trace 2] `void FutureGet.getUnderLock()`,locks `this.FutureGet.lock` in class `FutureGet*`,calls `Object Future.get()` from `void FutureGet.getUnderLock()`] +codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeout50000001MicroSecondsBad(), 77, STARVATION, no_bucket, ERROR, [`void FutureGet.getTimeout50000001MicroSecondsBad()`,calls `Object Future.get(long,TimeUnit)` from `void FutureGet.getTimeout50000001MicroSecondsBad()`] +codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeout64BitsBad(), 84, STARVATION, no_bucket, ERROR, [`void FutureGet.getTimeout64BitsBad()`,calls `Object Future.get(long,TimeUnit)` from `void FutureGet.getTimeout64BitsBad()`] +codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeoutOneDayBad(), 42, STARVATION, no_bucket, ERROR, [`void FutureGet.getTimeoutOneDayBad()`,calls `Object Future.get(long,TimeUnit)` from `void FutureGet.getTimeoutOneDayBad()`] +codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeoutOneHourBad(), 56, STARVATION, no_bucket, ERROR, [`void FutureGet.getTimeoutOneHourBad()`,calls `Object Future.get(long,TimeUnit)` from `void FutureGet.getTimeoutOneHourBad()`] codetoanalyze/java/starvation/IndirectBlock.java, void IndirectBlock.takeExpensiveLockOnUiThreadBad(), 22, STARVATION, no_bucket, ERROR, [[Trace 1] `void IndirectBlock.takeExpensiveLockOnUiThreadBad()`,locks `this.IndirectBlock.expensiveLock` in class `IndirectBlock*`,[Trace 2] `void IndirectBlock.doTransactUnderLock()`,locks `this.IndirectBlock.expensiveLock` in class `IndirectBlock*`,calls `boolean Binder.transact(int,Parcel,Parcel,int)` from `void IndirectBlock.doTransactUnderLock()`] codetoanalyze/java/starvation/IndirectBlock.java, void IndirectBlock.takeRemoteExpensiveLockOnUiThreadBad(IndirectInterproc), 33, STARVATION, no_bucket, ERROR, [[Trace 1] `void IndirectBlock.takeRemoteExpensiveLockOnUiThreadBad(IndirectInterproc)`,Method call: `void IndirectInterproc.takeLock()`,locks `this` in class `IndirectInterproc*`,[Trace 2] `void IndirectInterproc.doTransactUnderLock(Binder)`,locks `this` in class `IndirectInterproc*`,calls `boolean Binder.transact(int,Parcel,Parcel,int)` from `void IndirectInterproc.doTransactUnderLock(Binder)`] codetoanalyze/java/starvation/InnerClass.java, InnerClass$InnerClassA.(InnerClass,Object), 48, DEADLOCK, no_bucket, ERROR, [[Trace 1] `InnerClass$InnerClassA.(InnerClass,Object)`,locks `this` in class `InnerClass$InnerClassA*`,Method call: `void InnerClass.bar()`,locks `this` in class `InnerClass*`,[Trace 2] `void InnerClass.outerInnerBad(InnerClass$InnerClassA)`,locks `this` in class `InnerClass*`,Method call: `void InnerClass$InnerClassA.baz()`,locks `this` in class `InnerClass$InnerClassA*`]