Introduced detector for rule VNA-00 and VNA-02 with test cases

CHANGELOG.md

@@ -5,6 +5,10 @@ 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-??-??
+### Added
+* New detector `BadVisibilityOnSharedPrimitiveVariables` for new bug type `SPV_BAD_VISIBILITY_ON_SHARED_PRIMITIVE_VARIABLES` (See [SEI CERT rule VNA00-J](https://wiki.sei.cmu.edu/confluence/display/java/VNA00-J.+Ensure+visibility+when+accessing+shared+primitive+variables))
+* New detector `CompoundOperationsOnSharedVariables` for new bug type `COSV_COMPOUND_OPERATIONS_ON_SHARED_VARIABLES` (See [SEI CERT rule VNA02-J](https://wiki.sei.cmu.edu/confluence/display/java/VNA02-J.+Ensure+that+compound+operations+on+shared+variables+are+atomic))
+
 ### Fixed
 - Fixed False positives for `RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE` on try-with-resources with interface references ([#1931](https://github.com/spotbugs/spotbugs/issues/1931))
 

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

@@ -0,0 +1,100 @@
+package edu.umd.cs.findbugs.detect;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.SystemProperties;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+import org.junit.Test;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.hasItem;
+
+public class BadVisibilityOnSharedPrimitiveVariablesTest extends AbstractIntegrationTest {
+
+    @Test
+    public void failurePath_fieldWithBadVisibility_whenOtherMethodHasSynchronizedBlock() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/sharedPrimitiveVariables/SynchronizedBlockAndBadVisibilityOnField.class");
+
+        assertSPVNumOfBugs(1);
+        assertSPVBug("SynchronizedBlockAndBadVisibilityOnField", "shutdown", 17);
+    }
+
+    @Test
+    public void failurePath_fieldWithBadVisibility_whenOtherMethodIsSynchronized() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/sharedPrimitiveVariables/SynchronizedMethodAndBadVisibilityOnField.class");
+
+        assertSPVNumOfBugs(1);
+        assertSPVBug("SynchronizedMethodAndBadVisibilityOnField", "shutdown", 17);
+    }
+
+    @Test
+    public void failurePath_fieldWithBadVisibility_whenClassExtendsThread() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/sharedPrimitiveVariables/FieldWithBadVisibilityThread.class");
+
+        assertSPVNumOfBugs(1);
+        assertSPVBug("FieldWithBadVisibilityThread", "shutdown", 18);
+    }
+
+    @Test
+    public void failurePath_fieldWithBadVisibility_whenClassImplementsRunnable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/sharedPrimitiveVariables/FieldWithBadVisibilityRunnable.class");
+
+        assertSPVNumOfBugs(1);
+        assertSPVBug("FieldWithBadVisibilityRunnable", "shutdown", 18);
+    }
+
+    @Test
+    public void happyPath_atomicField() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/sharedPrimitiveVariables/AtomicField.class");
+
+        assertSPVNumOfBugs(0);
+    }
+
+    @Test
+    public void happyPath_volatileField() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/sharedPrimitiveVariables/VolatileField.class");
+
+        assertSPVNumOfBugs(0);
+    }
+
+    @Test
+    public void happyPath_synchronizedBlock() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/sharedPrimitiveVariables/SynchronizedBlock.class");
+
+        assertSPVNumOfBugs(0);
+    }
+
+    @Test
+    public void happyPath_synchronizedMethod() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/sharedPrimitiveVariables/SynchronizedMethod.class");
+
+        assertSPVNumOfBugs(0);
+    }
+
+    private void assertSPVNumOfBugs(int num) {
+        final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("SPV_BAD_VISIBILITY_ON_SHARED_PRIMITIVE_VARIABLES")
+                .build();
+        assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher));
+    }
+
+    private void assertSPVBug(String className, String methodName, int line) {
+        final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType("SPV_BAD_VISIBILITY_ON_SHARED_PRIMITIVE_VARIABLES")
+                .inClass(className)
+                .inMethod(methodName)
+                .atLine(line)
+                .build();
+        assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
+    }
+
+}

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

