From 441d26ccd5bd800b2faddf20a30517e6596e4e35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mih=C3=A1ly=20G=C3=B6ncz?= <32682501+gonczmisi@users.noreply.github.com> Date: Thu, 7 Apr 2022 04:14:59 +0200 Subject: [PATCH] fix: Excluded locking on java.lang.Class objects from SSD bug (#1984) Co-authored-by: Kengo TODA --- CHANGELOG.md | 1 + ...stanceLockOnSharedStaticDataCheckTest.java | 7 ++++++ .../FindInstanceLockOnSharedStaticData.java | 25 ++++++++++++++++--- .../LockingOnJavaLangClassObject.java | 11 ++++++++ 4 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 spotbugsTestCases/src/java/instanceLockOnSharedStaticData/LockingOnJavaLangClassObject.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 4379d24a695..e331a4cea23 100644 --- 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/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindInstanceLockOnSharedStaticDataCheckTest.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindInstanceLockOnSharedStaticDataCheckTest.java index ecbec0a05df..8f627acf7eb 100644 --- a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindInstanceLockOnSharedStaticDataCheckTest.java +++ b/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"); 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 index c7b75cec4b6..e85e56b9441 100644 --- 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 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 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 new file mode 100644 index 00000000000..b401e557349 --- /dev/null +++ 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++; + } + } +}