Warn for catching RuntimeException, Exception and Throwable

CHANGELOG.md

@@ -6,6 +6,9 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 
 ## Unreleased - 2022-??-??
 
+### Added
+- Detector `RuntimeExceptionCapture` extended to detect catching of `RuntimeException`, `Exception` and `Throwable` according to [SEI CERT rule ERR08-J](https://wiki.sei.cmu.edu/confluence/display/java/ERR08-J.+Do+not+catch+NullPointerException+or+any+of+its+ancestors)
+
 ## 4.7.1 - 2022-06-26
 ### Fixed
 - Fixed False positives for `RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE` on try-with-resources with interface references ([#1931](https://github.com/spotbugs/spotbugs/issues/1931))

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java

@@ -0,0 +1,55 @@
+package edu.umd.cs.findbugs.detect;
+
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import org.junit.Test;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.annotations.Confidence;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+
+public class RuntimeExceptionCapture2Test extends AbstractIntegrationTest {
+    @Test
+    public void test() {
+        performAnalysis("REC2Test.class");
+
+        fail("testFail", 54, "REC2_CATCH_RUNTIME_EXCEPTION", Confidence.LOW);
+        fail("testFail", 56, "REC2_CATCH_EXCEPTION", Confidence.LOW);
+        fail("testFail2", 67, "REC2_CATCH_RUNTIME_EXCEPTION", Confidence.LOW);
+        fail("testFail3", 79, "REC2_CATCH_THROWABLE", Confidence.LOW);
+
+        pass("testPass", "REC2_CATCH_RUNTIME_EXCEPTION");
+        pass("testPass", "REC2_CATCH_EXCEPTION");
+        pass("testPass2", "REC2_CATCH_THROWABLE");
+        pass("testPass3", "REC2_CATCH_THROWABLE");
+    }
+
+    private void fail(String methodName, int line, String exceptionName, Confidence confidence) {
+        countBugs(methodName, exceptionName, 1);
+        final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType(exceptionName)
+                .inClass("REC2Test")
+                .inMethod(methodName)
+                .withConfidence(confidence)
+                .atLine(line)
+                .build();
+        assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
+    }
+
+    private void pass(String methodName, String exceptionName) {
+        countBugs(methodName, exceptionName, 0);
+    }
+
+    private void countBugs(String methodName, String exceptionName, int count) {
+        BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType(exceptionName)
+                .inClass("REC2Test")
+                .inMethod(methodName)
+                .build();
+        assertThat(getBugCollection(), containsExactly(count, bugTypeMatcher));
+    }
+}

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java

@@ -0,0 +1,61 @@
+package edu.umd.cs.findbugs.detect;
+
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import org.junit.Test;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.annotations.Confidence;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+
+public class RuntimeExceptionCaptureTest extends AbstractIntegrationTest {
+    @Test
+    public void test() {
+        performAnalysis("RECTest.class");
+
+        fail("testFail", 46, Confidence.MEDIUM);
+        fail("testFail2", 58, Confidence.MEDIUM);
+        fail("testFail3", 71, Confidence.MEDIUM);
+        fail("testFail4", 83, Confidence.MEDIUM);
+        fail("testFail5", 95, Confidence.MEDIUM);
+        fail("testFail6", 108, Confidence.MEDIUM);
+        fail("testFail7", 123, Confidence.LOW);
+
+        pass("testPass");
+        pass("testPass2");
+        pass("testPass3");
+        pass("testPass4");
+        pass("testPass5");
+        pass("testPass6");
+        pass("testPass7");
+    }
+
+    private void fail(String methodName, int line, Confidence confidence) {
+        countBugs(methodName, 1);
+        final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType("REC_CATCH_EXCEPTION")
+                .inClass("RECTest")
+                .inMethod(methodName)
+                .withConfidence(confidence)
+                .atLine(line)
+                .build();
+        assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
+    }
+
+    private void pass(String methodName) {
+        countBugs(methodName, 0);
+    }
+
+    private void countBugs(String methodName, int count) {
+        BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("REC_CATCH_EXCEPTION")
+                .inClass("RECTest")
+                .inMethod(methodName)
+                .build();
+        assertThat(getBugCollection(), containsExactly(count, bugTypeMatcher));
+    }
+}

spotbugs/etc/findbugs.xml

@@ -548,7 +548,7 @@
           <Detector class="edu.umd.cs.findbugs.detect.InstantiateStaticClass" speed="fast"
                     reports="ISC_INSTANTIATE_STATIC_CLASS"/>
           <Detector class="edu.umd.cs.findbugs.detect.RuntimeExceptionCapture"
-                    reports="REC_CATCH_EXCEPTION" speed="fast"/>
+                    reports="REC_CATCH_EXCEPTION,REC2_CATCH_EXCEPTION,REC2_CATCH_RUNTIME_EXCEPTION,REC2_CATCH_THROWABLE" speed="fast"/>
           <Detector class="edu.umd.cs.findbugs.detect.DontCatchNullPointerException"
                     reports="DCN_NULLPOINTER_EXCEPTION" speed="fast"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindFloatEquality" speed="fast"
@@ -1154,6 +1154,9 @@
           <BugPattern abbrev="WMI" type="WMI_WRONG_MAP_ITERATOR" category="PERFORMANCE"/>
           <BugPattern abbrev="ISC" type="ISC_INSTANTIATE_STATIC_CLASS" category="BAD_PRACTICE"/>
           <BugPattern abbrev="REC" type="REC_CATCH_EXCEPTION" category="STYLE" cweid="396"/>
+          <BugPattern abbrev="REC2" type="REC2_CATCH_EXCEPTION" category="BAD_PRACTICE"/>
+          <BugPattern abbrev="REC2" type="REC2_CATCH_RUNTIME_EXCEPTION" category="BAD_PRACTICE"/>
+          <BugPattern abbrev="REC2" type="REC2_CATCH_THROWABLE" category="BAD_PRACTICE"/>
           <BugPattern abbrev="DCN" type="DCN_NULLPOINTER_EXCEPTION" category="STYLE"/>
           <BugPattern abbrev="FE" type="FE_FLOATING_POINT_EQUALITY" category="STYLE"/>
           <BugPattern abbrev="FE" type="FE_TEST_IF_EQUAL_TO_NOT_A_NUMBER" category="CORRECTNESS"/>

spotbugs/etc/messages.xml

@@ -6991,6 +6991,74 @@ does not need to be created, just access the static methods directly using the c
   ]]>
      </Details>
   </BugPattern>
+  <BugPattern type="REC2_CATCH_EXCEPTION">
+    <ShortDescription>Exception is caught when Exception is not thrown</ShortDescription>
+    <LongDescription>Exception is caught when Exception is not thrown in {1}</LongDescription>
+    <Details>
+  <![CDATA[
+  <p>
+    This method uses a try-catch block that catches Exception objects, but Exception is not
+    thrown within the try block. This is risky, because future changes to signatures of method
+    invoked in the try block could add more exceptions to the list of potential exceptions the
+    caller must handle, or adding method calls into the try block may also throw additional
+    exceptions which must be handled. Additionally, adding code into the try block in the future
+    may introduce the throwing of new runtime exceptions either because of programming errors
+    (e.g. improper checks such as a null pointer check) or intentionally but some of them may
+    require special handling. However, catching RuntimeException itself hides the problem of not
+    handling these exceptions and not fixing the errors.However, catching Exception itself hides
+    the problem of not handling these exceptions. Instead, specify all the exceptions that must
+    be handled here.
+  </p>
+  <p>
+    See SEI CERT rule <a href="https://wiki.sei.cmu.edu/confluence/display/java/ERR08-J.+Do+not+catch+NullPointerException+or+any+of+its+ancestors">ERR08-J. Do not catch NullPointerException or any of its ancestors</a>.
+  </p>
+  ]]>
+    </Details>
+  </BugPattern>
+  <BugPattern type="REC2_CATCH_RUNTIME_EXCEPTION">
+    <ShortDescription>Exception is caught when RuntimeException is not thrown</ShortDescription>
+    <LongDescription>Exception is caught when RuntimeException is not thrown in {1}</LongDescription>
+    <Details>
+  <![CDATA[
+  <p>
+    This method uses a try-catch block that catches RuntimeException objects, but RuntimeException is not
+    thrown explicitly within the try block. This is risky, because various runtime exceptions may occur
+    in the try block and adding code into the try block in the future may introduce the throwing
+    of new runtime exceptions either because of programming errors (e.g. improper checks such as a null
+    pointer check) or intentionally but some of them may require special handling. However, catching
+    RuntimeException itself hides the problem of not handling these exceptions and not fixing the errors.
+    Instead, specify all the runtime exceptions that must be handled here.
+  </p>
+  <p>
+      See SEI CERT rule <a href="https://wiki.sei.cmu.edu/confluence/display/java/ERR08-J.+Do+not+catch+NullPointerException+or+any+of+its+ancestors">ERR08-J. Do not catch NullPointerException or any of its ancestors</a>.
+  </p>
+  ]]>
+    </Details>
+  </BugPattern>
+  <BugPattern type="REC2_CATCH_THROWABLE">
+    <ShortDescription>Exception is caught when Throwable is not thrown</ShortDescription>
+    <LongDescription>Exception is caught when Throwable is not thrown in {1}</LongDescription>
+    <Details>
+  <![CDATA[
+  <p>
+    This method uses a try-catch block that catches Throwable objects, but Throwable is not
+    thrown explicitly within the try block. This is risky, because future changes to signatures
+    of method invoked in the try block could add more exceptions to the list of potential
+    exceptions the caller must handle, or adding method calls into the try block may also throw
+    additional exceptions which must be handled. Additionally, adding code into the try block in
+    the future may introduce the throwing of new runtime exceptions either because of programming
+    errors (e.g. improper checks such as a null pointer check) or intentionally but some of them may
+    require special handling. However, catching Throwable itself hides the problem of not
+    handling these exceptions and not fixing the errors.However, catching Throwable itself hides
+    the problem of not handling these exceptions. Instead, specify all the exceptions that must
+    be handled here.
+  </p>
+  <p>
+      See SEI CERT rule <a href="https://wiki.sei.cmu.edu/confluence/display/java/ERR08-J.+Do+not+catch+NullPointerException+or+any+of+its+ancestors">ERR08-J. Do not catch NullPointerException or any of its ancestors</a>.
+  </p>
+  ]]>
+    </Details>
+  </BugPattern>
   <BugPattern type="DCN_NULLPOINTER_EXCEPTION">
     <ShortDescription>NullPointerException caught</ShortDescription>
     <LongDescription>Do not catch NullPointerException like in {1}</LongDescription>
@@ -8890,6 +8958,7 @@ Using floating-point variables should not be used as loop counters, as they are
   <BugCode abbrev="ISC">Instantiated Static Class</BugCode>
   <BugCode abbrev="DCN">Don't Catch NullPointer Exception</BugCode>
   <BugCode abbrev="REC">RuntimeException capture</BugCode>
+  <BugCode abbrev="REC2">RuntimeException capture</BugCode>
   <BugCode abbrev="FE">Test for floating point equality</BugCode>
   <BugCode abbrev="UM">Unnecessary Math on constants</BugCode>
   <BugCode abbrev="UC">Useless code</BugCode>

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindNullDeref.java

@@ -49,8 +49,6 @@
 import org.apache.bcel.generic.IFNONNULL;
 import org.apache.bcel.generic.IFNULL;
 import org.apache.bcel.generic.INVOKEDYNAMIC;
-import org.apache.bcel.generic.INVOKEINTERFACE;
-import org.apache.bcel.generic.INVOKEVIRTUAL;
 import org.apache.bcel.generic.Instruction;
 import org.apache.bcel.generic.InstructionHandle;
 import org.apache.bcel.generic.InstructionList;
@@ -134,6 +132,8 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import static edu.umd.cs.findbugs.util.Util.isGeneratedCodeInCatchBlockViaLineNumber;
+
 /**
  * A Detector to find instructions where a NullPointerException might be raised.
  * We also look for useless reference comparisons involving null and non-null
@@ -1834,7 +1834,7 @@ private boolean isGeneratedCodeInCatchBlock(@NonNull ConstantPool cp, int pc) {
             // this line also marks the end of the try catch block
             int line = lineNumberTable.getSourceLine(pc);
             if (line > 0) {
-                return isGeneratedCodeInCatchBlockViaLineNumber(cp, pc, lineNumberTable, line, list, throwableList);
+                return isGeneratedCodeInCatchBlockViaLineNumber(cp, lineNumberTable, line, list, throwableList);
             }
         }
 
@@ -1849,112 +1849,4 @@ private boolean isGeneratedCodeInCatchBlock(@NonNull ConstantPool cp, int pc) {
                     return codeException.getEndPC() + insnLength == pc;
                 });
     }
-
-    /**
-     * Java 11+ compiler generates redundant null checks for try-with-resources.
-     * This method tries to distinguish such code from manually written code by analysing null numbers.
-     * @param cp the constant pool
-     * @param pc the program counter
-     * @param lineNumberTable the table with the line numbers
-     * @param line the line which belongs to the program counter
-     * @param instructions the list of instructions
-     * @param throwables the list of Throwables in this method
-     * @return true if the pc specifies redundant null check generated by javac
-     */
-    private boolean isGeneratedCodeInCatchBlockViaLineNumber(@NonNull ConstantPool cp, int pc, @NonNull LineNumberTable lineNumberTable, int line,
-            @NonNull InstructionList instructions, @NonNull List<CodeException> throwables) {
-        // The compiler generated code can also be written by the regular program. The only
-        // difference is that the line numbers for a lot of instructions are the same.
-        // This is what the code below relies on. Line numbers are optional, so it might
-        // not work in call cases.
-        //
-        // The following operations must have the same line numbers as the start or end of the original try catch block.
-        // - the reported code position (which is the null check)
-        // - at least one assignment
-        // - at least one call of addSuppressed
-        // - at least one throws statement
-        //
-        // The two calls to the close method need to have the same line number as the end of a throwables catch block.
-        // There must be also another catch of Throwable, but the line number does not need to match.
-        //
-        // To understand the code, please have a look at the test Issue600Test.
-        // TryWithResources* are the try-with-resources examples, ExplicitNullCheck* is TryWithResources* compiled and then decompiled.
-
-        ConstantPoolGen cpg = new ConstantPoolGen(cp);
-
-        // The two generated catch blocks might show different starting line numbers. Both are needed.
-        Set<Integer> relevantLineNumbers = throwables.stream()
-                .map(x -> lineNumberTable.getSourceLine(x.getStartPC()))
-                .collect(Collectors.toSet());
-        relevantLineNumbers.add(line);
-
-        boolean assignmentPresent = false;
-        boolean addSuppressedPresent = false;
-        boolean throwsPresent = false;
-        int closeCounter = 0;
-        for (InstructionHandle handle : instructions.getInstructionHandles()) {
-            int currentLine = lineNumberTable.getSourceLine(handle.getPosition());
-
-            switch (handle.getInstruction().getOpcode()) {
-            case Const.ASTORE:
-            case Const.ASTORE_0:
-            case Const.ASTORE_1:
-            case Const.ASTORE_2:
-            case Const.ASTORE_3:
-                if (relevantLineNumbers.contains(currentLine)) {
-                    assignmentPresent = true;
-                }
-                break;
-            case Const.INVOKEVIRTUAL:
-                if (handle.getInstruction() instanceof INVOKEVIRTUAL) {
-                    String methodName = ((INVOKEVIRTUAL) handle.getInstruction()).getMethodName(cpg);
-                    if ("close".equals(methodName)) {
-                        // the close methods get the line number of the end of the try block assigned
-                        if (throwables.stream().anyMatch(x -> lineNumberTable.getSourceLine(x.getEndPC()) == currentLine)) {
-                            closeCounter++;
-                        }
-                    } else if ("addSuppressed".equals(methodName)) {
-                        if (relevantLineNumbers.contains(currentLine)) {
-                            addSuppressedPresent = true;
-                        }
-                    }
-                }
-                break;
-            case Const.INVOKEINTERFACE:
-                if (handle.getInstruction() instanceof INVOKEINTERFACE) {
-                    String methodName = ((INVOKEINTERFACE) handle.getInstruction()).getMethodName(cpg);
-                    if ("close".equals(methodName)) {
-                        // the close methods get the line number of the end of the try block assigned
-                        if (throwables.stream().anyMatch(x -> lineNumberTable.getSourceLine(x.getEndPC()) == currentLine)) {
-                            closeCounter++;
-                        }
-                    } else if ("addSuppressed".equals(methodName)) {
-                        if (relevantLineNumbers.contains(currentLine)) {
-                            addSuppressedPresent = true;
-                        }
-                    }
-                }
-                break;
-            case Const.ATHROW:
-                if (relevantLineNumbers.contains(currentLine) && handle.getInstruction() instanceof ATHROW) {
-                    Class<?>[] exceptions = ((ATHROW) handle.getInstruction()).getExceptions();
-                    if (exceptions.length == 1 && Throwable.class.equals(exceptions[0])) {
-                        // even if try-with-resources catches exceptions, the compiler generates a nested try-catch with Throwable.
-                        throwsPresent = true;
-                    }
-                }
-                break;
-            default:
-                break;
-            }
-        }
-        boolean matchingCatches = false;
-        if (throwables.size() >= 2) {
-            // make sure that the reported line matches the start or end line of the generated try catch blocks
-            matchingCatches = throwables.stream().anyMatch(
-                    x -> lineNumberTable.getSourceLine(x.getStartPC()) == line) || throwables.stream().anyMatch(
-                            x -> lineNumberTable.getSourceLine(x.getEndPC()) == line);
-        }
-        return matchingCatches && assignmentPresent && addSuppressedPresent && throwsPresent && closeCounter >= 2;
-    }
 }