From 4e45a62d3c9d4908d3e14fbed66825f8e24cffed Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Mon, 20 Jan 2020 10:17:30 -0800 Subject: [PATCH] [starvation] fix inner class tests Summary: Use more informative method names, and add comments explaining the logic behind each test. Correct two cases which are FPs instead of legitimate reports. Reviewed By: artempyanykh Differential Revision: D19465227 fbshipit-source-id: 29332e2b9 --- .../java/starvation/InnerClass.java | 34 ++++++++++++------- .../codetoanalyze/java/starvation/issues.exp | 8 ++--- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/infer/tests/codetoanalyze/java/starvation/InnerClass.java b/infer/tests/codetoanalyze/java/starvation/InnerClass.java index 6b8954a07..c93b3f3ac 100644 --- a/infer/tests/codetoanalyze/java/starvation/InnerClass.java +++ b/infer/tests/codetoanalyze/java/starvation/InnerClass.java @@ -6,46 +6,54 @@ */ class InnerClass { + // shouldn't be flagged + // we don't know that [a.this$0 == this] and even if it were + // this will simply lock this twice synchronized void outerInnerOk(InnerClassA a) { - a.foo(); + a.lockOuter(); } - synchronized void bar() {} + synchronized void lockOuter() {} - synchronized void outerInnerBad(InnerClassA a) { - a.baz(); + // following is flagged currently but shouldn't + // we don't known that [a.this$0 == this]! + synchronized void FP_outerInnerOk(InnerClassA a) { + a.lockInner(); } class InnerClassA { - void foo() { + void lockOuter() { synchronized (InnerClass.this) { } } void outerInnerOk() { synchronized (InnerClass.this) { - InnerClass.this.bar(); + InnerClass.this.lockOuter(); } } - synchronized void baz() {} + synchronized void lockInner() {} synchronized void innerOuterBad() { - InnerClass.this.bar(); + InnerClass.this.lockOuter(); } - // ctrs generate different access paths so test these too - // following should not be flagged + // constructors generate different access paths so test these too + // TODO these tests do not generate yet different access paths to the above :( + + // following should not be flagged -- it's a double lock on [this.this$0] InnerClassA() { synchronized (InnerClass.this) { - InnerClass.this.bar(); + InnerClass.this.lockOuter(); } } - // following should be flagged with outer_inner_bad() + // following would be flagged with outerInnerBad but should not + // because [this] is not accessible yet to any other thread! InnerClassA(Object o) { synchronized (this) { - InnerClass.this.bar(); + InnerClass.this.lockOuter(); } } } diff --git a/infer/tests/codetoanalyze/java/starvation/issues.exp b/infer/tests/codetoanalyze/java/starvation/issues.exp index 6ee93aa6b..2050c63f5 100644 --- a/infer/tests/codetoanalyze/java/starvation/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation/issues.exp @@ -13,10 +13,10 @@ codetoanalyze/java/starvation/FutureGet.java, FutureGet.getTimeoutOneDayBad():vo codetoanalyze/java/starvation/FutureGet.java, FutureGet.getTimeoutOneHourBad():void, 59, STARVATION, no_bucket, ERROR, [`void FutureGet.getTimeoutOneHourBad()`,calls `Object Future.get(long,TimeUnit)`] codetoanalyze/java/starvation/IndirectBlock.java, IndirectBlock.takeExpensiveLockOnUiThreadBad():void, 23, STARVATION, no_bucket, ERROR, [[Trace 1] `void IndirectBlock.takeExpensiveLockOnUiThreadBad()`, locks `this.expensiveLock` in `class IndirectBlock`,[Trace 2] `void IndirectBlock.doTransactUnderLock()`, locks `this.expensiveLock` in `class IndirectBlock`,calls `boolean Binder.transact(int,Parcel,Parcel,int)`] codetoanalyze/java/starvation/IndirectBlock.java, IndirectBlock.takeRemoteExpensiveLockOnUiThreadBad(IndirectInterproc):void, 35, 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)`] -codetoanalyze/java/starvation/InnerClass.java, InnerClass$InnerClassA.(InnerClass,java.lang.Object), 47, 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`] -codetoanalyze/java/starvation/InnerClass.java, InnerClass$InnerClassA.innerOuterBad():void, 34, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void InnerClass$InnerClassA.innerOuterBad()`, 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`] -codetoanalyze/java/starvation/InnerClass.java, InnerClass.outerInnerBad(InnerClass$InnerClassA):void, 16, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void InnerClass.outerInnerBad(InnerClass$InnerClassA)`, locks `this` in `class InnerClass`,Method call: `void InnerClass$InnerClassA.baz()`, locks `this` in `class InnerClass$InnerClassA`,[Trace 2] `InnerClass$InnerClassA.(InnerClass,Object)`, locks `this` in `class InnerClass$InnerClassA`,Method call: `void InnerClass.bar()`, locks `this` in `class InnerClass`] -codetoanalyze/java/starvation/InnerClass.java, InnerClass.outerInnerBad(InnerClass$InnerClassA):void, 16, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void InnerClass.outerInnerBad(InnerClass$InnerClassA)`, locks `this` in `class InnerClass`,Method call: `void InnerClass$InnerClassA.baz()`, locks `this` in `class InnerClass$InnerClassA`,[Trace 2] `void InnerClass$InnerClassA.innerOuterBad()`, locks `this` in `class InnerClass$InnerClassA`,Method call: `void InnerClass.bar()`, locks `this` in `class InnerClass`] +codetoanalyze/java/starvation/InnerClass.java, InnerClass$InnerClassA.(InnerClass,java.lang.Object), 55, DEADLOCK, no_bucket, ERROR, [[Trace 1] `InnerClass$InnerClassA.(InnerClass,Object)`, locks `this` in `class InnerClass$InnerClassA`,Method call: `void InnerClass.lockOuter()`, locks `this` in `class InnerClass`,[Trace 2] `void InnerClass.FP_outerInnerOk(InnerClass$InnerClassA)`, locks `this` in `class InnerClass`,Method call: `void InnerClass$InnerClassA.lockInner()`, locks `this` in `class InnerClass$InnerClassA`] +codetoanalyze/java/starvation/InnerClass.java, InnerClass$InnerClassA.innerOuterBad():void, 39, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void InnerClass$InnerClassA.innerOuterBad()`, locks `this` in `class InnerClass$InnerClassA`,Method call: `void InnerClass.lockOuter()`, locks `this` in `class InnerClass`,[Trace 2] `void InnerClass.FP_outerInnerOk(InnerClass$InnerClassA)`, locks `this` in `class InnerClass`,Method call: `void InnerClass$InnerClassA.lockInner()`, locks `this` in `class InnerClass$InnerClassA`] +codetoanalyze/java/starvation/InnerClass.java, InnerClass.FP_outerInnerOk(InnerClass$InnerClassA):void, 21, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void InnerClass.FP_outerInnerOk(InnerClass$InnerClassA)`, locks `this` in `class InnerClass`,Method call: `void InnerClass$InnerClassA.lockInner()`, locks `this` in `class InnerClass$InnerClassA`,[Trace 2] `InnerClass$InnerClassA.(InnerClass,Object)`, locks `this` in `class InnerClass$InnerClassA`,Method call: `void InnerClass.lockOuter()`, locks `this` in `class InnerClass`] +codetoanalyze/java/starvation/InnerClass.java, InnerClass.FP_outerInnerOk(InnerClass$InnerClassA):void, 21, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void InnerClass.FP_outerInnerOk(InnerClass$InnerClassA)`, locks `this` in `class InnerClass`,Method call: `void InnerClass$InnerClassA.lockInner()`, locks `this` in `class InnerClass$InnerClassA`,[Trace 2] `void InnerClass$InnerClassA.innerOuterBad()`, locks `this` in `class InnerClass$InnerClassA`,Method call: `void InnerClass.lockOuter()`, locks `this` in `class InnerClass`] codetoanalyze/java/starvation/Interclass.java, Interclass.interclass1Bad(InterclassA):void, 10, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void Interclass.interclass1Bad(InterclassA)`, locks `this` in `class Interclass`,Method call: `void InterclassA.interclass1Bad()`, locks `this` in `class InterclassA`,[Trace 2] `void InterclassA.interclass2Bad(Interclass)`, locks `this` in `class InterclassA`,Method call: `void Interclass.interclass2Bad()`, locks `this` in `class Interclass`] codetoanalyze/java/starvation/Interclass.java, InterclassA.interclass2Bad(Interclass):void, 37, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void InterclassA.interclass2Bad(Interclass)`, locks `this` in `class InterclassA`,Method call: `void Interclass.interclass2Bad()`, locks `this` in `class Interclass`,[Trace 2] `void Interclass.interclass1Bad(InterclassA)`, locks `this` in `class Interclass`,Method call: `void InterclassA.interclass1Bad()`, locks `this` in `class InterclassA`] codetoanalyze/java/starvation/Interproc.java, Interproc.interproc1Bad(InterprocA):void, 10, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void Interproc.interproc1Bad(InterprocA)`, locks `this` in `class Interproc`,Method call: `void Interproc.interproc2Bad(InterprocA)`, locks `b` in `class InterprocA`,[Trace 2] `void InterprocA.interproc1Bad(Interproc)`, locks `this` in `class InterprocA`,Method call: `void InterprocA.interproc2Bad(Interproc)`, locks `d` in `class Interproc`]