LCK08-J. Ensure actively held locks are released on exceptional conditions

CHANGELOG.md

@@ -5,6 +5,9 @@ This is the changelog for SpotBugs. This follows [Keep a Changelog v1.0.0](http:
 Currently the versioning policy of this project follows [Semantic Versioning v2.0.0](http://semver.org/spec/v2.0.0.html).
 
 ## Unreleased - 2023-??-??
+### 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))([#2055](https://github.com/spotbugs/spotbugs/pull/2055))
+
 ### Changed
 - Bump up Apache Commons BCEL to the version 6.6.1 ([#2223](https://github.com/spotbugs/spotbugs/pull/2223))
 - Bump up slf4j-api to 2.0.3 ([#2220](https://github.com/spotbugs/spotbugs/pull/2220))

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));
+    }
+
+}

spotbugs/etc/findbugs.xml

@@ -497,7 +497,8 @@
           <Detector class="edu.umd.cs.findbugs.detect.FindUselessControlFlow" speed="fast"
                     reports="UCF_USELESS_CONTROL_FLOW,UCF_USELESS_CONTROL_FLOW_NEXT_LINE"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindUnreleasedLock" speed="moderate"
-                    requirejre="1.5" reports="UL_UNRELEASED_LOCK,UL_UNRELEASED_LOCK_EXCEPTION_PATH"/>
+                    requirejre="1.5"
+                    reports="UL_UNRELEASED_LOCK,UL_UNRELEASED_LOCK_EXCEPTION_PATH,CWO_CLOSED_WITHOUT_OPENED"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindRefComparison" speed="slow"
                     reports="ES_COMPARING_STRINGS_WITH_EQ,ES_COMPARING_PARAMETER_STRING_WITH_EQ,RC_REF_COMPARISON,RC_REF_COMPARISON_BAD_PRACTICE,RC_REF_COMPARISON_BAD_PRACTICE_BOOLEAN,EC_UNRELATED_TYPES,EC_NULL_ARG,EC_UNRELATED_CLASS_AND_INTERFACE,EC_UNRELATED_INTERFACES,EC_ARRAY_AND_NONARRAY,EC_INCOMPATIBLE_ARRAY_COMPARE,EC_BAD_ARRAY_COMPARE,EC_UNRELATED_TYPES_USING_POINTER_EQUALITY,DMI_DOH"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindMismatchedWaitOrNotify" speed="moderate"
@@ -1286,4 +1287,5 @@
           <BugPattern abbrev="PERM" type="PERM_SUPER_NOT_CALLED_IN_GETPERMISSIONS" category="MALICIOUS_CODE" />
           <BugPattern abbrev="USC" type="USC_POTENTIAL_SECURITY_CHECK_BASED_ON_UNTRUSTED_SOURCE"
                     category="MALICIOUS_CODE"/>
+          <BugPattern abbrev="CWO" type="CWO_CLOSED_WITHOUT_OPENED" category="MT_CORRECTNESS" />
 </FindbugsPlugin>

spotbugs/etc/messages.xml

@@ -1054,13 +1054,14 @@ Returning a zero length array is generally preferred in this context to returnin
   </Detector>
   <Detector class="edu.umd.cs.findbugs.detect.FindUnreleasedLock">
     <Details>
-<![CDATA[
-<p> This detector looks for JSR-166 (<code>java.util.concurrent</code>)
-locks which are acquired, but not released on all paths out of the method.&nbsp;
-It is a moderately fast detector.&nbsp; Note that in order to use this
-detector, you need to have the <code>java.util.concurrent</code> package
-in the auxiliary classpath (or be analyzing the package itself).</p>
-]]>
+      <![CDATA[
+        <p> This detector looks for JSR-166 (<code>java.util.concurrent</code>)
+        locks which are acquired, but not released on all paths out of the method.&nbsp;
+        The detector also detects if a lock is released without even being opened in the first place.
+        It is a moderately fast detector.&nbsp; Note that in order to use this
+        detector, you need to have the <code>java.util.concurrent</code> package
+        in the auxiliary classpath (or be analyzing the package itself).</p>
+      ]]>
     </Details>
   </Detector>
   <Detector class="edu.umd.cs.findbugs.detect.FindRefComparison">
@@ -8785,6 +8786,18 @@ Using floating-point variables should not be used as loop counters, as they are
       </p>]]>
     </Details>
   </BugPattern>
+  <BugPattern type="CWO_CLOSED_WITHOUT_OPENED">
+    <ShortDescription>Method releases lock without opening it</ShortDescription>
+    <LongDescription>{1} releases lock without acquiring it in the first place</LongDescription>
+    <Details>
+      <![CDATA[
+        <p> This method releases a JSR-166 (<code>java.util.concurrent</code>) lock,
+        but it is possible that the lock is not even acquired at this point,
+        due to an exception thrower method before the locking.
+        </p>
+      ]]>
+    </Details>
+  </BugPattern>
   <!--
   **********************************************************************
    BugCodes
@@ -8938,4 +8951,5 @@ Using floating-point variables should not be used as loop counters, as they are
   <BugCode abbrev="THROWS">Exception throwing related problems</BugCode>
   <BugCode abbrev="PERM">Custom class loader does not call its superclass's getPermissions()</BugCode>
   <BugCode abbrev="USC">Potential security check based on untrusted source</BugCode>
+  <BugCode abbrev="CWO">Lock closed without being opened in the first place</BugCode>
 </MessageCollection>

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

spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueFrame.java

@@ -37,20 +37,31 @@ public class ResourceValueFrame extends Frame<ResourceValue> {
      */
     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<ResourceValue> 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() {