From 856cead855e1a94931b480914e60c1e135f94a82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81d=C3=A1m=20Balogh?= Date: Wed, 6 Jul 2022 18:37:44 +0200 Subject: [PATCH 1/9] Warn for catching 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) catching `RuntimeException`, `Exception` and `Throwable` is a bad practice because it may hide exceptions which must be handled or avoided. This PR extends detector `RuntimeExceptionCapture` to warn for these cases. --- CHANGELOG.md | 3 + .../detect/RuntimeExceptionCapture2Test.java | 55 ++++++++ .../detect/RuntimeExceptionCaptureTest.java | 61 +++++++++ spotbugs/etc/findbugs.xml | 5 +- spotbugs/etc/messages.xml | 69 ++++++++++ .../umd/cs/findbugs/detect/FindNullDeref.java | 114 +---------------- .../detect/RuntimeExceptionCapture.java | 66 +++++++--- .../java/edu/umd/cs/findbugs/util/Util.java | 120 ++++++++++++++++++ spotbugsTestCases/src/java/REC2Test.java | 116 +++++++++++++++++ spotbugsTestCases/src/java/RECTest.java | 43 +++++++ .../src/java/lambdas/Issue60.java | 1 - 11 files changed, 521 insertions(+), 132 deletions(-) create mode 100644 spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java create mode 100644 spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java create mode 100755 spotbugsTestCases/src/java/REC2Test.java diff --git a/CHANGELOG.md b/CHANGELOG.md index cefc5a39a1a..768e964bca2 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java new file mode 100644 index 00000000000..681f117b8db --- /dev/null +++ b/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)); + } +} diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java new file mode 100644 index 00000000000..dfcb033a452 --- /dev/null +++ b/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)); + } +} diff --git a/spotbugs/etc/findbugs.xml b/spotbugs/etc/findbugs.xml index e9be54de7d9..27240e2e2f3 100644 --- a/spotbugs/etc/findbugs.xml +++ b/spotbugs/etc/findbugs.xml @@ -548,7 +548,7 @@ + reports="REC_CATCH_EXCEPTION,REC2_CATCH_EXCEPTION,REC2_CATCH_RUNTIME_EXCEPTION,REC2_CATCH_THROWABLE" speed="fast"/> + + + diff --git a/spotbugs/etc/messages.xml b/spotbugs/etc/messages.xml index 3db916f01c9..2e5685caabf 100644 --- a/spotbugs/etc/messages.xml +++ b/spotbugs/etc/messages.xml @@ -6991,6 +6991,74 @@ does not need to be created, just access the static methods directly using the c ]]> + + Exception is caught when Exception is not thrown + Exception is caught when Exception is not thrown in {1} +
+ + 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. +

+

+ See SEI CERT rule ERR08-J. Do not catch NullPointerException or any of its ancestors. +

+ ]]> +
+ + + Exception is caught when RuntimeException is not thrown + Exception is caught when RuntimeException is not thrown in {1} +
+ + 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. +

+

+ See SEI CERT rule ERR08-J. Do not catch NullPointerException or any of its ancestors. +

+ ]]> +
+ + + Exception is caught when Throwable is not thrown + Exception is caught when Throwable is not thrown in {1} +
+ + 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. +

+

+ See SEI CERT rule ERR08-J. Do not catch NullPointerException or any of its ancestors. +

