Skip to content

Commit 2872d40

Browse files
Patrik SüliPatrikScully
Patrik Süli
authored and
PatrikScully
committed
Add VNA03-J detector with test cases
1 parent 0b00474 commit 2872d40

File tree

54 files changed

+1773
-45
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

54 files changed

+1773
-45
lines changed

‎CHANGELOG.md

+5
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
99
### Fixed
1010
- Do not report BC_UNCONFIRMED_CAST for Java 21's type switches when the switch instruction is TABLESWITCH ([#2782](https://github.com/spotbugs/spotbugs/issues/2782))
1111

12+
### Added
13+
- New detector `AtomicOperationsCombinedDetector` and introduced new bug types:
14+
- `AT_COMBINED_ATOMIC_OPERATIONS_ARE_NOT_ATOMIC` is reported in case of combined atomic operations are not synchronized.
15+
- `AT_ATOMIC_OPERATION_NEEDS_SYNCHRONIZATION` is reported when an atomic operation is not synchronized, but should be because of thread safety.
16+
1217
## 4.8.5 - 2024-05-03
1318
### Fixed
1419
- Fix FP `SING_SINGLETON_GETTER_NOT_SYNCHRONIZED` with eager instances ([#2932](https://github.com/spotbugs/spotbugs/issues/2932))

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

+256
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,256 @@
1+
package edu.umd.cs.findbugs.detect;
2+
3+
import org.junit.jupiter.api.Test;
4+
import edu.umd.cs.findbugs.AbstractIntegrationTest;
5+
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
6+
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
7+
8+
import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
9+
import static org.hamcrest.MatcherAssert.assertThat;
10+
import static org.hamcrest.Matchers.hasItem;
11+
12+
class AtomicOperationsCombinedDetectorTest extends AbstractIntegrationTest {
13+
14+
public static final String OPERATIONS_ARE_NOT_ATOMIC = "AT_COMBINED_ATOMIC_OPERATIONS_ARE_NOT_ATOMIC";
15+
public static final String NEEDS_SYNCHRONIZATION = "AT_ATOMIC_OPERATION_NEEDS_SYNCHRONIZATION";
16+
17+
@Test
18+
void testSafeSynchronizedList() {
19+
performAnalysis(
20+
"atomicMethods/SafeSynchronizedList.class",
21+
"atomicMethods/SafeSynchronizedLists.class",
22+
"atomicMethods/SafeSynchronizedList2.class",
23+
"atomicMethods/SafeSynchronizedListWithMultipleSync.class",
24+
"atomicMethods/SafeSynchronizedListWithMultipleSync2.class",
25+
"atomicMethods/SafeSynchronizedListWithMethodInit.class",
26+
"atomicMethods/SafeSingleSynchronizedListCall.class",
27+
"atomicMethods/SynchronizedListWithDifferentLock.class");
28+
assertZeroBugs();
29+
}
30+
31+
@Test
32+
void testUnsafeSynchronizedList() {
33+
performAnalysis("atomicMethods/UnsafeSynchronizedList.class");
34+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 1);
35+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 0);
36+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedList", "addAndPrintNumbers", 13);
37+
}
38+
39+
@Test
40+
void testUnsafeSynchronizedListWithMultipleSync() {
41+
performAnalysis("atomicMethods/UnsafeSynchronizedListWithMultipleSync.class");
42+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 1);
43+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 0);
44+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedListWithMultipleSync", "addAndPrintNumbers", 32);
45+
}
46+
47+
@Test
48+
void testUnsafeSynchronizedListWithMethodInit() {
49+
performAnalysis("atomicMethods/UnsafeSynchronizedListWithMethodInit.class");
50+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 1);
51+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 1);
52+
assertBug(NEEDS_SYNCHRONIZATION, "UnsafeSynchronizedListWithMethodInit", "init", 12);
53+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedListWithMethodInit", "addAndPrintNumbers", 17);
54+
}
55+
56+
@Test
57+
void testSafeSynchronizedSet() {
58+
performAnalysis(
59+
"atomicMethods/SafeSynchronizedSet.class",
60+
"atomicMethods/SafeSynchronizedSet2.class",
61+
"atomicMethods/SafeSynchronizedSetWithUnusedVars.class");
62+
assertZeroBugs();
63+
}
64+
65+
@Test
66+
void testUnsafeSynchronizedSet() {
67+
performAnalysis(
68+
"atomicMethods/UnsafeSynchronizedSet.class",
69+
"atomicMethods/UnsafeSynchronizedSortedSet.class",
70+
"atomicMethods/UnsafeSynchronizedNavigableSet.class");
71+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 3);
72+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 0);
73+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedSet", "addAndPrintNumbers", 13);
74+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedSortedSet", "addAndPrintNumbers", 12);
75+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedNavigableSet", "addAndPrintNumbers", 12);
76+
}
77+
78+
@Test
79+
void testUnsafeSynchronizedCollection() {
80+
performAnalysis("atomicMethods/UnsafeSynchronizedCollection.class");
81+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 1);
82+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 0);
83+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedCollection", "addAndPrintNumbers", 12);
84+
}
85+
86+
@Test
87+
void testUnsafeSynchronizedMap() {
88+
performAnalysis(
89+
"atomicMethods/UnsafeSynchronizedMap.class",
90+
"atomicMethods/UnsafeSynchronizedNavigableMap.class",
91+
"atomicMethods/UnsafeSynchronizedSortedMap.class");
92+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 3);
93+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 0);
94+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedMap", "addAndPrintNumbers", 13);
95+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedNavigableMap", "addAndPrintNumbers", 12);
96+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedSortedMap", "addAndPrintNumbers", 12);
97+
}
98+
99+
@Test
100+
void testSynchronizedListInSynchronizedMethod() {
101+
performAnalysis(
102+
"atomicMethods/SynchronizedListInSynchronizedMethod.class",
103+
"atomicMethods/SynchronizeInSynchronizedMethod.class",
104+
"atomicMethods/SafeSynchronizedListWithPrivateMethods.class");
105+
assertZeroBugs();
106+
}
107+
108+
@Test
109+
void testUnsafeSynchronizedListAndSet() {
110+
performAnalysis("atomicMethods/UnsafeSynchronizedListAndSet.class");
111+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 2);
112+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 0);
113+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedListAndSet", "addAndPrintNumbers", 16);
114+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedListAndSet", "addAndPrintNumbers", 20);
115+
}
116+
117+
@Test
118+
void testPartialSynchronizedListAndSet() {
119+
performAnalysis("atomicMethods/PartialSynchronizedListAndSet.class", "atomicMethods/PartialSynchronizedListAndSet2.class");
120+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 2);
121+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 0);
122+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "PartialSynchronizedListAndSet", "addAndPrintNumbers", 16);
123+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "PartialSynchronizedListAndSet2", "addAndPrintNumbers", 22);
124+
}
125+
126+
@Test
127+
void testUnsafeSynchronizedListWithPrivateMethods() {
128+
performAnalysis("atomicMethods/UnsafeSynchronizedListWithPrivateMethods.class");
129+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 1);
130+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 0);
131+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedListWithPrivateMethods", "addAndPrintNumbers", 13);
132+
}
133+
134+
@Test
135+
void testUnsafeSynchronizedSetInMultipleMethods() {
136+
performAnalysis("atomicMethods/UnsafeSynchronizedSetInMultipleMethods.class");
137+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 2);
138+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 0);
139+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedSetInMultipleMethods", "addAndPrintNumbers", 13);
140+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedSetInMultipleMethods", "removeAndPrintNumbers", 19);
141+
}
142+
143+
@Test
144+
void testSafeSynchronizedMapIncrement() {
145+
performAnalysis("atomicMethods/SafeSynchronizedMapIncrement.class");
146+
assertZeroBugs();
147+
}
148+
149+
@Test
150+
void testUnsafeSynchronizedMapIncrement() {
151+
performAnalysis("atomicMethods/UnsafeSynchronizedMapIncrement.class");
152+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 1);
153+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 1);
154+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeSynchronizedMapIncrement", "increment", 17);
155+
assertBug(NEEDS_SYNCHRONIZATION, "UnsafeSynchronizedMapIncrement", "getCount", 21);
156+
}
157+
158+
@Test
159+
void testSafeAtomicReference() {
160+
performAnalysis("atomicMethods/SafeAtomicReference.class", "atomicMethods/AtomicLongWithMethodParam.class");
161+
assertZeroBugs();
162+
}
163+
164+
@Test
165+
void testUnsafeAtomicReference() {
166+
performAnalysis(
167+
"atomicMethods/UnsafeAtomicReference.class",
168+
"atomicMethods/UnsafeAtomicReference2.class",
169+
"atomicMethods/UnsafeAtomicReference3.class",
170+
"atomicMethods/UnsafeAtomicReference4.class",
171+
"atomicMethods/UnsafeAtomicReference4$Inner.class",
172+
"atomicMethods/UnsafeAtomicReference5.class");
173+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 11);
174+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 3);
175+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeAtomicReference", "update", 17);
176+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeAtomicReference", "add", 22);
177+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeAtomicReference2", "incrementAtomicInteger", 18);
178+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeAtomicReference2", "incrementAtomicInteger", 18);
179+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeAtomicReference2", "incrementAtomicInteger2", 26);
180+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeAtomicReference2", "incrementAtomicInteger2", 26);
181+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeAtomicReference3", "preparation", 11);
182+
assertBug(NEEDS_SYNCHRONIZATION, "UnsafeAtomicReference3", "run", 15);
183+
assertBug(NEEDS_SYNCHRONIZATION, "UnsafeAtomicReference3", "after", 19);
184+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeAtomicReference4", "initProcessing", 22);
185+
assertBug(NEEDS_SYNCHRONIZATION, "UnsafeAtomicReference4", "finishProcessing", 33);
186+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeAtomicReference5", "lambda$incrementAtomicInteger$0", 15);
187+
}
188+
189+
@Test
190+
void testUnsafeAtomicReferenceValueSet() {
191+
performAnalysis("atomicMethods/UnsafeAtomicReferenceValueSet.class");
192+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 1);
193+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 1);
194+
assertBug(NEEDS_SYNCHRONIZATION, "UnsafeAtomicReferenceValueSet", "init", 10);
195+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeAtomicReferenceValueSet", "update", 15);
196+
}
197+
198+
@Test
199+
void testSafeLocalAtomicInteger() {
200+
performAnalysis("atomicMethods/SafeLocalAtomicInteger.class", "atomicMethods/SafeLocalAtomicInteger2.class");
201+
assertZeroBugs();
202+
}
203+
204+
@Test
205+
void testUnsafeLocalAtomicInteger() {
206+
performAnalysis("atomicMethods/UnsafeLocalAtomicInteger.class");
207+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 1);
208+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 0);
209+
assertBug(OPERATIONS_ARE_NOT_ATOMIC, "UnsafeLocalAtomicInteger", "increment", 23);
210+
}
211+
212+
@Test
213+
void testSafePresynchronizedPrivateMethod() {
214+
performAnalysis("atomicMethods/SafePresynchronizedPrivateMethod.class");
215+
assertZeroBugs();
216+
}
217+
218+
@Test
219+
void testSafeAtomicValueUsage() {
220+
performAnalysis("atomicMethods/SafeAtomicValueUsage.class");
221+
assertZeroBugs();
222+
}
223+
224+
@Test
225+
void testSafeAtomicInFinallyBlock() {
226+
performAnalysis("atomicMethods/SafeAtomicInFinallyBlock.class");
227+
assertZeroBugs();
228+
}
229+
230+
@Test
231+
void testSafeAtomicCallsWithLambda() {
232+
performAnalysis("atomicMethods/SafeAtomicCallsWithLambda.class");
233+
assertZeroBugs();
234+
}
235+
236+
private void assertZeroBugs() {
237+
assertNumOfBugs(OPERATIONS_ARE_NOT_ATOMIC, 0);
238+
assertNumOfBugs(NEEDS_SYNCHRONIZATION, 0);
239+
}
240+
241+
private void assertNumOfBugs(String bugType, int num) {
242+
final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
243+
.bugType(bugType).build();
244+
assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher));
245+
}
246+
247+
private void assertBug(String bugType, String className, String methodName, int line) {
248+
final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
249+
.bugType(bugType)
250+
.inClass(className)
251+
.inMethod(methodName)
252+
.atLine(line)
253+
.build();
254+
assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
255+
}
256+
}

