fix: Excluded locking on java.lang.Class objects from SSD bug (#1984)

Co-authored-by: Kengo TODA <skypencil@gmail.com>
spotbugs · Apr 7, 2022 · 441d26c · 441d26c
1 parent f6adc7d
commit 441d26c
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 ### Fixed
 - 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))
+- 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

diff --git a/...src/test/java/edu/umd/cs/findbugs/detect/FindInstanceLockOnSharedStaticDataCheckTest.java b/...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");

diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindInstanceLockOnSharedStaticData.java b/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);
         }
     }

diff --git a/spotbugsTestCases/src/java/instanceLockOnSharedStaticData/LockingOnJavaLangClassObject.java b/spotbugsTestCases/src/java/instanceLockOnSharedStaticData/LockingOnJavaLangClassObject.java
@@ -0,0 +1,11 @@
+package instanceLockOnSharedStaticData;
+
+public class LockingOnJavaLangClassObject {
+    private static int counter = 0;
+
+    public static int incrementCounter() {
+        synchronized (LockingOnJavaLangClassObject.class) {
+            return counter++;
+        }
+    }
+}