Merge branch 'master' into master

spotbugs · Sep 1, 2022 · c4a1867 · c4a1867
2 parents 182df26 + 06a1eeb
commit c4a1867
Show file tree

Hide file tree

Showing 15 changed files with 150 additions and 86 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,8 +7,12 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 ## Unreleased - 2022-??-??
 ### Fixed
 - Bumped gson from 2.9.0 to 2.9.1 ([#2136](https://github.com/spotbugs/spotbugs/pull/2136))
+- Bump up SLF4J API to `2.0.0`
+- Bump up logback to `1.4.0`
+- Bump up log4j2 binding to `2.18.0`
 - Fixed InvalidInputException in Eclipse while bug reporting ([#2134](https://github.com/spotbugs/spotbugs/issues/2134))
 - Avoid warning on use of security manager on Java 17 and newer. ([#1579](https://github.com/spotbugs/spotbugs/issues/1579))
+- 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))
 
 ## 4.7.1 - 2022-06-26
 ### Fixed

diff --git a/build.gradle b/build.gradle
@@ -1,6 +1,6 @@
 plugins {
   id "org.sonarqube" version "3.4.0.2513"
-  id "com.diffplug.spotless" version "6.5.2"
+  id "com.diffplug.spotless" version "6.10.0"
   id "org.gradle.crypto.checksum" version "1.4.0"
   id "com.github.spotbugs" version "5.0.10"
   id "io.github.gradle-nexus.publish-plugin" version "1.1.0"

diff --git a/buildSrc/src/main/kotlin/constraints.gradle.kts b/buildSrc/src/main/kotlin/constraints.gradle.kts
@@ -7,14 +7,14 @@ dependencies {
         implementation("org.apache.logging.log4j:log4j-core") {
             version {
                 strictly("[2.17.1, 3[")
-                prefer("2.17.1")
+                prefer("2.18.0")
             }
             because("CVE-2021-44228, CVE-2021-45046, CVE-2021-45105, CVE-2021-44832: Log4j vulnerable to remote code execution and other critical security vulnerabilities")
         }
         implementation("ch.qos.logback:logback-core") {
             version {
                 strictly("[1.2.9, 2[")
-                prefer("1.2.10")
+                prefer("1.4.0")
             }
             because("CVE-2021-42550: Logback vulnerable to remote code execution vulnerabilities")
         }

diff --git a/docs/locale/ja/LC_MESSAGES/bugDescriptions.po b/docs/locale/ja/LC_MESSAGES/bugDescriptions.po
@@ -541,7 +541,7 @@ msgstr ""
 
 #: ../../generated/bugDescriptionList.inc:524
 msgid ""
-"  <p> This code negatives the return value of a compareTo or compare "
+"  <p> This code negates the return value of a compareTo or compare "
 "method.\n"
 "This is a questionable or bad programming practice, since if the return\n"
 "value is Integer.MIN_VALUE, negating the return value won't\n"

diff --git a/docs/locale/pt_BR/LC_MESSAGES/bugDescriptions.po b/docs/locale/pt_BR/LC_MESSAGES/bugDescriptions.po
@@ -704,7 +704,7 @@ msgstr ""
 
 #: ../../generated/bugDescriptionList.inc:524
 msgid ""
-"  <p> This code negatives the return value of a compareTo or compare "
+"  <p> This code negates the return value of a compareTo or compare "
 "method.\n"
 "This is a questionable or bad programming practice, since if the return\n"
 "value is Integer.MIN_VALUE, negating the return value won't\n"

diff --git a/spotbugs-tests/build.gradle b/spotbugs-tests/build.gradle
@@ -2,7 +2,7 @@ apply from: "$rootDir/gradle/checkstyle.gradle"
 apply from: "$rootDir/gradle/jacoco.gradle"
 
 ext {
-  log4jVersion = '2.17.2'
+  log4jVersion = '2.18.0'
 }
 
 dependencies {

diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/AbstractIntegrationTest.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/AbstractIntegrationTest.java
@@ -21,10 +21,14 @@
 
 import static org.junit.Assert.assertTrue;
 
-import java.io.File;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Arrays;
-import java.util.stream.Collectors;
+import java.util.List;
 
 import edu.umd.cs.findbugs.internalAnnotations.SlashedClassName;
 import edu.umd.cs.findbugs.test.AnalysisRunner;
@@ -64,40 +68,36 @@
 public abstract class AbstractIntegrationTest {
 
     /**
-     * Build path if running command line build
+     * Build prefix directories to search for classes
      */
-    private static final String BUILD_CLASSES_CLI = "/build/classes/java/main/";
-
-    /**
-     * Build path if running in Eclipse
-     */
-    private static final String BUILD_CLASSES_ECLIPSE = "/classesEclipse/";
+    private static final List<Path> BUILD_CLASS_SEARCH_PREFIXES = List.of(
+            // Build path if running command line build
+            Paths.get("build/classes/java/main"),
+            Paths.get("build/classes/groovy/main"),
+            // Build path if running in Eclipse
+            Paths.get("classesEclipse"));
 
     private BugCollectionBugReporter bugReporter;
 
-    private static File getFindbugsTestCases() {
-        final File f = new File(SystemProperties.getProperty("spotbugsTestCases.home", "../spotbugsTestCases"));
-        assertTrue("'spotbugsTestCases' directory not found", f.exists());
-        assertTrue(f.isDirectory());
-        assertTrue(f.canRead());
+    private static Path getFindbugsTestCases() {
+        final Path p = Paths.get(SystemProperties.getProperty("spotbugsTestCases.home", "../spotbugsTestCases"));
+        assertTrue("'spotbugsTestCases' directory not found", Files.exists(p));
+        assertTrue(Files.isDirectory(p));
+        assertTrue(Files.isReadable(p));
 
-        return f;
+        return p;
     }
 
-    private static File getFindbugsTestCasesFile(final String path) {
-        File f = new File(getFindbugsTestCases(), path);
-        if (!f.exists() && path.startsWith(BUILD_CLASSES_CLI)) {
-            String replaced = path.replace(BUILD_CLASSES_CLI, BUILD_CLASSES_ECLIPSE);
-            replaced = replaced.replace("../java9/", "");
-            File f2 = new File(getFindbugsTestCases(), replaced);
-            if (f2.exists()) {
-                f = f2;
-            }
-        }
-        assertTrue(f.getAbsolutePath() + " not found", f.exists());
-        assertTrue(f.getAbsolutePath() + " is not readable", f.canRead());
+    private static Path getFindbugsTestCasesFile(final String path) {
+        final Path root = getFindbugsTestCases();
+        final Path p = BUILD_CLASS_SEARCH_PREFIXES.stream()
+                .map(prefix -> root.resolve(prefix).resolve(path))
+                .filter(Files::exists)
+                .findFirst()
+                .orElseThrow(() -> new AssertionError(path + " not found"));
 
-        return f;
+        assertTrue(p + " is not readable", Files.isReadable(p));
+        return p;
     }
 
     protected BugCollection getBugCollection() {
@@ -112,19 +112,21 @@ protected BugCollection getBugCollection() {
     protected void performAnalysis(@SlashedClassName final String... analyzeMe) {
         AnalysisRunner runner = new AnalysisRunner();
 
-        final File lib = getFindbugsTestCasesFile("lib");
-        for (final File f : lib.listFiles()) {
-            final String path = f.getPath();
-            if (f.canRead() && path.endsWith(".jar")) {
-                runner.addAuxClasspathEntry(f.toPath());
+        final Path lib = getFindbugsTestCases().resolve("lib");
+        try (DirectoryStream<Path> ds = Files.newDirectoryStream(lib, "*.jar")) {
+            for (Path jar : ds) {
+                if (Files.isReadable(jar)) {
+                    runner.addAuxClasspathEntry(jar);
+                }
             }
+        } catch (IOException e) {
+            throw new UncheckedIOException(e);
         }
 
         // TODO : Unwire this once we move bug samples to a proper sourceset
         Path[] paths = Arrays.stream(analyzeMe)
-                .map(s -> getFindbugsTestCasesFile(BUILD_CLASSES_CLI + s).toPath())
-                .collect(Collectors.toList())
-                .toArray(new Path[analyzeMe.length]);
+                .map(AbstractIntegrationTest::getFindbugsTestCasesFile)
+                .toArray(Path[]::new);
         bugReporter = runner.run(paths);
     }
 }
diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/ba/Issue1338Test.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/ba/Issue1338Test.java
@@ -0,0 +1,29 @@
+package edu.umd.cs.findbugs.ba;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.junit.Assert.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;
+
+/**
+ * @see <a href="https://github.com/spotbugs/spotbugs/issues/1338">GitHub issue</a>
+ */
+public class Issue1338Test extends AbstractIntegrationTest {
+
+    private static final String[] CLASS_LIST = { "../java11/module-info.class", "../java11/Issue1338.class", };
+
+    /**
+     * Test that calling a method call when initializing a try-with-resource variable doesn't result in redundant nullcheck of nonnull value.
+     */
+    @Test
+    public void testMethodCallInTryWithResource() {
+        performAnalysis(CLASS_LIST);
+        BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder().bugType("RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE")
+                .build();
+        assertThat(getBugCollection(), containsExactly(0, bugTypeMatcher));
+    }
+}
diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/ba/Issue2145Test.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/ba/Issue2145Test.java
@@ -0,0 +1,19 @@
+package edu.umd.cs.findbugs.ba;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.hasSize;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.SortedBugCollection;
+import org.junit.Test;
+
+public class Issue2145Test extends AbstractIntegrationTest {
+
+    @Test
+    public void test() {
+        performAnalysis("ghIssues/Issue2145.class");
+        SortedBugCollection bugCollection = (SortedBugCollection) getBugCollection();
+        assertThat(bugCollection.getErrors(), hasSize(0));
+    }
+
+}
diff --git a/spotbugs/build.gradle b/spotbugs/build.gradle
@@ -24,7 +24,7 @@ configurations {
 
 ext {
   asmVersion = '9.3'
-  log4jVersion = '2.17.2'
+  log4jVersion = '2.18.0'
 }
 
 sourceSets {
@@ -86,8 +86,8 @@ dependencies {
   implementation 'jaxen:jaxen:1.2.0' // only transitive through dom4j:dom4j:1.6.1, which has an *optional* dependency on jaxen:jaxen.
   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.3'
+  api 'org.slf4j:slf4j-api:2.0.0'
+  implementation 'net.sf.saxon:Saxon-HE:11.4'
   logBinding ("org.apache.logging.log4j:log4j-slf4j18-impl:$log4jVersion") {
     exclude group: 'org.slf4j'
   }
@@ -357,14 +357,14 @@ dependencies {
     logBinding("org.apache.logging.log4j:log4j-core") {
       version {
         strictly("[2.17.1, 3[")
-        prefer("2.17.1")
+        prefer("2.18.0")
       }
       because("CVE-2021-44228, CVE-2021-45046, CVE-2021-45105, CVE-2021-44832: Log4j vulnerable to remote code execution and other critical security vulnerabilities")
     }
     logBinding("ch.qos.logback:logback-core") {
       version {
         strictly("[1.2.9, 2[")
-        prefer("1.2.10")
+        prefer("1.4.0")
       }
       because("CVE-2021-42550: Logback vulnerable to remote code execution vulnerabilities")
     }

diff --git a/spotbugs/etc/messages.xml b/spotbugs/etc/messages.xml
@@ -3252,7 +3252,7 @@ that classfile. See <a href="http://bugs.java.com/bugdatabase/view_bug.do?bug_id
     <LongDescription>{1} negates the return value of {2}</LongDescription>
     <Details>
 <![CDATA[
-  <p> This code negatives the return value of a compareTo or compare method.
+  <p> This code negates the return value of a compareTo or compare method.
 This is a questionable or bad programming practice, since if the return
 value is Integer.MIN_VALUE, negating the return value won't
 negate the sign of the result. You can achieve the same intended result

diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/BetterCFGBuilder2.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/BetterCFGBuilder2.java
@@ -35,40 +35,7 @@
 import org.apache.bcel.classfile.ClassParser;
 import org.apache.bcel.classfile.JavaClass;
 import org.apache.bcel.classfile.Method;
-import org.apache.bcel.generic.ACONST_NULL;
-import org.apache.bcel.generic.ALOAD;
-import org.apache.bcel.generic.BranchInstruction;
-import org.apache.bcel.generic.ClassGen;
-import org.apache.bcel.generic.CodeExceptionGen;
-import org.apache.bcel.generic.ConstantPoolGen;
-import org.apache.bcel.generic.ExceptionThrower;
-import org.apache.bcel.generic.GETFIELD;
-import org.apache.bcel.generic.GETSTATIC;
-import org.apache.bcel.generic.GOTO;
-import org.apache.bcel.generic.GotoInstruction;
-import org.apache.bcel.generic.ICONST;
-import org.apache.bcel.generic.IFNONNULL;
-import org.apache.bcel.generic.IFNULL;
-import org.apache.bcel.generic.IF_ACMPEQ;
-import org.apache.bcel.generic.IF_ACMPNE;
-import org.apache.bcel.generic.INSTANCEOF;
-import org.apache.bcel.generic.INVOKESTATIC;
-import org.apache.bcel.generic.IfInstruction;
-import org.apache.bcel.generic.Instruction;
-import org.apache.bcel.generic.InstructionHandle;
-import org.apache.bcel.generic.InstructionList;
-import org.apache.bcel.generic.InstructionTargeter;
-import org.apache.bcel.generic.JsrInstruction;
-import org.apache.bcel.generic.LDC;
-import org.apache.bcel.generic.MONITOREXIT;
-import org.apache.bcel.generic.MethodGen;
-import org.apache.bcel.generic.NEW;
-import org.apache.bcel.generic.NOP;
-import org.apache.bcel.generic.POP;
-import org.apache.bcel.generic.POP2;
-import org.apache.bcel.generic.PUTFIELD;
-import org.apache.bcel.generic.PUTSTATIC;
-import org.apache.bcel.generic.ReturnInstruction;
+import org.apache.bcel.generic.*;
 
 import edu.umd.cs.findbugs.SystemProperties;
 import edu.umd.cs.findbugs.ba.type.ExceptionSetFactory;
@@ -1010,6 +977,7 @@ private boolean isPEI(InstructionHandle handle) throws CFGBuilderException {
         if (ins instanceof PUTFIELD && !methodGen.isStatic()) {
             // Assume that PUTFIELD on this object is not PEI
             int depth = ins.consumeStack(cpg);
+
             for (InstructionHandle prev = handle.getPrev(); prev != null; prev = prev.getPrev()) {
                 Instruction prevInst = prev.getInstruction();
                 if (prevInst instanceof BranchInstruction) {
@@ -1027,11 +995,19 @@ private boolean isPEI(InstructionHandle handle) throws CFGBuilderException {
                         return true;
                     }
                 }
-                depth = depth - prevInst.produceStack(cpg) + prevInst.consumeStack(cpg);
+                if (prevInst instanceof DUP_X2 && depth == 4) {
+                    depth = 1;
+                } else if (prevInst instanceof DUP_X1 && depth == 3) {
+                    depth = 1;
+                } else {
+                    depth = depth - prevInst.produceStack(cpg) + prevInst.consumeStack(cpg);
+                }
                 if (depth < 1) {
                     throw new CFGBuilderException("Invalid stack at " + prev + " when checking " + handle);
                 }
                 if (depth == 1) {
+                    // now we assume that the previous instruction (prevPrev) is the opcode
+                    // that pushes the reference to the target of PUTFIELD
                     InstructionHandle prevPrev = prev.getPrev();
                     if (prevPrev != null && prevPrev.getInstruction() instanceof BranchInstruction) {
                         continue;

diff --git a/spotbugsTestCases/build.gradle b/spotbugsTestCases/build.gradle
@@ -1,8 +1,15 @@
+plugins {
+  id 'groovy'
+}
+
 sourceSets {
   main {
     java {
       srcDirs = ['src/java', 'src/fakeAnnotations', 'src/fakeLibraries']
     }
+    groovy {
+      srcDirs = ['src/groovy']
+    }
   }
 }
 
@@ -20,17 +27,18 @@ dependencies {
   implementation "com.google.inject.extensions:guice-assistedinject:${guiceVersion}"
   implementation "com.google.inject.extensions:guice-servlet:${guiceVersion}"
   implementation 'com.google.truth:truth:1.1.3'
-  implementation 'joda-time:joda-time:2.10.14'
+  implementation 'joda-time:joda-time:2.11.1'
   api 'net.jcip:jcip-annotations:1.0'
-  implementation 'org.springframework:spring-core:5.3.21'
+  implementation 'org.springframework:spring-core:5.3.22'
   compileOnly 'javax.annotation:javax.annotation-api:1.3.2'
-  implementation 'org.checkerframework:checker-qual:3.22.2'
+  implementation 'org.checkerframework:checker-qual:3.24.0'
 
   implementation 'junit:junit:4.13.2'
   implementation 'org.testng:testng:7.5'
 
   implementation project(':spotbugs')
   api project(':spotbugs-annotations')
+  implementation 'org.apache.groovy:groovy-all:4.0.4'
 }
 
 tasks.withType(JavaCompile).configureEach {

diff --git a/spotbugsTestCases/src/groovy/ghIssues/Issue2145.groovy b/spotbugsTestCases/src/groovy/ghIssues/Issue2145.groovy
@@ -0,0 +1,8 @@
+package ghIssues
+
+class Issue2145 {
+    final double field1
+    Issue2145(double field1) {
+        this.field1 = field1
+    }
+}