‎spotbugs/etc/findbugs.xml

+4
Original file line numberDiff line numberDiff line change
@@ -644,6 +644,8 @@
644644
reports="UR_UNINIT_READ_CALLED_FROM_SUPER_CONSTRUCTOR"/>
645645
<Detector class="edu.umd.cs.findbugs.detect.AtomicityProblem"
646646
reports="AT_OPERATION_SEQUENCE_ON_CONCURRENT_ABSTRACTION"/>
647+
<Detector class="edu.umd.cs.findbugs.detect.AtomicOperationsCombinedDetector"
648+
reports="AT_COMBINED_ATOMIC_OPERATIONS_ARE_NOT_ATOMIC,AT_ATOMIC_OPERATION_NEEDS_SYNCHRONIZATION"/>
647649
<Detector class="edu.umd.cs.findbugs.detect.CheckExpectedWarnings"
648650
reports="FB_UNEXPECTED_WARNING,FB_MISSING_EXPECTED_WARNING" disabled="true"
649651
hidden="false"/>
@@ -727,6 +729,8 @@
727729

728730
<BugPattern abbrev="AT" type="AT_OPERATION_SEQUENCE_ON_CONCURRENT_ABSTRACTION"
729731
category="MT_CORRECTNESS"/>
732+
<BugPattern abbrev="AT" type="AT_COMBINED_ATOMIC_OPERATIONS_ARE_NOT_ATOMIC" category="MT_CORRECTNESS"/>
733+
<BugPattern abbrev="AT" type="AT_ATOMIC_OPERATION_NEEDS_SYNCHRONIZATION" category="MT_CORRECTNESS"/>
730734
<BugPattern abbrev="XSS" type="XSS_REQUEST_PARAMETER_TO_SEND_ERROR" category="SECURITY" cweid="81"/>
731735
<BugPattern abbrev="SKIPPED" type="SKIPPED_CLASS_TOO_BIG" category="EXPERIMENTAL"/>
732736
<BugPattern abbrev="XSS" type="XSS_REQUEST_PARAMETER_TO_SERVLET_WRITER"

