From 818b14cee799caffc9cc0ceefa560e24b82921ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Orosz=20B=C3=A1lint?= <64847175+oroszbd@users.noreply.github.com> Date: Sat, 26 Mar 2022 05:51:16 +0100 Subject: [PATCH] Added new detector for rule ERR07 (#1956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Added detector and tests for ThrowingExceptions * Issue893 - removed throws clause * Issue1338 - specialized throws clause * Ran gradlew spotlessApply * docs: move CHANGELOG entry under the "Unreleased" Co-authored-by: Bálint Dominik Orosz Co-authored-by: Kengo TODA Co-authored-by: Kengo TODA --- CHANGELOG.md | 8 +++ .../detect/CheckThrowingExceptions.java | 49 +++++++++++++++ spotbugs/etc/findbugs.xml | 5 ++ spotbugs/etc/messages.xml | 60 +++++++++++++++++++ .../findbugs/detect/ThrowingExceptions.java | 51 ++++++++++++++++ .../src/java/MethodsThrowingExceptions.java | 39 ++++++++++++ .../src/java/ghIssues/Issue1338.java | 4 +- .../src/java/ghIssues/Issue893.java | 2 +- 8 files changed, 216 insertions(+), 2 deletions(-) create mode 100644 spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/CheckThrowingExceptions.java create mode 100644 spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ThrowingExceptions.java create mode 100644 spotbugsTestCases/src/java/MethodsThrowingExceptions.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 60f59413046..54ae7902b2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ Currently the versioning policy of this project follows [Semantic Versioning v2. - Bumped Saxon-HE from 10.6 to 11.2 ([#1955](https://github.com/spotbugs/spotbugs/pull/1955)) - Fixed traversal of nested archives governed by `-nested:true` ([#1930](https://github.com/spotbugs/spotbugs/pull/1930)) +### Added +* New detector `ThrowingExceptions` and introduced new bug types: + * `THROWS_METHOD_THROWS_RUNTIMEEXCEPTION` is reported in case of a method throwing RuntimeException, + * `THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION` is reported when a method has Exception in its throws clause and + * `THROWS_METHOD_THROWS_CLAUSE_THROWABLE` is reported when a method has Throwable in its throws clause + +(See [SEI CERT ERR07-J](https://wiki.sei.cmu.edu/confluence/display/java/ERR07-J.+Do+not+throw+RuntimeException%2C+Exception%2C+or+Throwable)) + ## 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/CheckThrowingExceptions.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/CheckThrowingExceptions.java new file mode 100644 index 00000000000..6ab725587cb --- /dev/null +++ b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/CheckThrowingExceptions.java @@ -0,0 +1,49 @@ +package edu.umd.cs.findbugs.detect; + +import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly; +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.test.matcher.BugInstanceMatcher; +import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder; + +public class CheckThrowingExceptions extends AbstractIntegrationTest { + @Test + public void throwingExceptionsTests() { + performAnalysis("MethodsThrowingExceptions.class"); + + assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", 1); + assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", 1); + assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", 1); + + assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", 1, "MethodsThrowingExceptions", "isCapitalizedThrowingRuntimeException"); + assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", 0, "MethodsThrowingExceptions", "isCapitalizedThrowingSpecializedException"); + + assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", 1, "MethodsThrowingExceptions", "methodThrowingBasicException"); + assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", 0, "MethodsThrowingExceptions", "methodThrowingIOException"); + + assertNumOfTHROWSBugs("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", 1, "MethodsThrowingExceptions", "methodThrowingThrowable"); + } + + private void assertNumOfTHROWSBugs(String bugType, int num) { + final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder() + .bugType(bugType) + .build(); + assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher)); + } + + private void assertNumOfTHROWSBugs(String bugType, int num, String className, String method) { + final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder() + .bugType(bugType) + .inClass(className) + .inMethod(method) + .build(); + if (num > 0) { + assertThat(getBugCollection(), hasItem(bugTypeMatcher)); + } + assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher)); + } +} diff --git a/spotbugs/etc/findbugs.xml b/spotbugs/etc/findbugs.xml index afe4a1793e8..632952e4d4a 100644 --- a/spotbugs/etc/findbugs.xml +++ b/spotbugs/etc/findbugs.xml @@ -659,6 +659,8 @@ reports="MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR,MC_OVERRIDABLE_METHOD_CALL_IN_CLONE"/> + @@ -1271,5 +1273,8 @@ + + + diff --git a/spotbugs/etc/messages.xml b/spotbugs/etc/messages.xml index 7f337c1e0ae..7822b4d8bbd 100644 --- a/spotbugs/etc/messages.xml +++ b/spotbugs/etc/messages.xml @@ -710,6 +710,14 @@ boolean operators (| and & instead of This detector looks for violations of the idioms for writing cloneable classes.

+]]> + + + +
+ This detector looks for methods throwing RuntimeException and methods +that have Exception or Throwable in their throws clause.

]]>
@@ -8653,7 +8661,58 @@ after the call to initLogging, the logger configuration is lost

]]> + + Method intentionally throws RuntimeException. + Method intentionally throws RuntimeException. +
+ + Method intentionally throws RuntimeException.
+ + According to the SEI CERT ERR07-J rule, + throwing a RuntimeException may cause errors, like the caller not being able to examine the exception and therefore cannot properly recover from it.
+ + Moreover, throwing a RuntimeException would force the caller to catch RuntimeException and therefore violate the + SEI CERT ERR08-J rule.
+ + Please note that you can derive from Exception or RuntimeException and may throw a new instance of that exception. +

]]> +
+ + + Method lists Exception in its throws clause. + Method lists Exception in its throws clause. +
+ + Method lists Exception in its throws clause.
+ When declaring a method, the types of exceptions in the throws clause should be the most specific. + Therefore, using Exception in the throws clause would force the caller to either use it in its own throws clause, or use it in a try-catch block (when it does not necessarily + contain any meaningful information about the thrown exception).

