False positive for SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA and other problems with the report

[trancexpress]

Reproduction with snippet:

package test;

public class TestStaticAccess {

	private static final Object lock = new Object();

	private static int x = 0;
	private static int y = 0;

	public synchronized void foo() {

		synchronized (lock) {
			x++;
			y++;
		}
	}
	
	public static void main(String[] args) {
		TestStaticAccess t = new TestStaticAccess();
		t.foo();
		System.out.println(x);
		System.out.println(y);
	}
}

SpotBugs 4.7 reports 2 problems:

At TestStaticAccess.java:[line 13]
In method test.TestStaticAccess.foo()
Field test.TestStaticAccess.x
Value x
Value synchronized method

Bug: Static field "test.TestStaticAccess.foo()" is modified by an instance level test.TestStaticAccess.foo().
Instance level synchronization, that modifies a static shared object could cause security leaks. If the lock or the method is not static, that modifies the static field, and is synchronized, that could leave the shared static data unprotected against concurrent access. This could occur in two ways, if a synchronization method uses a non-static lock object, or a synchronized method is declared as non-static. Both ways are ineffective. Best solution is to use a private static final lock object to secure the shared static data.

See SEI CERT rule [LCK06-J. Do not use an instance lock to protect shared static data](https://wiki.sei.cmu.edu/confluence/display/java/LCK06-J.+Do+not+use+an+instance+lock+to+protect+shared+static+data).

Rank: Scary (7), confidence: Normal
Pattern: SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA 
Type: SSD, Category: CORRECTNESS (Correctness)

At TestStaticAccess.java:[line 14]
In method test.TestStaticAccess.foo()
Field test.TestStaticAccess.y
Value y
Value synchronized method

Bug: Static field "test.TestStaticAccess.foo()" is modified by an instance level test.TestStaticAccess.foo().
Instance level synchronization, that modifies a static shared object could cause security leaks. If the lock or the method is not static, that modifies the static field, and is synchronized, that could leave the shared static data unprotected against concurrent access. This could occur in two ways, if a synchronization method uses a non-static lock object, or a synchronized method is declared as non-static. Both ways are ineffective. Best solution is to use a private static final lock object to secure the shared static data.

See SEI CERT rule [LCK06-J. Do not use an instance lock to protect shared static data](https://wiki.sei.cmu.edu/confluence/display/java/LCK06-J.+Do+not+use+an+instance+lock+to+protect+shared+static+data).

Rank: Scary (7), confidence: Normal
Pattern: SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA 
Type: SSD, Category: CORRECTNESS (Correctness)

There are 3 problems (that I see) with this (sorted from most important to least important):

1. The synchronization in the snippet is fine. There is a static final lock in a synchronized block, around the static field access. The use of which is also advised in the description of the bug.
2. The static field is not actually listed in the description of the bug. Instead, the description lists the method in which the problem was found.
3. The sentence "Instance level synchronization, that modifies a static shared object could cause security leaks." to me as a developer has no meaning. Security leaks? This is a race condition, a multi-threading problem. It might be a security problem depending on the code. In most cases, it will not be. But a security leak? As in, someone found about my password or passkey?

[trancexpress]

There is 1 more false positive, but I'm not sure what detector settings I need. The default ones don't report it. The settings in the product I work on do.

package test;

public class TestStaticAccess {

	private static final Object lock = new Object();

	private static int x = 0;
	private static int y = 0;

	public synchronized void foo() {

		synchronized (lock) {
			x++;
			y++;
		}
	}

	public static class Inner {

		private static Object o = null;

		private Object bar() {
			synchronized (lock) {
				if (o == null) {
					o = new Object();
				}
				return o;
			}
		}
	}

	public static void main(String[] args) {
		TestStaticAccess t = new TestStaticAccess();
		t.foo();
		Inner i = new Inner();
		i.bar();
		System.out.println(x);
		System.out.println(y);
	}

}

Bug: Static field "test.TestStaticAccess$Inner.bar()" is modified by an instance level test.TestStaticAccess$Inner.bar().
Instance level synchronization, that modifies a static shared object could cause security leaks. If the lock or the method is not static, that modifies the static field, and is synchronized, that could leave the shared static data unprotected against concurrent access. This could occur in two ways, if a synchronization method uses a non-static lock object, or a synchronized method is declared as non-static. Both ways are ineffective. Best solution is to use a private static final lock object to secure the shared static data.

See SEI CERT rule [LCK06-J. Do not use an instance lock to protect shared static data](https://wiki.sei.cmu.edu/confluence/display/java/LCK06-J.+Do+not+use+an+instance+lock+to+protect+shared+static+data).

[gonczmisi]

It should be a problem with the settings. If you check the test findNoSSDBugInClass_StaticLockObjectOnStaticSharedData in FindInstanceLockOnSharedStaticDataCheckTest you can see that this case covers the one you defined.

From what I see now, the category of the bugtype should be MT_CORRECTNESS, instead of CORRECTNESS (I don't know if this has anything to do with your projects settings).

I will fix the messages, thanks for the remarks. 👍

[trancexpress]

It should be a problem with the settings. If you check the test findNoSSDBugInClass_StaticLockObjectOnStaticSharedData in FindInstanceLockOnSharedStaticDataCheckTest you can see that this case covers the one you defined.

Which settings would that be? Compiler (JDT) or SpotBugs settings?

The test you mentioned has this in its contents:

package instanceLockOnSharedStaticData;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

public class StaticLockObjectOnStaticSharedData {
    private static int counter;
    private static final Object lock = new Object();

    @SuppressFBWarnings("ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD")
    public void methodWithoutBugs() {
        synchronized (lock) {
            counter++;
        }
    }

}

This is not the same as the snippet in the description, since the method is not synchronized.

The test also fails if the test snippet is adjusted to be similar to the snippet from the description.

diff --git a/spotbugsTestCases/src/java/instanceLockOnSharedStaticData/StaticLockObjectOnStaticSharedData.java b/spotbugsTestCases/src/java/instanceLockOnSharedStaticData/StaticLockObjectOnStaticSharedData.java
index ab1485c78..6c586846e 100644
--- a/spotbugsTestCases/src/java/instanceLockOnSharedStaticData/StaticLockObjectOnStaticSharedData.java
+++ b/spotbugsTestCases/src/java/instanceLockOnSharedStaticData/StaticLockObjectOnStaticSharedData.java                                                                                                                                                                        
@@ -7,7 +7,7 @@ public class StaticLockObjectOnStaticSharedData {
     private static final Object lock = new Object();
 
     @SuppressFBWarnings("ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD")
-    public void methodWithoutBugs() {
+    public synchronized void methodWithoutBugs() {
         synchronized (lock) {
             counter++;
         }

> Task :spotbugs-tests:test

edu.umd.cs.findbugs.detect.FindInstanceLockOnSharedStaticDataCheckTest > findNoSSDBugInClass_StaticLockObjectOnStaticSharedData FAILED
    java.lang.AssertionError: 
    Expected: Iterable containing exactly <0> BugInstance with:
    bugType="SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA",
         but: was <[SSD: Instance level lock was used on a shared static data]>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
        at edu.umd.cs.findbugs.detect.FindInstanceLockOnSharedStaticDataCheckTest.assertNumOfSSDBugs(FindInstanceLockOnSharedStaticDataCheckTest.java:49)
        at edu.umd.cs.findbugs.detect.FindInstanceLockOnSharedStaticDataCheckTest.findNoSSDBugInClass_StaticLockObjectOnStaticSharedData(FindInstanceLockOnSharedStaticDataCheckTest.java:43)

364 tests completed, 1 failed, 3 skipped

> Task :spotbugs-tests:test FAILED

FAILURE: Build failed with an exception.

Whereas the test doesn't fail without changes. I don't think this is a settings problem.

[gonczmisi]

I see, although the synchronized block provides proper safety with the static lock object, the method itself technically violates the rule, since it's not static and synchronized.
I will propose a fix for it, thanks for the perception!

[gonczmisi]

@trancexpress PR is opened, you can check the fix there.

[trancexpress]

@trancexpress PR is opened, you can check the fix there.

I tested with the fix, it works. Thanks!