+ ]]> +
+ NullPointerException caught Do not catch NullPointerException like in {1} @@ -8890,6 +8958,7 @@ Using floating-point variables should not be used as loop counters, as they are Instantiated Static Class Don't Catch NullPointer Exception RuntimeException capture + RuntimeException capture Test for floating point equality Unnecessary Math on constants Useless code diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindNullDeref.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindNullDeref.java index 09d246cc8e3..974a91f2341 100644 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindNullDeref.java +++ b/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 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 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; - } } diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture.java index d5a62b684a6..90069955ea2 100755 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture.java @@ -21,6 +21,7 @@ package edu.umd.cs.findbugs.detect; import java.util.ArrayList; +import java.util.Arrays; import java.util.BitSet; import java.util.Collection; import java.util.HashSet; @@ -30,10 +31,13 @@ import org.apache.bcel.Const; import org.apache.bcel.classfile.Code; import org.apache.bcel.classfile.CodeException; +import org.apache.bcel.classfile.ConstantPool; import org.apache.bcel.classfile.JavaClass; +import org.apache.bcel.classfile.LineNumberTable; import org.apache.bcel.classfile.Method; import org.apache.bcel.generic.ASTORE; import org.apache.bcel.generic.InstructionHandle; +import org.apache.bcel.generic.InstructionList; import edu.umd.cs.findbugs.BugAccumulator; import edu.umd.cs.findbugs.BugInstance; @@ -46,6 +50,7 @@ import edu.umd.cs.findbugs.ba.CFG; import edu.umd.cs.findbugs.ba.CFGBuilderException; import edu.umd.cs.findbugs.ba.DataflowAnalysisException; +import edu.umd.cs.findbugs.ba.EdgeTypes; import edu.umd.cs.findbugs.ba.Hierarchy2; import edu.umd.cs.findbugs.ba.LiveLocalStoreDataflow; import edu.umd.cs.findbugs.ba.Location; @@ -61,6 +66,8 @@ import edu.umd.cs.findbugs.internalAnnotations.DottedClassName; import edu.umd.cs.findbugs.util.ClassName; +import static edu.umd.cs.findbugs.util.Util.isGeneratedCodeInCatchBlockViaLineNumber; + /** * RuntimeExceptionCapture * @@ -88,6 +95,8 @@ private static class ExceptionCaught { public boolean dead = false; + public boolean rethrown = false; + public ExceptionCaught(String exceptionClass, int startOffset, int endOffset, int sourcePC) { this.exceptionClass = exceptionClass; this.startOffset = startOffset; @@ -121,6 +130,10 @@ public void visitJavaClass(JavaClass c) { @Override public void visitAfter(Code obj) { for (ExceptionCaught caughtException : catchList) { + if (caughtException.rethrown) { + continue; + } + Set thrownSet = new HashSet<>(); for (ExceptionThrown thrownException : throwList) { if (thrownException.offset >= caughtException.startOffset && thrownException.offset < caughtException.endOffset) { @@ -130,38 +143,41 @@ public void visitAfter(Code obj) { } } } - int catchClauses = 0; + + int priority = caughtException.dead ? NORMAL_PRIORITY : LOW_PRIORITY; if ("java.lang.Exception".equals(caughtException.exceptionClass) && !caughtException.seen) { // Now we have a case where Exception is caught, but not thrown boolean rteCaught = false; for (ExceptionCaught otherException : catchList) { if (otherException.startOffset == caughtException.startOffset && otherException.endOffset == caughtException.endOffset) { - catchClauses++; if ("java.lang.RuntimeException".equals(otherException.exceptionClass)) { rteCaught = true; } } } - int range = caughtException.endOffset - caughtException.startOffset; + if (!rteCaught) { - int priority = LOW_PRIORITY + 1; - if (range > 300) { - priority--; - } else if (range < 30) { - priority++; - } - if (catchClauses > 1) { - priority++; - } - if (thrownSet.size() > 1) { - priority--; - } - if (caughtException.dead) { - priority--; - } accumulator.accumulateBug(new BugInstance(this, "REC_CATCH_EXCEPTION", priority).addClassAndMethod(this), SourceLineAnnotation.fromVisitedInstruction(getClassContext(), this, caughtException.sourcePC)); + } else { + accumulator.accumulateBug(new BugInstance(this, "REC2_CATCH_EXCEPTION", LOW_PRIORITY).addClassAndMethod(this), + SourceLineAnnotation.fromVisitedInstruction(getClassContext(), this, caughtException.sourcePC)); + } + } else if ("java.lang.RuntimeException".equals(caughtException.exceptionClass) && !caughtException.seen) { + accumulator.accumulateBug(new BugInstance(this, "REC2_CATCH_RUNTIME_EXCEPTION", LOW_PRIORITY).addClassAndMethod(this), + SourceLineAnnotation.fromVisitedInstruction(getClassContext(), this, caughtException.sourcePC)); + } else if ("java.lang.Throwable".equals(caughtException.exceptionClass) && !caughtException.seen) { + Method method = getMethod(); + ConstantPool cp = method.getConstantPool(); + LineNumberTable lineNumberTable = method.getLineNumberTable(); + int line = lineNumberTable.getSourceLine(caughtException.sourcePC); + InstructionList list = new InstructionList(method.getCode().getCode()); + + if (!method.isSynthetic() && + !isGeneratedCodeInCatchBlockViaLineNumber(cp, lineNumberTable, line, list, Arrays.asList(obj.getExceptionTable()))) { + accumulator.accumulateBug(new BugInstance(this, "REC2_CATCH_THROWABLE", LOW_PRIORITY).addClassAndMethod(this), + SourceLineAnnotation.fromVisitedInstruction(getClassContext(), this, caughtException.sourcePC)); } } } @@ -200,8 +216,20 @@ public void visit(CodeException obj) { System.out.println("Dead exception store at " + first); } caughtException.dead = true; - break; } + + // See if the excpetion is rethrown at the end of the handler. + while (block != null) { + InstructionHandle last = block.getLastInstruction(); + if (last != null) { + if (last.getInstruction().getOpcode() == Const.ATHROW) { + caughtException.rethrown = true; + break; + } + } + block = cfg.getSuccessorWithEdgeType(block, EdgeTypes.FALL_THROUGH_EDGE); + } + break; } } } catch (MethodUnprofitableException e) { diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/util/Util.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/util/Util.java index 114a9be1598..82c91227844 100644 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/util/Util.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/util/Util.java @@ -44,6 +44,7 @@ import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; import java.util.zip.ZipFile; import javax.annotation.Nonnull; @@ -51,7 +52,19 @@ import javax.annotation.WillCloseWhenClosed; import javax.annotation.WillNotClose; +import org.apache.bcel.Const; +import org.apache.bcel.classfile.CodeException; +import org.apache.bcel.classfile.ConstantPool; +import org.apache.bcel.classfile.LineNumberTable; +import org.apache.bcel.generic.ATHROW; +import org.apache.bcel.generic.ConstantPoolGen; +import org.apache.bcel.generic.INVOKEINTERFACE; +import org.apache.bcel.generic.INVOKEVIRTUAL; +import org.apache.bcel.generic.InstructionHandle; +import org.apache.bcel.generic.InstructionList; + import edu.umd.cs.findbugs.SystemProperties; +import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import edu.umd.cs.findbugs.charsets.UTF8; @@ -344,4 +357,111 @@ public static boolean isPowerOfTwo(int i) { && (i | (i - 1)) + 1 == 2 * i; } + /** + * 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 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 + */ + public static boolean isGeneratedCodeInCatchBlockViaLineNumber(@NonNull ConstantPool cp, @NonNull LineNumberTable lineNumberTable, + int line, @NonNull InstructionList instructions, @NonNull List 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 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; + } + } diff --git a/spotbugsTestCases/src/java/REC2Test.java b/spotbugsTestCases/src/java/REC2Test.java new file mode 100755 index 00000000000..3f2474a58f6 --- /dev/null +++ b/spotbugsTestCases/src/java/REC2Test.java @@ -0,0 +1,116 @@ +import java.io.IOException; + +/** + * RECTest + * + * @author Adam Balogh + */ +public class REC2Test { + public static Exception anException = new IOException(); + + public static void staticThrowsException() throws Exception { + throw new Exception(); + } + + public static void staticThrowsIOException() throws IOException { + throw new IOException(); + } + + public void throwsNothing() { + } + + public void throwsException() throws Exception { + throw new Exception(); + } + + public void throwsIOException() throws IOException { + throw new IOException(); + } + + public void throwsTwoExceptions() throws IOException, ClassNotFoundException { + throw new IOException(); + } + + private void dontTriggerEmptyExceptionHandler() { + } + + private class Resource implements AutoCloseable { + @Override + public void close() throws IOException { + } + } + + private Resource getResource() throws IOException { + return new Resource(); + } + + // should fail -- catches both Exception and RuntimeException + public void testFail() { + try { + for (int i = 0; i < 1000; i++) + for (int j = i; j < 1000; j++) + throwsNothing(); + throwsIOException(); + } catch (RuntimeException e) { + dontTriggerEmptyExceptionHandler(); + } catch (Exception e) { + dontTriggerEmptyExceptionHandler(); + } + } + + // should fail -- catches RuntimeException + public void testFail2() { + try { + for (int i = 0; i < 1000; i++) + for (int j = i; j < 1000; j++) + throwsNothing(); + } catch (RuntimeException e) { + dontTriggerEmptyExceptionHandler(); + } + } + + // should fail -- catches Throwable + public void testFail3() { + try { + for (int i = 0; i < 1000; i++) + for (int j = i; j < 1000; j++) + throwsNothing(); + throwsIOException(); + } catch (Throwable t) { + dontTriggerEmptyExceptionHandler(); + } + } + + // should pass -- rethrows both Exception and RuntimeException + public void testPass() { + try { + for (int i = 0; i < 1000; i++) + for (int j = i; j < 1000; j++) + throwsNothing(); + throwsIOException(); + } catch (RuntimeException e) { + throw e; + } catch (Exception e) { + throw new RuntimeException(e); + } + } + + // should pass -- rethrows Throwable + public void testPass2() { + try { + for (int i = 0; i < 1000; i++) + for (int j = i; j < 1000; j++) + throwsNothing(); + throwsIOException(); + } catch (Throwable t) { + throw new RuntimeException(t); + } + } + + // should pass -- The try-with-resources construct catches Throwable but it + // is skipped in the detector. + public void testPass3() throws IOException { + try (Resource res = getResource()) { + } + } +} diff --git a/spotbugsTestCases/src/java/RECTest.java b/spotbugsTestCases/src/java/RECTest.java index 0389c9fae51..3ce4ffa087c 100755 --- a/spotbugsTestCases/src/java/RECTest.java +++ b/spotbugsTestCases/src/java/RECTest.java @@ -110,6 +110,21 @@ public void testFail6() { } } + // should fail with LOW priority -- catches E, but E not thrown, however e + //// is used in the handler + public void testFail7() { + try { + for (int i = 0; i < 1000; i++) + for (int j = i; j < 1000; j++) + throwsNothing(); + for (int i = 0; i < 1000; i++) + for (int j = i; j < 1000; j++) + throwsNothing(); + } catch (Exception e) { + e.printStackTrace(); + } + } + // should pass -- catches E, but E thrown public void testPass() { try { @@ -171,4 +186,32 @@ public void testPass5() { dontTriggerEmptyExceptionHandler(); } } + + // should pass -- catches E, but rethrows it + public void testPass6() { + try { + for (int i = 0; i < 1000; i++) + for (int j = i; j < 1000; j++) + throwsNothing(); + for (int i = 0; i < 1000; i++) + for (int j = i; j < 1000; j++) + throwsNothing(); + } catch (Exception e) { + throw e; + } + } + + // should pass -- catches E, but rethrows it nested into a RuntimeException + public void testPass7() { + try { + for (int i = 0; i < 1000; i++) + for (int j = i; j < 1000; j++) + throwsNothing(); + for (int i = 0; i < 1000; i++) + for (int j = i; j < 1000; j++) + throwsNothing(); + } catch (Exception e) { + throw new RuntimeException(e); + } + } } diff --git a/spotbugsTestCases/src/java/lambdas/Issue60.java b/spotbugsTestCases/src/java/lambdas/Issue60.java index 0033b1f8c7f..5b1fa968826 100644 --- a/spotbugsTestCases/src/java/lambdas/Issue60.java +++ b/spotbugsTestCases/src/java/lambdas/Issue60.java @@ -20,5 +20,4 @@ public Stream keys() { return Stream. of() .flatMap(p -> p.stringPropertyNames().stream()); } - } From 7b0d8382230ded583db874c4f0f0900a5f8505c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81d=C3=A1m=20Balogh?= Date: Mon, 25 Jul 2022 17:21:36 +0200 Subject: [PATCH 2/9] Catching RuntimeException is ignored when the called method may be the method of a class which is the generic parameter of the current method or the current class. --- .../detect/RuntimeExceptionCapture2Test.java | 30 ++++---- .../detect/RuntimeExceptionCapture.java | 68 ++++++++++++++++--- spotbugsTestCases/src/java/REC2Test.java | 26 +++++++ 3 files changed, 104 insertions(+), 20 deletions(-) diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java index 681f117b8db..288ac257211 100644 --- a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java +++ b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java @@ -15,17 +15,23 @@ 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"); + performAnalysis("REC2Test.class", + "REC2Test$Resource.class", + "REC2Test$TestPass5.class", + "REC2Test$Unknown.class", + "REC2Test$1.class"); + + fail("testFail", 58, "REC2_CATCH_RUNTIME_EXCEPTION", Confidence.LOW); + fail("testFail", 60, "REC2_CATCH_EXCEPTION", Confidence.LOW); + fail("testFail2", 71, "REC2_CATCH_RUNTIME_EXCEPTION", Confidence.LOW); + fail("testFail3", 83, "REC2_CATCH_THROWABLE", Confidence.LOW); + + pass("REC2Test", "testPass", "REC2_CATCH_RUNTIME_EXCEPTION"); + pass("REC2Test", "testPass", "REC2_CATCH_EXCEPTION"); + pass("REC2Test", "testPass2", "REC2_CATCH_THROWABLE"); + pass("REC2Test", "testPass3", "REC2_CATCH_THROWABLE"); + pass("REC2Test", "testPass4", "REC2_CATCH_RUNTIME_EXCEPTION"); + pass("REC2Test$TestPass5", "testPass5", "REC2_CATCH_RUNTIME_EXCEPTION"); } private void fail(String methodName, int line, String exceptionName, Confidence confidence) { @@ -40,7 +46,7 @@ private void fail(String methodName, int line, String exceptionName, Confidence assertThat(getBugCollection(), hasItem(bugInstanceMatcher)); } - private void pass(String methodName, String exceptionName) { + private void pass(String className, String methodName, String exceptionName) { countBugs(methodName, exceptionName, 0); } diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture.java index 90069955ea2..cea69fd484a 100755 --- a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture.java +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture.java @@ -26,6 +26,7 @@ import java.util.Collection; import java.util.HashSet; import java.util.List; +import java.util.Optional; import java.util.Set; import org.apache.bcel.Const; @@ -35,9 +36,11 @@ import org.apache.bcel.classfile.JavaClass; import org.apache.bcel.classfile.LineNumberTable; import org.apache.bcel.classfile.Method; +import org.apache.bcel.classfile.Signature; import org.apache.bcel.generic.ASTORE; import org.apache.bcel.generic.InstructionHandle; import org.apache.bcel.generic.InstructionList; +import org.apache.commons.lang3.StringUtils; import edu.umd.cs.findbugs.BugAccumulator; import edu.umd.cs.findbugs.BugInstance; @@ -84,6 +87,8 @@ public class RuntimeExceptionCapture extends OpcodeStackDetector implements Stat private final List throwList = new ArrayList<>(); + private boolean invokesMethodOfGenericParameter = false; + private final BugAccumulator accumulator; private static class ExceptionCaught { @@ -164,10 +169,12 @@ public void visitAfter(Code obj) { accumulator.accumulateBug(new BugInstance(this, "REC2_CATCH_EXCEPTION", LOW_PRIORITY).addClassAndMethod(this), SourceLineAnnotation.fromVisitedInstruction(getClassContext(), this, caughtException.sourcePC)); } - } else if ("java.lang.RuntimeException".equals(caughtException.exceptionClass) && !caughtException.seen) { + } else if (!invokesMethodOfGenericParameter && "java.lang.RuntimeException".equals(caughtException.exceptionClass) && + !caughtException.seen) { accumulator.accumulateBug(new BugInstance(this, "REC2_CATCH_RUNTIME_EXCEPTION", LOW_PRIORITY).addClassAndMethod(this), SourceLineAnnotation.fromVisitedInstruction(getClassContext(), this, caughtException.sourcePC)); - } else if ("java.lang.Throwable".equals(caughtException.exceptionClass) && !caughtException.seen) { + } else if ("java.lang.Throwable".equals(caughtException.exceptionClass) && + !caughtException.seen) { Method method = getMethod(); ConstantPool cp = method.getConstantPool(); LineNumberTable lineNumberTable = method.getLineNumberTable(); @@ -183,6 +190,7 @@ public void visitAfter(Code obj) { } catchList.clear(); throwList.clear(); + invokesMethodOfGenericParameter = false; } @Override @@ -218,14 +226,12 @@ public void visit(CodeException obj) { caughtException.dead = true; } - // See if the excpetion is rethrown at the end of the handler. + // See if the exception is rethrown at the end of the handler. while (block != null) { InstructionHandle last = block.getLastInstruction(); - if (last != null) { - if (last.getInstruction().getOpcode() == Const.ATHROW) { - caughtException.rethrown = true; - break; - } + if (last != null && last.getInstruction().getOpcode() == Const.ATHROW) { + caughtException.rethrown = true; + break; } block = cfg.getSuccessorWithEdgeType(block, EdgeTypes.FALL_THROUGH_EDGE); } @@ -264,7 +270,18 @@ public void sawOpcode(int seen) { case Const.INVOKEVIRTUAL: case Const.INVOKESPECIAL: case Const.INVOKESTATIC: + case Const.INVOKEINTERFACE: String className = getClassConstantOperand(); + if (seen == Const.INVOKEINTERFACE) { + OpcodeStack.Item object = getStack().getStackItem(getStack().getStackDepth() - 1); + Optional classSig = Arrays.stream(getThisClass().getAttributes()) + .filter(attr -> attr instanceof Signature) + .map(attr -> (Signature) attr) + .findAny(); + if (maybeGenericParameter(object.getSignature(), getMethod().getGenericSignature(), classSig)) { + invokesMethodOfGenericParameter = true; + } + } if (!className.startsWith("[")) { try { XClass c = Global.getAnalysisCache().getClassAnalysis(XClass.class, @@ -293,4 +310,39 @@ public void sawOpcode(int seen) { } + private boolean maybeGenericParameter(String signature, String genericSignature, Optional classSig) { + List genericParameters = extractGenericParameters(genericSignature); + if (classSig.isPresent()) { + genericParameters.addAll(extractGenericParameters(classSig.get().getSignature())); + } + + for (String genParam : genericParameters) { + int doubleColonIdx = genParam.indexOf("::"); + if (doubleColonIdx == -1) { + continue; + } + if (signature.equals(genParam.substring(doubleColonIdx + 2))) { + return true; + } + } + return false; + } + + private List extractGenericParameters(String genericSignature) { + List result = new ArrayList<>(); + String params = StringUtils.substringBetween(genericSignature, "<", ">"); + if (params != null) { + int last = 0; + while (true) { + int newLast = params.indexOf(';', last) + 1; + if (newLast == 0) { + break; + } + result.add(params.substring(last, newLast)); + last = newLast; + } + } + return result; + } + } diff --git a/spotbugsTestCases/src/java/REC2Test.java b/spotbugsTestCases/src/java/REC2Test.java index 3f2474a58f6..ca0efa598fc 100755 --- a/spotbugsTestCases/src/java/REC2Test.java +++ b/spotbugsTestCases/src/java/REC2Test.java @@ -44,6 +44,10 @@ private Resource getResource() throws IOException { return new Resource(); } + private interface Unknown { + void doesSomething(); + } + // should fail -- catches both Exception and RuntimeException public void testFail() { try { @@ -113,4 +117,26 @@ public void testPass3() throws IOException { try (Resource res = getResource()) { } } + + // should pass -- Invokation of an unknown method may require catching of + // `RutimeException`, `Exception` or `Throwable`. + public void testPass4(U u) { + try { + u.doesSomething(); + } catch (RuntimeException e) { + dontTriggerEmptyExceptionHandler(); + } + } + + public class TestPass5 { + // should pass -- Invokation of an unknown method may require catching of + // `RutimeException`, `Exception` or `Throwable`. + public void testPass5(U u) { + try { + u.doesSomething(); + } catch (RuntimeException e) { + dontTriggerEmptyExceptionHandler(); + } + } + } } From f3921292d41262c722cfa5fc8da0cad659ecade3 Mon Sep 17 00:00:00 2001 From: baloghadamsoftware Date: Thu, 27 Apr 2023 15:10:33 +0200 Subject: [PATCH 3/9] CHANGELOG updated --- CHANGELOG.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9764f8a7bbf..3ffec975ab3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2. ### Added * New simple name-based AnnotationMatcher for exclude files (now bug annotations store the class java annotations in an attribute called `classAnnotationNames`). For example, use like in an excludeFilter.xml to ignore classes generated by the Immutable framework. This ignores all class, method or field bugs in classes with that annotation. +* 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) ### Security - Disable access to external entities when processing XML ([#2217](https://github.com/spotbugs/spotbugs/pull/2217)) @@ -58,9 +59,6 @@ Currently the versioning policy of this project follows [Semantic Versioning v2. - Fixed false positives `EI_EXPOSE_REP` thrown in case of fields initialized by the `of` or `copyOf` method of a `List`, `Map` or `Set` ([#1771](https://github.com/spotbugs/spotbugs/issues/1771)) - Fixed CFGBuilderException thrown when `dup_x2` is used to swap the reference and wide-value (double, long) in the stack ([#2146](https://github.com/spotbugs/spotbugs/pull/2146)) -### 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)) From 4cb638fdc2bbe652471f2b1f53b151e4fd6238c0 Mon Sep 17 00:00:00 2001 From: Jeremy Landis Date: Sun, 27 Aug 2023 14:47:35 -0400 Subject: [PATCH 4/9] Update messages.xml --- spotbugs/etc/messages.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spotbugs/etc/messages.xml b/spotbugs/etc/messages.xml index 6241dce5b3d..915e6193f81 100644 --- a/spotbugs/etc/messages.xml +++ b/spotbugs/etc/messages.xml @@ -7005,7 +7005,7 @@ does not need to be created, just access the static methods directly using the c 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 + 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.