+ + For more information, see the SEI CERT ERR07-J rule. +

]]> +
+ + + Method lists Throwable in its throws clause. + Method lists Throwable in its throws clause. +
+ + Method lists Throwable in its throws clause.
+ + When declaring a method, the types of exceptions in the throws clause should be the most specific. + Therefore, using Throwable in the throws clause would force the caller to either use it in its own throws clause, or use it in a try-catch block (when it does not necessarily + contain any meaningful information about the thrown exception).
+ + Furthermore, using Throwable like that is semantically a bad practice, considered that Throwables include Errors as well, but by definition they occur in unrecoverable scenarios.

+ + For more information, see the SEI CERT ERR07-J rule. +

]]> +
+ Custom class loader does not call its superclass's getPermissions() Custom class loader {1} does not call its superclass's getPermissions() @@ -8818,5 +8877,6 @@ after the call to initLogging, the logger configuration is lost Reflection increasing accessibility of fields Dangerous call to overridable method Do not use an instance lock to protect shared static data + Exception throwing related problems Custom class loader does not call its superclass's getPermissions() diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ThrowingExceptions.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ThrowingExceptions.java new file mode 100644 index 00000000000..9811431fc6f --- /dev/null +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ThrowingExceptions.java @@ -0,0 +1,51 @@ +package edu.umd.cs.findbugs.detect; + +import edu.umd.cs.findbugs.BugInstance; +import edu.umd.cs.findbugs.BugReporter; +import edu.umd.cs.findbugs.OpcodeStack; +import edu.umd.cs.findbugs.ba.XMethod; +import edu.umd.cs.findbugs.bcel.OpcodeStackDetector; + +import org.apache.bcel.Const; +import org.apache.bcel.classfile.ExceptionTable; +import org.apache.bcel.classfile.Method; + + +public class ThrowingExceptions extends OpcodeStackDetector { + private final BugReporter bugReporter; + + public ThrowingExceptions(BugReporter bugReporter) { + this.bugReporter = bugReporter; + } + + @Override + public void visit(Method obj) { + ExceptionTable exceptionTable = obj.getExceptionTable(); + if (exceptionTable != null) { + String[] exceptionNames = exceptionTable.getExceptionNames(); + for (String exception : exceptionNames) { + if ("java.lang.Exception".equals(exception)) { + reportBug("THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION", getXMethod()); + } else if ("java.lang.Throwable".equals(exception)) { + reportBug("THROWS_METHOD_THROWS_CLAUSE_THROWABLE", getXMethod()); + } + } + } + } + + @Override + public void sawOpcode(int seen) { + if (seen == Const.ATHROW) { + OpcodeStack.Item item = stack.getStackItem(0); + if (item != null) { + if ("Ljava/lang/RuntimeException;".equals(item.getSignature())) { + reportBug("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", getXMethod()); + } + } + } + } + + private void reportBug(String bugName, XMethod method) { + bugReporter.reportBug(new BugInstance(this, bugName, NORMAL_PRIORITY).addClass(this).addMethod(method)); + } +} diff --git a/spotbugsTestCases/src/java/MethodsThrowingExceptions.java b/spotbugsTestCases/src/java/MethodsThrowingExceptions.java new file mode 100644 index 00000000000..d239e0a1f77 --- /dev/null +++ b/spotbugsTestCases/src/java/MethodsThrowingExceptions.java @@ -0,0 +1,39 @@ +import java.io.IOException; + +public class MethodsThrowingExceptions { + boolean isCapitalizedThrowingRuntimeException(String s) { + if (s == null) { + throw new RuntimeException("Null String"); + } + if (s.equals("")) { + return true; + } + String first = s.substring(0, 1); + String rest = s.substring(1); + return first.equals(first.toUpperCase()) && rest.equals(rest.toLowerCase()); + } + + boolean isCapitalizedThrowingSpecializedException(String s) { + if (s == null) { + throw new NullPointerException(); + } + if (s.equals("")) { + return true; + } + String first = s.substring(0, 1); + String rest = s.substring(1); + return first.equals(first.toUpperCase()) && rest.equals(rest.toLowerCase()); + } + + private void methodThrowingBasicException() throws Exception { + System.out.println("Method throwing Exception"); + } + + private void methodThrowingIOException() throws IOException { + System.out.println("Method throwing IOException"); + } + + private void methodThrowingThrowable() throws Throwable { + System.out.println("Method throwing Throwable"); + } +} diff --git a/spotbugsTestCases/src/java/ghIssues/Issue1338.java b/spotbugsTestCases/src/java/ghIssues/Issue1338.java index bb8525aa7f9..3282a101725 100644 --- a/spotbugsTestCases/src/java/ghIssues/Issue1338.java +++ b/spotbugsTestCases/src/java/ghIssues/Issue1338.java @@ -1,12 +1,14 @@ package ghIssues; import java.io.InputStream; +import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.security.KeyStore; +import java.security.KeyStoreException; public class Issue1338 { - public static KeyStore loadKeyStore(final Path path) throws Exception { + public static KeyStore loadKeyStore(final Path path) throws IOException, KeyStoreException { try (InputStream inputStream = Files.newInputStream(path)) { // final KeyStore keyStore = KeyStore.getInstance("JKS"); // keyStore.load(inputStream, password.toCharArray()); diff --git a/spotbugsTestCases/src/java/ghIssues/Issue893.java b/spotbugsTestCases/src/java/ghIssues/Issue893.java index 7b622ccc0c0..349583881b9 100644 --- a/spotbugsTestCases/src/java/ghIssues/Issue893.java +++ b/spotbugsTestCases/src/java/ghIssues/Issue893.java @@ -1,7 +1,7 @@ package ghIssues; public class Issue893 { - public boolean test() throws Exception { + public boolean test() { Boolean b = Boolean.valueOf(true); // Line below should not throw IllegalArgumentException during analysis b &= Boolean.valueOf(false);