Merge branch 'master' into remove-securitymanager

CHANGELOG.md

@@ -5,10 +5,23 @@ 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 - 2022-??-??
+### Changed
+- Updated documentation by adding parenthesis `()` to the negative odd check message ([#1995](https://github.com/spotbugs/spotbugs/issues/1995))
+
 ### Fixed
-- Bumped Saxon-HE from 10.6 to 11.2 ([#1955](https://github.com/spotbugs/spotbugs/pull/1955))
+- Bumped Saxon-HE from 10.6 to 11.3 ([#1955](https://github.com/spotbugs/spotbugs/pull/1955), [#1999](https://github.com/spotbugs/spotbugs/pull/1999))
 - Fixed traversal of nested archives governed by `-nested:true` ([#1930](https://github.com/spotbugs/spotbugs/pull/1930))
 - Warnings of deprecated System::setSecurityManager calls on Java 17 ([#1983](https://github.com/spotbugs/spotbugs/pull/1983))
+- Fixed false positive SSD bug for locking on java.lang.Class objects ([#1978](https://github.com/spotbugs/spotbugs/issues/1978))
+- Bump ObjectWeb ASM from 9.2 to 9.3 supporting JDK 19 ([#2004](https://github.com/spotbugs/spotbugs/pull/2004))
+
+### 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

build.gradle

@@ -1,6 +1,6 @@
 plugins {
   id "org.sonarqube" version "3.3"
-  id "com.diffplug.spotless" version "6.3.0"
+  id "com.diffplug.spotless" version "6.4.2"
   id "org.gradle.crypto.checksum" version "1.4.0"
   id "com.github.spotbugs" version "5.0.6"
   id "io.github.gradle-nexus.publish-plugin" version "1.1.0"

buildSrc/build.gradle.kts

@@ -6,5 +6,5 @@ repositories {
     gradlePluginPortal()
 }
 dependencies {
-    implementation("com.diffplug.gradle:goomph:3.35.0")
+    implementation("com.diffplug.gradle:goomph:3.36.0")
 }

docs/filter.rst

@@ -31,7 +31,7 @@ Types of Match clauses
 This element specifies a particular bug ``pattern`` or ``patterns`` to match. The ``pattern`` attribute is a comma-separated list of bug pattern types.
 You can find the bug pattern types for particular warnings by looking at the output produced by the **-xml** output option (the type attribute of BugInstance elements), or from the :doc:`bugDescriptions`.
 
-For more coarse-grained matching, use ``code`` attribute. It takes a comma-separated list of bug abbreviations. For most-coarse grained matching use ``category`` attribute, that takes a comma separated list of bug category names: ``CORRECTNESS``, ``MT_CORRECTNESS``, ``BAD_PRACTICICE``, ``PERFORMANCE``, ``STYLE``.
+For more coarse-grained matching, use ``code`` attribute. It takes a comma-separated list of bug abbreviations. For most-coarse grained matching use ``category`` attribute, that takes a comma separated list of bug category names: ``CORRECTNESS``, ``MT_CORRECTNESS``, ``BAD_PRACTICE``, ``PERFORMANCE``, ``STYLE``.
 
 If more than one of the attributes mentioned above are specified on the same <Bug> element, all bug patterns that match either one of specified pattern names, or abbreviations, or categories will be matched.
 

settings.gradle

@@ -1,5 +1,5 @@
 plugins {
-  id "com.gradle.enterprise" version "3.8.1"
+  id "com.gradle.enterprise" version "3.9"
 }
 
 include ':eclipsePlugin'

spotbugs-tests/build.gradle

@@ -19,7 +19,7 @@ dependencies {
   implementation 'org.hamcrest:hamcrest-all:1.3'
   implementation 'org.apache.ant:ant:1.10.12'
   implementation "org.apache.logging.log4j:log4j-slf4j18-impl:$log4jVersion"
-  implementation 'com.google.errorprone:error_prone_annotations:2.11.0'
+  implementation 'com.google.errorprone:error_prone_annotations:2.12.1'
   implementation files(project(":spotbugs").sourceSets.gui.output)
 }
 

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

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

@@ -29,6 +29,13 @@ public void findSSDBugInClass_InstanceLevelSynchronizedMethod() {
         assertSSDBug("InstanceLevelSynchronizedMethod", "methodWithBug", 10);
     }
 
+    @Test
+    public void findNoSSDBugInClass_LockingOnJavaLangClassObject() {
+        performAnalysis("instanceLockOnSharedStaticData/LockingOnJavaLangClassObject.class");
+
+        assertNumOfSSDBugs(0);
+    }
+
     @Test
     public void findNoSSDBugInClass_StaticLockObjectOnStaticSharedData() {
         performAnalysis("instanceLockOnSharedStaticData/StaticLockObjectOnStaticSharedData.class");

spotbugs/build.gradle

@@ -23,7 +23,7 @@ configurations {
 }
 
 ext {
-  asmVersion = '9.2'
+  asmVersion = '9.3'
   log4jVersion = '2.17.2'
 }
 
@@ -87,7 +87,7 @@ dependencies {
   api 'org.apache.commons:commons-lang3:3.12.0'
   api 'org.apache.commons:commons-text:1.9'
   api 'org.slf4j:slf4j-api:1.8.0-beta4'
-  implementation 'net.sf.saxon:Saxon-HE:11.2'
+  implementation 'net.sf.saxon:Saxon-HE:11.3'
   logBinding ("org.apache.logging.log4j:log4j-slf4j18-impl:$log4jVersion") {
     exclude group: 'org.slf4j'
   }

spotbugs/etc/findbugs.xml

@@ -659,6 +659,8 @@
                     reports="MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR,MC_OVERRIDABLE_METHOD_CALL_IN_CLONE"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindInstanceLockOnSharedStaticData" speed="fast"
                     reports="SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA"/>
+          <Detector class="edu.umd.cs.findbugs.detect.ThrowingExceptions" speed="fast"
+                    reports="THROWS_METHOD_THROWS_RUNTIMEEXCEPTION,THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION,THROWS_METHOD_THROWS_CLAUSE_THROWABLE" />
           <Detector class="edu.umd.cs.findbugs.detect.PermissionsSuper"
                     reports="PERM_SUPER_NOT_CALLED_IN_GETPERMISSIONS"/>
             <!-- Bug Categories -->
@@ -1271,5 +1273,8 @@
           <BugPattern abbrev="MC" type="MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR" category="MALICIOUS_CODE" />
           <BugPattern abbrev="MC" type="MC_OVERRIDABLE_METHOD_CALL_IN_CLONE" category="MALICIOUS_CODE" />
           <BugPattern abbrev="SSD" type="SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA" category="CORRECTNESS" />
+          <BugPattern abbrev="THROWS" type="THROWS_METHOD_THROWS_RUNTIMEEXCEPTION" category="BAD_PRACTICE" />
+          <BugPattern abbrev="THROWS" type="THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION" category="BAD_PRACTICE" />
+          <BugPattern abbrev="THROWS" type="THROWS_METHOD_THROWS_CLAUSE_THROWABLE" category="BAD_PRACTICE" />
           <BugPattern abbrev="PERM" type="PERM_SUPER_NOT_CALLED_IN_GETPERMISSIONS" category="MALICIOUS_CODE" />
 </FindbugsPlugin>

spotbugs/etc/messages.xml

@@ -710,6 +710,14 @@ boolean operators (<code>|</code> and <code>&amp;</code> instead of
 <![CDATA[
 <p> This detector looks for violations of the idioms for writing
 cloneable classes. </p>
+]]>
+    </Details>
+  </Detector>
+  <Detector class="edu.umd.cs.findbugs.detect.ThrowingExceptions">
+    <Details>
+<![CDATA[
+<p> This detector looks for methods throwing RuntimeException and methods 
+that have Exception or Throwable in their throws clause. </p>
 ]]>
     </Details>
   </Detector>
@@ -7750,7 +7758,7 @@ publicized the bug pattern</a>.
 <p>
 The code uses x % 2 == 1 to check to see if a value is odd, but this won't work
 for negative numbers (e.g., (-5) % 2 == -1). If this code is intending to check
-for oddness, consider using x &amp; 1 == 1, or x % 2 != 0.
+for oddness, consider using (x &amp; 1) == 1, or x % 2 != 0.
 </p>
 ]]>
     </Details>
@@ -8653,7 +8661,58 @@ after the call to initLogging, the logger configuration is lost
       </p>]]>
     </Details>
   </BugPattern>
+  <BugPattern type="THROWS_METHOD_THROWS_RUNTIMEEXCEPTION">
+    <ShortDescription>Method intentionally throws RuntimeException.</ShortDescription>
+    <LongDescription>Method intentionally throws RuntimeException.</LongDescription>
+    <Details>
+      <![CDATA[
+        <p>
+        Method intentionally throws RuntimeException.<br>
+        
+        According to the <a href="https://wiki.sei.cmu.edu/confluence/display/java/ERR07-J.+Do+not+throw+RuntimeException%2C+Exception%2C+or+Throwable">SEI CERT ERR07-J rule</a>, 
+        throwing a RuntimeException may cause errors, like the caller not being able to examine the exception and therefore cannot properly recover from it.<br>
+
+        Moreover, throwing a RuntimeException would force the caller to catch RuntimeException and therefore violate the 
+        <a href="https://wiki.sei.cmu.edu/confluence/display/java/ERR08-J.+Do+not+catch+NullPointerException+or+any+of+its+ancestors">SEI CERT ERR08-J rule</a>.<br>
+        
+        Please note that you can derive from Exception or RuntimeException and may throw a new instance of that exception.
+        </p>]]>
+    </Details>
+  </BugPattern>
+  <BugPattern type="THROWS_METHOD_THROWS_CLAUSE_BASIC_EXCEPTION">
+    <ShortDescription>Method lists Exception in its throws clause.</ShortDescription>
+    <LongDescription>Method lists Exception in its throws clause.</LongDescription>
+    <Details>
+      <![CDATA[
+        <p>
+        Method lists Exception in its throws clause.<br>
 
+        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).<br><br>
+        
+        For more information, see the <a href="https://wiki.sei.cmu.edu/confluence/display/java/ERR07-J.+Do+not+throw+RuntimeException%2C+Exception%2C+or+Throwable">SEI CERT ERR07-J rule</a>.
+        </p>]]>
+    </Details>
+  </BugPattern>
+  <BugPattern type="THROWS_METHOD_THROWS_CLAUSE_THROWABLE">
+    <ShortDescription>Method lists Throwable in its throws clause.</ShortDescription>
+    <LongDescription>Method lists Throwable in its throws clause.</LongDescription>
+    <Details>
+      <![CDATA[
+        <p>
+        Method lists Throwable in its throws clause.<br>
+
+        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).<br>
+        
+        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.<br><br>
+        
+        For more information, see the <a href="https://wiki.sei.cmu.edu/confluence/display/java/ERR07-J.+Do+not+throw+RuntimeException%2C+Exception%2C+or+Throwable">SEI CERT ERR07-J rule</a>.
+        </p>]]>
+    </Details>
+  </BugPattern>
   <BugPattern type="PERM_SUPER_NOT_CALLED_IN_GETPERMISSIONS">
     <ShortDescription>Custom class loader does not call its superclass's getPermissions()</ShortDescription>
     <LongDescription>Custom class loader {1} does not call its superclass's getPermissions()</LongDescription>
@@ -8818,5 +8877,6 @@ after the call to initLogging, the logger configuration is lost
   <BugCode abbrev="REFLF">Reflection increasing accessibility of fields</BugCode>
   <BugCode abbrev="MC">Dangerous call to overridable method</BugCode>
   <BugCode abbrev="SSD">Do not use an instance lock to protect shared static data</BugCode>
+  <BugCode abbrev="THROWS">Exception throwing related problems</BugCode>
   <BugCode abbrev="PERM">Custom class loader does not call its superclass's getPermissions()</BugCode>
 </MessageCollection>

spotbugs/etc/messages_fr.xml

@@ -3253,7 +3253,7 @@ double value2 =  x / (double) y;
   <LongDescription>Un test d'impaire ne fonctionnera pas avec les négatifs dans {1}</LongDescription>
   <Details>
 <![CDATA[
-<p>Le code utilise <code>x % 2 == 1</code> pour véifier si une valeur est impaire, mais cela ne fonctionera pas avec une valeur négative (Ex. : <code>(-5) % 2 == -1</code>). Si ce code doit tester si une valeur est impaire, envisagez d'utiliser <code>x & 1 == 1</code> ou <code>x % 2 != 0</code>.</p>
+<p>Le code utilise <code>x % 2 == 1</code> pour véifier si une valeur est impaire, mais cela ne fonctionera pas avec une valeur négative (Ex. : <code>(-5) % 2 == -1</code>). Si ce code doit tester si une valeur est impaire, envisagez d'utiliser <code>(x & 1) == 1</code> ou <code>x % 2 != 0</code>.</p>
 ]]>
   </Details>
  </BugPattern>

spotbugs/etc/messages_ja.xml

@@ -8272,7 +8272,7 @@ Joshua Bloch が <a href="http://googleresearch.blogspot.com/2006/06/extra-extra
 <![CDATA[
 <p>
 このコードは <code>x % 2 == 1</code> を使用して値が負数なのか確かめていますが，負数 (たとえば， <code>(-5) % 2 == -1</code>) なので機能しません。
-奇数チェックを意図しているなら， <code>x &amp; 1 == 1</code> または <code>x % 2 != 0</code> を使用することを検討してください。
+奇数チェックを意図しているなら， <code>(x &amp; 1) == 1</code> または <code>x % 2 != 0</code> を使用することを検討してください。
 </p>
 ]]>
     </Details>

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

@@ -25,22 +25,25 @@
 import edu.umd.cs.findbugs.ba.XField;
 import edu.umd.cs.findbugs.ba.XMethod;
 import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
-import org.apache.bcel.Const;
-import org.apache.bcel.classfile.JavaClass;
-
 import java.util.Optional;
 import java.util.stream.Stream;
+import org.apache.bcel.Const;
+import org.apache.bcel.classfile.JavaClass;
 
 public class FindInstanceLockOnSharedStaticData extends OpcodeStackDetector {
 
+    private static final String JAVA_LANG_CLASS = "java.lang.Class";
+
     private final BugAccumulator bugAccumulator;
     private boolean isInsideSynchronizedBlock;
     private Optional<XField> maybeLockObject;
+    private boolean isLockObjectInstanceOfJavaLangClass;
 
     public FindInstanceLockOnSharedStaticData(BugReporter bugReporter) {
         this.bugAccumulator = new BugAccumulator(bugReporter);
         isInsideSynchronizedBlock = false;
         maybeLockObject = Optional.empty();
+        isLockObjectInstanceOfJavaLangClass = false;
     }
 
     @Override
@@ -50,6 +53,18 @@ public void sawOpcode(int seen) {
             isInsideSynchronizedBlock = true;
             OpcodeStack.Item lockObject = stack.getStackItem(0);
             maybeLockObject = Optional.ofNullable(lockObject.getXField());
+
+            // Locking on java.lang.Class objects is appropriate, since there is only a single instance of them
+            if (!maybeLockObject.isPresent()) {
+                try {
+                    Optional<JavaClass> javaClassOfLockObject = Optional.ofNullable(lockObject.getJavaClass());
+                    isLockObjectInstanceOfJavaLangClass = javaClassOfLockObject
+                            .map(javaClass -> javaClass.getClassName().equals(JAVA_LANG_CLASS))
+                            .orElse(false);
+                } catch (ClassNotFoundException ignored) {
+                }
+            }
+
             return;
         } else if (seen == Const.MONITOREXIT) {
             isInsideSynchronizedBlock = false;
@@ -72,7 +87,8 @@ public void sawOpcode(int seen) {
                 return;
             }
 
-            boolean isLockObjectAppropriate = maybeLockObject.map(XField::isStatic).orElse(false);
+            boolean isLockObjectAppropriate = maybeLockObject.map(XField::isStatic).orElse(false)
+                    || isLockObjectInstanceOfJavaLangClass;
             if (fieldToModify.isPresent() && isInsideSynchronizedBlock && !isLockObjectAppropriate) {
                 bugAccumulator.accumulateBug(
                         new BugInstance(this, "SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA", NORMAL_PRIORITY)
@@ -98,6 +114,7 @@ public void visit(JavaClass obj) {
         if (classIsInteresting) {
             isInsideSynchronizedBlock = false;
             maybeLockObject = Optional.empty();
+            isLockObjectInstanceOfJavaLangClass = false;
             super.visit(obj);
         }
     }