@@ -7049,7 +7049,7 @@ does not need to be created, just access the static methods directly using the c 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 + 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.

From 2d8bb128cf28ddc0f160a213d29d836820f2a84e Mon Sep 17 00:00:00 2001 From: Jeremy Landis Date: Sun, 27 Aug 2023 14:48:35 -0400 Subject: [PATCH 5/9] Update RECTest.java --- spotbugsTestCases/src/java/RECTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spotbugsTestCases/src/java/RECTest.java b/spotbugsTestCases/src/java/RECTest.java index 3ce4ffa087c..a2f29ae462b 100755 --- a/spotbugsTestCases/src/java/RECTest.java +++ b/spotbugsTestCases/src/java/RECTest.java @@ -111,7 +111,7 @@ public void testFail6() { } // should fail with LOW priority -- catches E, but E not thrown, however e - //// is used in the handler + // is used in the handler public void testFail7() { try { for (int i = 0; i < 1000; i++) From 484599d035a76b3179e78d2525bc5b3d4605c8bb Mon Sep 17 00:00:00 2001 From: Jeremy Landis Date: Sun, 27 Aug 2023 14:51:50 -0400 Subject: [PATCH 6/9] Update RECTest.java --- spotbugsTestCases/src/java/RECTest.java | 108 ++++++++++++++++-------- 1 file changed, 72 insertions(+), 36 deletions(-) diff --git a/spotbugsTestCases/src/java/RECTest.java b/spotbugsTestCases/src/java/RECTest.java index a2f29ae462b..3cfc3444d61 100755 --- a/spotbugsTestCases/src/java/RECTest.java +++ b/spotbugsTestCases/src/java/RECTest.java @@ -37,12 +37,16 @@ private void dontTriggerEmptyExceptionHandler() { // should fail -- catches E, but E not thrown public void testFail() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + } + } + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } } catch (Exception e) { dontTriggerEmptyExceptionHandler(); } @@ -51,9 +55,11 @@ public void testFail() { // should fail -- catches E, but E not thrown public void testFail2() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } throw new IOException(); } catch (Exception e) { dontTriggerEmptyExceptionHandler(); @@ -63,9 +69,11 @@ public void testFail2() { // should fail -- catches E, but E not thrown public void testFail3() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } IOException e = new IOException(); throw e; } catch (Exception e) { @@ -76,9 +84,11 @@ public void testFail3() { // should fail -- catches E, but E not thrown public void testFail4() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } throwsIOException(); } catch (Exception e) { dontTriggerEmptyExceptionHandler(); @@ -88,9 +98,11 @@ public void testFail4() { // should fail -- catches E, but E not thrown public void testFail5() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } staticThrowsIOException(); } catch (Exception e) { dontTriggerEmptyExceptionHandler(); @@ -101,9 +113,11 @@ public void testFail5() { // but E not thrown public void testFail6() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } throwsTwoExceptions(); } catch (Exception e) { dontTriggerEmptyExceptionHandler(); @@ -114,12 +128,16 @@ public void testFail6() { // is used in the handler public void testFail7() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + } + } + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } } catch (Exception e) { e.printStackTrace(); } @@ -128,9 +146,11 @@ public void testFail7() { // should pass -- catches E, but E thrown public void testPass() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } throw new Exception(); } catch (Exception e) { dontTriggerEmptyExceptionHandler(); @@ -140,9 +160,11 @@ public void testPass() { // should pass -- catches E, but E thrown indirectly public void testPass2() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } throw anException; } catch (Exception e) { dontTriggerEmptyExceptionHandler(); @@ -152,9 +174,11 @@ public void testPass2() { // should pass -- catches E, but E thrown by method public void testPass3() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } throwsException(); } catch (Exception e) { dontTriggerEmptyExceptionHandler(); @@ -164,9 +188,11 @@ public void testPass3() { // should pass -- catches E, but E thrown by static method public void testPass4() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } staticThrowsException(); } catch (Exception e) { dontTriggerEmptyExceptionHandler(); @@ -176,9 +202,11 @@ public void testPass4() { // should pass -- catches E, but RuntimeException caught first public void testPass5() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } throwsIOException(); } catch (RuntimeException e) { dontTriggerEmptyExceptionHandler(); @@ -190,12 +218,16 @@ public void testPass5() { // should pass -- catches E, but rethrows it public void testPass6() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + } + } + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } } catch (Exception e) { throw e; } @@ -204,12 +236,16 @@ public void testPass6() { // should pass -- catches E, but rethrows it nested into a RuntimeException public void testPass7() { try { - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); - for (int i = 0; i < 1000; i++) - for (int j = i; j < 1000; j++) + } + } + for (int i = 0; i < 1000; i++) { + for (int j = i; j < 1000; j++) { throwsNothing(); + } + } } catch (Exception e) { throw new RuntimeException(e); } From 2ee239b746429da6a9cd75979bfe9678738b722e Mon Sep 17 00:00:00 2001 From: Judit Knoll Date: Thu, 12 Oct 2023 17:35:07 +0200 Subject: [PATCH 7/9] Merge fix --- .../cs/findbugs/detect/RuntimeExceptionCapture2Test.java | 6 +++--- .../umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java index 288ac257211..6e87b34c803 100644 --- a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java +++ b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java @@ -3,7 +3,7 @@ import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.MatcherAssert.assertThat; -import org.junit.Test; +import org.junit.jupiter.api.Test; import edu.umd.cs.findbugs.AbstractIntegrationTest; import edu.umd.cs.findbugs.annotations.Confidence; @@ -12,9 +12,9 @@ import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly; -public class RuntimeExceptionCapture2Test extends AbstractIntegrationTest { +class RuntimeExceptionCapture2Test extends AbstractIntegrationTest { @Test - public void test() { + void test() { performAnalysis("REC2Test.class", "REC2Test$Resource.class", "REC2Test$TestPass5.class", diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java index dfcb033a452..01885059170 100644 --- a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java +++ b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java @@ -3,7 +3,7 @@ import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.MatcherAssert.assertThat; -import org.junit.Test; +import org.junit.jupiter.api.Test; import edu.umd.cs.findbugs.AbstractIntegrationTest; import edu.umd.cs.findbugs.annotations.Confidence; @@ -12,9 +12,9 @@ import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly; -public class RuntimeExceptionCaptureTest extends AbstractIntegrationTest { +class RuntimeExceptionCaptureTest extends AbstractIntegrationTest { @Test - public void test() { + void test() { performAnalysis("RECTest.class"); fail("testFail", 46, Confidence.MEDIUM); From d27f18e4efb18fcdea6edcbf439b15956201274e Mon Sep 17 00:00:00 2001 From: Judit Knoll Date: Thu, 12 Oct 2023 17:40:44 +0200 Subject: [PATCH 8/9] Fix line numbers in tests --- .../detect/RuntimeExceptionCaptureTest.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java index 01885059170..18697c1e7a9 100644 --- a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java +++ b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCaptureTest.java @@ -17,13 +17,13 @@ class RuntimeExceptionCaptureTest extends AbstractIntegrationTest { 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); + fail("testFail", 50, Confidence.MEDIUM); + fail("testFail2", 64, Confidence.MEDIUM); + fail("testFail3", 79, Confidence.MEDIUM); + fail("testFail4", 93, Confidence.MEDIUM); + fail("testFail5", 107, Confidence.MEDIUM); + fail("testFail6", 122, Confidence.MEDIUM); + fail("testFail7", 141, Confidence.LOW); pass("testPass"); pass("testPass2"); From 262d9ee9008e2aca7bb223fa45d14ed0e2fcb26c Mon Sep 17 00:00:00 2001 From: Judit Knoll Date: Fri, 13 Oct 2023 09:21:31 +0200 Subject: [PATCH 9/9] Add classname to tests --- .../cs/findbugs/detect/RuntimeExceptionCapture2Test.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java index 6e87b34c803..678a24b3ca5 100644 --- a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java +++ b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/RuntimeExceptionCapture2Test.java @@ -35,7 +35,7 @@ void test() { } private void fail(String methodName, int line, String exceptionName, Confidence confidence) { - countBugs(methodName, exceptionName, 1); + countBugs("REC2Test", methodName, exceptionName, 1); final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder() .bugType(exceptionName) .inClass("REC2Test") @@ -47,13 +47,13 @@ private void fail(String methodName, int line, String exceptionName, Confidence } private void pass(String className, String methodName, String exceptionName) { - countBugs(methodName, exceptionName, 0); + countBugs(className, methodName, exceptionName, 0); } - private void countBugs(String methodName, String exceptionName, int count) { + private void countBugs(String className, String methodName, String exceptionName, int count) { BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder() .bugType(exceptionName) - .inClass("REC2Test") + .inClass(className) .inMethod(methodName) .build(); assertThat(getBugCollection(), containsExactly(count, bugTypeMatcher));