@@ -0,0 +1,207 @@
+package edu.umd.cs.findbugs.detect;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.SystemProperties;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+import org.junit.Test;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.Matchers.hasItem;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class FindCompoundOperationsOnSharedVariablesTest extends AbstractIntegrationTest {
+
+    @Test
+    public void happyPath_compoundOperation_onSharedAtomicVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundOperationOnSharedAtomicVariable.class");
+
+        assertCOSVNumOfBugs(0);
+    }
+
+    @Test
+    public void happyPath_compoundOperationInsideSynchronizedBlock_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/SynchronizedBlockCompoundOperationOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(0);
+    }
+
+    @Test
+    public void happyPath_compoundOperationInsideSynchronizedMethod_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/SynchronizedMethodCompoundOperationOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(0);
+    }
+
+    // --num
+    @Test
+    public void reportsBugFor_compoundPreDecrementation_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundPreDecrementationOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundPreDecrementationOnSharedVariable", "getNum", 11);
+    }
+
+    // num--
+    @Test
+    public void reportsBugFor_compoundPostDecrementation_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundPostDecrementationOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundPostDecrementationOnSharedVariable", "getNum", 11);
+    }
+
+    // ++num
+    @Test
+    public void reportsBugFor_compoundPreIncrementation_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundPreIncrementationOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundPreIncrementationOnSharedVariable", "getNum", 11);
+    }
+
+    // num++
+    @Test
+    public void reportsBugFor_compoundPostIncrementation_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundPostIncrementationOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundPostIncrementationOnSharedVariable", "getNum", 11);
+    }
+
+    // &=
+    @Test
+    public void reportsBugFor_compoundIAND_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundIANDOperationOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundIANDOperationOnSharedVariable", "getNum", 11);
+    }
+
+    // |=
+    @Test
+    public void reportsBugFor_compoundIOR_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundIOROperationOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundIOROperationOnSharedVariable", "getNum", 11);
+    }
+
+    // >>>=
+    @Test
+    public void reportsBugFor_compoundLogicalRightShifting_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundLogicalRightShiftingOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundLogicalRightShiftingOnSharedVariable", "getNum", 11);
+    }
+
+    // >>=
+    @Test
+    public void reportsBugFor_compoundRightShifting_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundRightShiftingOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundRightShiftingOnSharedVariable", "getNum", 11);
+    }
+
+    // <<=
+    @Test
+    public void reportsBugFor_compoundLeftShifting_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundLeftShiftingOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundLeftShiftingOnSharedVariable", "getNum", 11);
+    }
+
+    // %=
+    @Test
+    public void reportsBugFor_compoundModulo_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundModuloOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundModuloOnSharedVariable", "getNum", 12);
+    }
+
+    // *=
+    @Test
+    public void reportsBugFor_compoundMultiplication_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundMultiplicationOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundMultiplicationOnSharedVariable", "getNum", 11);
+    }
+
+    // /=
+    @Test
+    public void reportsBugFor_compoundDivision_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundDivisionOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundDivisionOnSharedVariable", "getNum", 11);
+    }
+
+    // -=
+    @Test
+    public void reportsBugFor_compoundSubtraction_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundSubtractionOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundSubtractionOnSharedVariable", "getNum", 11);
+    }
+
+
+    // +=
+    @Test
+    public void reportsBugFor_compoundAddition_onSharedVolatileVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundAdditionOnSharedVolatileVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundAdditionOnSharedVolatileVariable", "getNum", 11);
+    }
+
+    // ^=
+    @Test
+    public void reportsBugFor_compoundXOROperation_onSharedVariable() {
+        SystemProperties.setProperty("ful.debug", "true");
+        performAnalysis("multithreaded/compoundOperationOnSharedVariables/CompoundXOROperationOnSharedVariable.class");
+
+        assertCOSVNumOfBugs(1);
+        assertCOSVBug("CompoundXOROperationOnSharedVariable", "getFlag", 11);
+    }
+
+    private void assertCOSVNumOfBugs(int num) {
+        final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("COSV_COMPOUND_OPERATIONS_ON_SHARED_VARIABLES")
+                .build();
+        assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher));
+    }
+
+    private void assertCOSVBug(String className, String methodName, int line) {
+        final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType("COSV_COMPOUND_OPERATIONS_ON_SHARED_VARIABLES")
+                .inClass(className)
+                .inMethod(methodName)
+                .atLine(line)
+                .build();
+        assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
+    }
+}

spotbugs/etc/findbugs.xml

@@ -669,6 +669,10 @@
                     reports="PERM_SUPER_NOT_CALLED_IN_GETPERMISSIONS"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindPotentialSecurityCheckBasedOnUntrustedSource" speed="fast"
                     reports="USC_POTENTIAL_SECURITY_CHECK_BASED_ON_UNTRUSTED_SOURCE"/>
+          <Detector class="edu.umd.cs.findbugs.detect.BadVisibilityOnSharedPrimitiveVariables"
+                    reports="SPV_BAD_VISIBILITY_ON_SHARED_PRIMITIVE_VARIABLES"/>
+          <Detector class="edu.umd.cs.findbugs.detect.CompoundOperationsOnSharedVariables"
+                    reports="COSV_COMPOUND_OPERATIONS_ON_SHARED_VARIABLES"/>
             <!-- Bug Categories -->
            <BugCategory category="NOISE" hidden="true"/>
 
@@ -1286,4 +1290,6 @@
           <BugPattern abbrev="PERM" type="PERM_SUPER_NOT_CALLED_IN_GETPERMISSIONS" category="MALICIOUS_CODE" />
           <BugPattern abbrev="USC" type="USC_POTENTIAL_SECURITY_CHECK_BASED_ON_UNTRUSTED_SOURCE"
                     category="MALICIOUS_CODE"/>
+          <BugPattern abbrev="SPV" type="SPV_BAD_VISIBILITY_ON_SHARED_PRIMITIVE_VARIABLES" category="MT_CORRECTNESS" />
+          <BugPattern abbrev="COSV" type="COSV_COMPOUND_OPERATIONS_ON_SHARED_VARIABLES" category="MT_CORRECTNESS"/>
 </FindbugsPlugin>