‎spotbugs/etc/messages.xml

+37
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,15 @@ applied to method parameters and uses of those method parameters. </p>
229229
]]>
230230
</Details>
231231
</Detector>
232+
<Detector class="edu.umd.cs.findbugs.detect.AtomicOperationsCombinedDetector">
233+
<Details>
234+
<![CDATA[
235+
<p> Finds sequences of operations on synchronized/atomic abstractions
236+
like SynchronizedList or AtomicReference that are not in a synchronized block.
237+
</p>
238+
]]>
239+
</Details>
240+
</Detector>
232241
<Detector class="edu.umd.cs.findbugs.detect.SynchronizationOnSharedBuiltinConstant">
233242
<Details>
234243
<![CDATA[
@@ -8672,6 +8681,34 @@ after the call to initLogging, the logger configuration is lost
86728681
]]>
86738682
</Details>
86748683
</BugPattern>
8684+
<BugPattern type="AT_COMBINED_ATOMIC_OPERATIONS_ARE_NOT_ATOMIC">
8685+
<ShortDescription>Sequence of calls on a synchronized abstraction may not be atomic</ShortDescription>
8686+
<LongDescription>Sequence of calls to {3} may not be atomic in {1}</LongDescription>
8687+
<Details>
8688+
<![CDATA[
8689+
<p>This code contains a sequence of calls to a synchronized/atomic abstraction
8690+
(such as a synchronized list or an AtomicReference) without synchronization.
8691+
These calls will not be executed atomically, so it's recommended to put these calls
8692+
into a synchronized block or method.</p>
8693+
<p>See SEI CERT rule
8694+
<a href="https://wiki.sei.cmu.edu/confluence/display/java/VNA03-J.+Do+not+assume+that+a+group+of+calls+to+independently+atomic+methods+is+atomic">
8695+
VNA03-J. Do not assume that a group of calls to independently atomic methods is atomic</a>.
8696+
</p>]]>
8697+
</Details>
8698+
</BugPattern>
8699+
<BugPattern type="AT_ATOMIC_OPERATION_NEEDS_SYNCHRONIZATION">
8700+
<ShortDescription>Atomic operation needs synchronization</ShortDescription>
8701+
<LongDescription>Atomic operation {4} needs synchronization in {1}</LongDescription>
8702+
<Details>
8703+
<![CDATA[
8704+
<p>This operation is atomic, but it needs to be executed in a synchronized block or method,
8705+
because in another method in this class, the object can be modified. To make both atomic, both need synchronization.</p>
8706+
<p>See SEI CERT rule
8707+
<a href="https://wiki.sei.cmu.edu/confluence/display/java/VNA03-J.+Do+not+assume+that+a+group+of+calls+to+independently+atomic+methods+is+atomic">
8708+
VNA03-J. Do not assume that a group of calls to independently atomic methods is atomic</a>.
8709+
</p>]]>
8710+
</Details>
8711+
</BugPattern>
86758712
<BugPattern type="DM_DEFAULT_ENCODING">
86768713
<ShortDescription>Reliance on default encoding</ShortDescription>
86778714
<LongDescription>Found reliance on default encoding in {1}: {2}</LongDescription>

‎spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ca/Call.java

+13-1
Original file line numberDiff line numberDiff line change
@@ -18,17 +18,25 @@
1818
*/
1919
package edu.umd.cs.findbugs.ba.ca;
2020

21+
import java.util.Collections;
22+
import java.util.List;
23+
24+
import edu.umd.cs.findbugs.ba.XField;
25+
2126
public class Call {
2227
private final String className;
2328

2429
private final String methodName;
2530

2631
private final String methodSig;
2732

28-
public Call(String className, String methodName, String methodSig) {
33+
private final List<XField> attributes;
34+
35+
public Call(String className, String methodName, String methodSig, List<XField> attributes) {
2936
this.className = className;
3037
this.methodName = methodName;
3138
this.methodSig = methodSig;
39+
this.attributes = attributes;
3240
}
3341

3442
public String getClassName() {
@@ -43,6 +51,10 @@ public String getMethodSig() {
4351
return methodSig;
4452
}
4553

54+
public List<XField> getAttributes() {
55+
return Collections.unmodifiableList(attributes);
56+
}
57+
4658
@Override
4759
public boolean equals(Object obj) {
4860
if (obj == null || obj.getClass() != this.getClass()) {

0 commit comments

Comments
 (0)