From 834fec3299fb49b6e19b76a3611908e40d109987 Mon Sep 17 00:00:00 2001 From: gonczmisi Date: Mon, 9 May 2022 23:54:13 +0200 Subject: [PATCH 1/5] Introduced new ResourceValueFrame statuses and applied them to FindUnreleasedLock --- .../cs/findbugs/ba/ResourceValueAnalysis.java | 8 ++ .../cs/findbugs/ba/ResourceValueFrame.java | 21 ++++- .../findbugs/detect/FindUnreleasedLock.java | 91 +++++++++++-------- 3 files changed, 75 insertions(+), 45 deletions(-) diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueAnalysis.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueAnalysis.java index 8fb4b4c957e..a62d9664225 100644 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueAnalysis.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueAnalysis.java @@ -106,6 +106,9 @@ public void meetInto(ResourceValueFrame fact, Edge edge, ResourceValueFrame resu // If status is OPEN, downgrade to OPEN_ON_EXCEPTION_PATH tmpFact = modifyFrame(fact, null); tmpFact.setStatus(ResourceValueFrame.OPEN_ON_EXCEPTION_PATH); + } else { + tmpFact = modifyFrame(fact, null); + tmpFact.setStatus(ResourceValueFrame.NOT_OPEN_ON_EXCEPTION_PATH); } if (fact.isValid()) { @@ -137,6 +140,11 @@ public void meetInto(ResourceValueFrame fact, Edge edge, ResourceValueFrame resu tmpFact.pushValue(ResourceValue.notInstance()); } } + } else { + if (result.getStatus() == ResourceValueFrame.NOT_OPEN_ON_EXCEPTION_PATH && fact.getStatus() == ResourceValueFrame.CLOSED) { + tmpFact = modifyFrame(fact, null); + tmpFact.setStatus(ResourceValueFrame.CLOSED_WITHOUT_OPENED); + } } // Make the resource nonexistent if it is compared against null diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueFrame.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueFrame.java index fe8faf315ce..b8040f896e9 100644 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueFrame.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueFrame.java @@ -37,20 +37,31 @@ public class ResourceValueFrame extends Frame { */ public static final int OPEN_ON_EXCEPTION_PATH = 2; + /** + * The resource is closed without being opened in the first place. + */ + public static final int CLOSED_WITHOUT_OPENED = 3; + /** * The resource is closed (or unlocked, etc). */ - public static final int CLOSED = 3; + public static final int CLOSED = 4; /** * The resource has been created, but is not open. */ - public static final int CREATED = 4; + public static final int CREATED = 5; /** * The resource doesn't exist. */ - public static final int NONEXISTENT = 5; + public static final int NONEXISTENT = 6; + + /** + * The resource is NOT open (or locked, etc) on paths that include exception + * control flow. + */ + public static final int NOT_OPEN_ON_EXCEPTION_PATH = 7; private int status; @@ -84,8 +95,8 @@ public void copyFrom(Frame other_) { this.status = other.status; } - private static final String[] statusList = { "(escaped)", "(open)", "(open_exception)", "(closed)", "(created)", - "(nonexistent)" }; + private static final String[] statusList = { "(escaped)", "(open)", "(open_exception)", "(closed_without_opened)", "(closed)", "(created)", + "(nonexistent)", "(not_open_exception)" }; @Override public String toString() { diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindUnreleasedLock.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindUnreleasedLock.java index 3a1dac5c7c8..d59f44d63b3 100644 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindUnreleasedLock.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindUnreleasedLock.java @@ -21,6 +21,7 @@ import java.util.BitSet; +import java.util.Optional; import org.apache.bcel.Const; import org.apache.bcel.classfile.Constant; import org.apache.bcel.classfile.ConstantClass; @@ -108,29 +109,40 @@ public void transferInstruction(InstructionHandle handle, BasicBlock basicBlock) final ConstantPoolGen cpg = getCPG(); final ResourceValueFrame frame = getFrame(); - int status = -1; + int status = ResourceValueFrame.NONEXISTENT; if (DEBUG) { + System.out.println("Before transferInstruction status of frame: " + frame.getStatus()); System.out.println("PC : " + handle.getPosition() + " " + ins); - } - if (DEBUG && ins instanceof InvokeInstruction) { - System.out.println(" " + ins.toString(cpg.getConstantPool())); - } - if (DEBUG) { - System.out.println("resource frame before instruction: " + frame.toString()); + if (ins instanceof InvokeInstruction) { + System.out.println(" " + ins.toString(cpg.getConstantPool())); + } + System.out.println("resource frame before instruction: " + frame); } // Is a lock acquired or released by this instruction? Location creationPoint = lock.getLocation(); - if (handle == creationPoint.getHandle() && basicBlock == creationPoint.getBasicBlock()) { + if (frame.getStatus() == ResourceValueFrame.CLOSED_WITHOUT_OPENED) { + status = ResourceValueFrame.CLOSED_WITHOUT_OPENED; + if (DEBUG) { + System.out.println("CLOSED WITHOUT OPENED"); + } + } else if (handle == creationPoint.getHandle() && basicBlock == creationPoint.getBasicBlock()) { status = ResourceValueFrame.OPEN; if (DEBUG) { System.out.println("OPEN"); } } else if (resourceTracker.isResourceClose(basicBlock, handle, cpg, lock, frame)) { - status = ResourceValueFrame.CLOSED; - if (DEBUG) { - System.out.println("CLOSE"); + if (frame.getStatus() == ResourceValueFrame.NOT_OPEN_ON_EXCEPTION_PATH) { + status = ResourceValueFrame.CLOSED_WITHOUT_OPENED; + if (DEBUG) { + System.out.println("CLOSED WITHOUT OPENED"); + } + } else { + status = ResourceValueFrame.CLOSED; + if (DEBUG) { + System.out.println("CLOSE"); + } } } @@ -166,11 +178,11 @@ public void transferInstruction(InstructionHandle handle, BasicBlock basicBlock) } // If needed, update frame status - if (status != -1) { + if (status != ResourceValueFrame.NONEXISTENT) { frame.setStatus(status); } if (DEBUG) { - System.out.println("resource frame after instruction: " + frame.toString()); + System.out.println("resource frame after instruction: " + frame); } } @@ -185,9 +197,7 @@ class LockResourceTracker implements ResourceTracker { private final RepositoryLookupFailureCallback lookupFailureCallback; private final CFG cfg; - private final ValueNumberDataflow vnaDataflow; - private final IsNullValueDataflow isNullDataflow; public LockResourceTracker(RepositoryLookupFailureCallback lookupFailureCallback, CFG cfg, @@ -202,11 +212,12 @@ public LockResourceTracker(RepositoryLookupFailureCallback lookupFailureCallback public Lock isResourceCreation(BasicBlock basicBlock, InstructionHandle handle, ConstantPoolGen cpg) throws DataflowAnalysisException { - InvokeInstruction inv = toInvokeInstruction(handle.getInstruction()); - if (inv == null) { + Optional maybeInv = toInvokeInstruction(handle.getInstruction()); + if (!maybeInv.isPresent()) { return null; } + InvokeInstruction inv = maybeInv.get(); String className = inv.getClassName(cpg); String methodName = inv.getName(cpg); String methodSig = inv.getSignature(cpg); @@ -220,8 +231,6 @@ public Lock isResourceCreation(BasicBlock basicBlock, InstructionHandle handle, ValueNumber lockValue = frame.getTopValue(); if (DEBUG) { System.out.println("Lock value is " + lockValue.getNumber() + ", frame=" + frame.toString()); - } - if (DEBUG) { ++numAcquires; } return new Lock(location, className, lockValue); @@ -233,13 +242,13 @@ public Lock isResourceCreation(BasicBlock basicBlock, InstructionHandle handle, } @Override - public boolean mightCloseResource(BasicBlock basicBlock, InstructionHandle handle, ConstantPoolGen cpg) - throws DataflowAnalysisException { - InvokeInstruction inv = toInvokeInstruction(handle.getInstruction()); - if (inv == null) { + public boolean mightCloseResource(BasicBlock basicBlock, InstructionHandle handle, ConstantPoolGen cpg) { + Optional maybeInv = toInvokeInstruction(handle.getInstruction()); + if (!maybeInv.isPresent()) { return false; } + InvokeInstruction inv = maybeInv.get(); String className = inv.getClassName(cpg); String methodName = inv.getName(cpg); String methodSig = inv.getSignature(cpg); @@ -247,7 +256,6 @@ public boolean mightCloseResource(BasicBlock basicBlock, InstructionHandle handl try { if ("unlock".equals(methodName) && "()V".equals(methodSig) && Hierarchy.isSubtype(className, "java.util.concurrent.locks.Lock")) { - return true; } } catch (ClassNotFoundException e) { @@ -315,7 +323,6 @@ public boolean ignoreExceptionEdge(Edge edge, Lock resource, ConstantPoolGen cpg } else if (ins instanceof InvokeInstruction) { InvokeInstruction iins = (InvokeInstruction) ins; String methodName = iins.getMethodName(cpg); - // System.out.println("Method " + methodName); if (methodName.startsWith("access$")) { return true; } @@ -343,12 +350,12 @@ public boolean isParamInstance(Lock resource, int slot) { return false; } - private InvokeInstruction toInvokeInstruction(Instruction ins) { + private Optional toInvokeInstruction(Instruction ins) { short opcode = ins.getOpcode(); if (opcode != Const.INVOKEVIRTUAL && opcode != Const.INVOKEINTERFACE) { - return null; + return Optional.empty(); } - return (InvokeInstruction) ins; + return Optional.of((InvokeInstruction) ins); } } @@ -383,8 +390,8 @@ public void visitClassContext(ClassContext classContext) { boolean sawUtilConcurrentLocks = false; for (Constant c : jclass.getConstantPool().getConstantPool()) { if (c instanceof ConstantMethodref) { - ConstantMethodref m = (ConstantMethodref) c; - ConstantClass cl = (ConstantClass) jclass.getConstantPool().getConstant(m.getClassIndex()); + ConstantMethodref constantMethodref = (ConstantMethodref) c; + ConstantClass cl = (ConstantClass) jclass.getConstantPool().getConstant(constantMethodref.getClassIndex()); ConstantUtf8 name = (ConstantUtf8) jclass.getConstantPool().getConstant(cl.getNameIndex()); String nameAsString = name.getBytes(); if (nameAsString.startsWith("java/util/concurrent/locks")) { @@ -410,7 +417,7 @@ public boolean prescreen(ClassContext classContext, Method method, boolean might MethodGen methodGen = classContext.getMethodGen(method); - return methodGen != null && methodGen.getName().toLowerCase().indexOf("lock") == -1 + return methodGen != null && !methodGen.getName().toLowerCase().contains("lock") && (bytecodeSet.get(Const.INVOKEVIRTUAL) || bytecodeSet.get(Const.INVOKEINTERFACE)); } @@ -433,7 +440,19 @@ public void inspectResult(ClassContext classContext, MethodGen methodGen, CFG cf } int exitStatus = exitFrame.getStatus(); - if (exitStatus == ResourceValueFrame.OPEN || exitStatus == ResourceValueFrame.OPEN_ON_EXCEPTION_PATH) { + if (exitStatus == ResourceValueFrame.OPEN + || exitStatus == ResourceValueFrame.OPEN_ON_EXCEPTION_PATH + || exitStatus == ResourceValueFrame.CLOSED_WITHOUT_OPENED) { + String sourceFile = javaClass.getSourceFileName(); + Location location = resource.getLocation(); + InstructionHandle handle = location.getHandle(); + InstructionHandle nextInstruction = handle.getNext(); + + // Exit early and don't report as error, intentionally + if (nextInstruction.getInstruction() instanceof RETURN) { + return; + } + String bugType; int priority; if (exitStatus == ResourceValueFrame.OPEN) { @@ -443,14 +462,6 @@ public void inspectResult(ClassContext classContext, MethodGen methodGen, CFG cf bugType = "UL_UNRELEASED_LOCK_EXCEPTION_PATH"; priority = NORMAL_PRIORITY; } - - String sourceFile = javaClass.getSourceFileName(); - Location location = resource.getLocation(); - InstructionHandle handle = location.getHandle(); - InstructionHandle nextInstruction = handle.getNext(); - if (nextInstruction.getInstruction() instanceof RETURN) { - return; // don't report as error; intentional - } bugAccumulator.accumulateBug(new BugInstance(this, bugType, priority).addClassAndMethod(methodGen, sourceFile), SourceLineAnnotation.fromVisitedInstruction(classContext, methodGen, sourceFile, handle)); } From 2d35194c5352ad010c18fba9294c9d04b6fd3093 Mon Sep 17 00:00:00 2001 From: gonczmisi Date: Mon, 9 May 2022 23:55:08 +0200 Subject: [PATCH 2/5] Added new bugtype CWO_CLOSED_WITHOUT_OPENED --- CHANGELOG.md | 3 + .../detect/FindUnreleasedLockTest.java | 60 +++++++++++++++++++ spotbugs/etc/findbugs.xml | 4 +- spotbugs/etc/messages.xml | 28 ++++++--- .../findbugs/detect/FindUnreleasedLock.java | 5 +- .../unreleasedLock/ProperlyClosedLock.java | 33 ++++++++++ .../ThrowExceptionAndUnlockBeforeLock.java | 33 ++++++++++ .../java/unreleasedLock/UnreleasedLock.java | 39 ++++++++++++ 8 files changed, 196 insertions(+), 9 deletions(-) create mode 100644 spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindUnreleasedLockTest.java create mode 100644 spotbugsTestCases/src/java/unreleasedLock/ProperlyClosedLock.java create mode 100644 spotbugsTestCases/src/java/unreleasedLock/ThrowExceptionAndUnlockBeforeLock.java create mode 100644 spotbugsTestCases/src/java/unreleasedLock/UnreleasedLock.java diff --git a/CHANGELOG.md b/CHANGELOG.md index e2a0e90dffb..4a0e3a2c8cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,9 @@ Currently the versioning policy of this project follows [Semantic Versioning v2. * New detector `DontUseFloatsAsLoopCounters` to detect usage of floating-point variables as loop counters (`FL_FLOATS_AS_LOOP_COUNTERS`), according to SEI CERT rules [NUM09-J. Do not use floating-point variables as loop counters](https://wiki.sei.cmu.edu/confluence/display/java/NUM09-J.+Do+not+use+floating-point+variables+as+loop+counters) * New test detector `ViewCFG` to visualize the control-flow graph for `SpotBugs` developers +### Added +* New bug type `CWO_CLOSED_WITHOUT_OPENED` for locks that might be released without even being acquired. (See[SEI CERT rule LCK08-J](https://wiki.sei.cmu.edu/confluence/display/java/LCK08-J.+Ensure+actively+held+locks+are+released+on+exceptional+conditions)) + ## 4.6.0 - 2022-03-08 ### Fixed - Fixed spotbugs build with ecj compiler ([#1903](https://github.com/spotbugs/spotbugs/issues/1903)) diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindUnreleasedLockTest.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindUnreleasedLockTest.java new file mode 100644 index 00000000000..c88e64fea87 --- /dev/null +++ b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindUnreleasedLockTest.java @@ -0,0 +1,60 @@ +package edu.umd.cs.findbugs.detect; + +import edu.umd.cs.findbugs.AbstractIntegrationTest; +import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher; +import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder; +import org.junit.Test; + +import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly; +import static org.hamcrest.CoreMatchers.hasItem; +import static org.hamcrest.MatcherAssert.assertThat; + +public class FindUnreleasedLockTest extends AbstractIntegrationTest { + + @Test + public void doesNotFindSSDBug_ifLockIsClosedInAProperWay() { + performAnalysis("unreleasedLock/ProperlyClosedLock.class"); + + assertAnalyzedClassHasNoBugs(); + } + + @Test + public void findPossibleUnlockCall_withoutLockCall() { + //SystemProperties.setProperty("ful.debug", "true"); + performAnalysis("unreleasedLock/ThrowExceptionAndUnlockBeforeLock.class"); + + assertAnalyzedClassHasBug("CWO_CLOSED_WITHOUT_OPENED", 1); + assertExactPlaceOfBug("ThrowExceptionAndUnlockBeforeLock", "doSomething", "CWO_CLOSED_WITHOUT_OPENED"); + } + + @Test + public void findUnresolvedLock_onExceptionalCondition() { + performAnalysis("unreleasedLock/UnreleasedLock.class"); + + assertAnalyzedClassHasBug("UL_UNRELEASED_LOCK_EXCEPTION_PATH", 1); + assertExactPlaceOfBug("UnreleasedLock", "doSomething", "UL_UNRELEASED_LOCK_EXCEPTION_PATH"); + } + + private void assertAnalyzedClassHasNoBugs() { + final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder() + .build(); + assertThat(getBugCollection(), containsExactly(0, bugTypeMatcher)); + } + + private void assertAnalyzedClassHasBug(String bugType, int numberOfBugs) { + final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder() + .bugType(bugType) + .build(); + assertThat(getBugCollection(), containsExactly(numberOfBugs, bugTypeMatcher)); + } + + private void assertExactPlaceOfBug(String className, String methodName, String bugType) { + final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder() + .bugType(bugType) + .inClass(className) + .inMethod(methodName) + .build(); + assertThat(getBugCollection(), hasItem(bugInstanceMatcher)); + } + +} diff --git a/spotbugs/etc/findbugs.xml b/spotbugs/etc/findbugs.xml index 8d60497ae13..469983bb94c 100644 --- a/spotbugs/etc/findbugs.xml +++ b/spotbugs/etc/findbugs.xml @@ -497,7 +497,8 @@ + requirejre="1.5" + reports="UL_UNRELEASED_LOCK,UL_UNRELEASED_LOCK_EXCEPTION_PATH,CWO_CLOSED_WITHOUT_OPENED"/> + diff --git a/spotbugs/etc/messages.xml b/spotbugs/etc/messages.xml index 249492647ba..2fe3b8b51d4 100644 --- a/spotbugs/etc/messages.xml +++ b/spotbugs/etc/messages.xml @@ -1054,13 +1054,14 @@ Returning a zero length array is generally preferred in this context to returnin
- This detector looks for JSR-166 (java.util.concurrent) -locks which are acquired, but not released on all paths out of the method.  -It is a moderately fast detector.  Note that in order to use this -detector, you need to have the java.util.concurrent package -in the auxiliary classpath (or be analyzing the package itself).

-]]> + This detector looks for JSR-166 (java.util.concurrent) + locks which are acquired, but not released on all paths out of the method.  + The detector also detects if a lock is released without even being opened in the first place. + It is a moderately fast detector.  Note that in order to use this + detector, you need to have the java.util.concurrent package + in the auxiliary classpath (or be analyzing the package itself).

+ ]]>
@@ -8786,6 +8787,18 @@ Using floating-point variables should not be used as loop counters, as they are

]]> + + Method releases lock without opening it + {1} releases lock without acquiring it in the first place +
+ This method releases a JSR-166 (java.util.concurrent) lock, + but it is possible that the lock is not even acquired at this point, + due to an exception thrower method before the locking. +

+ ]]> +
+