Skip to content

Commit b0cccfa

Browse files
PatrikScullyPatrikScully
committed
Move methods to util classes
1 parent 991137b commit b0cccfa

File tree

5 files changed

+126
-77
lines changed

5 files changed

+126
-77
lines changed

‎CHANGELOG.md

+4-4
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,17 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
3333
- Return objects directly instead of creating more garbage collection by defining them ([#3133](https://github.com/spotbugs/spotbugs/issues/3133))
3434
- Cleanup double initization and fix comments referring to findbugs instead of spotbugs([#3134](https://github.com/spotbugs/spotbugs/issues/3134))
3535

36+
### Added
37+
- New detector `ResourceInMultipleThreadsDetector` and introduced new bug type:
38+
- `AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD` is reported in case of unsafe resource access in multiple threads.
39+
3640
## 4.8.6 - 2024-06-17
3741
### Fixed
3842
- 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))
3943
- Do not throw exception when inspecting empty switch statements ([#2995](https://github.com/spotbugs/spotbugs/issues/2995))
4044
- Adjust priority since relaxed mode reports even `IGNORED_PRIORITY` ([#2994](https://github.com/spotbugs/spotbugs/issues/2994))
4145
- Fix duplicated log4j2 jar in distribution ([#3001](https://github.com/spotbugs/spotbugs/issues/3001))
4246

43-
### Added
44-
- New detector `ResourceInMultipleThreadsDetector` and introduced new bug type:
45-
- `AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD` is reported in case of unsafe resource access in multiple threads.
46-
4747
## 4.8.5 - 2024-05-03
4848
### Fixed
4949
- 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/ResourceInMultipleThreadsDetectorTest.java

+16-38
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,6 @@
22

33
import org.junit.jupiter.api.Test;
44
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;
115

126
class ResourceInMultipleThreadsDetectorTest extends AbstractIntegrationTest {
137

@@ -25,65 +19,49 @@ void testSafeUsages() {
2519
"commonResources/CombinedThreadsInShutdownHook$1.class",
2620
"commonResources/Vehicle.class",
2721
"commonResources/Vehicle$Builder.class");
28-
assertNumOfBugs(0);
22+
assertBugTypeCount("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", 0);
2923
}
3024

3125
@Test
3226
void testUnsafeFieldUsages() {
3327
performAnalysis("commonResources/UnsafeFieldUsage.class",
3428
"commonResources/Vehicle.class",
3529
"commonResources/Vehicle$Builder.class");
36-
assertNumOfBugs(5);
37-
assertBug("UnsafeFieldUsage", "lambda$new$0", 8);
38-
assertBug("UnsafeFieldUsage", "createVehicle", 16);
39-
assertBug("UnsafeFieldUsage", "createVehicle", 17);
40-
assertBug("UnsafeFieldUsage", "createVehicle", 18);
41-
assertBug("UnsafeFieldUsage", "createVehicle", 19);
30+
assertBugTypeCount("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", 5);
31+
assertBugInMethodAtLine("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", "UnsafeFieldUsage", "lambda$new$0", 8);
32+
assertBugInMethodAtLine("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", "UnsafeFieldUsage", "createVehicle", 16);
33+
assertBugInMethodAtLine("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", "UnsafeFieldUsage", "createVehicle", 17);
34+
assertBugInMethodAtLine("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", "UnsafeFieldUsage", "createVehicle", 18);
35+
assertBugInMethodAtLine("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", "UnsafeFieldUsage", "createVehicle", 19);
4236
}
4337

4438
@Test
4539
void testUnsafeFieldUsages2() {
4640
performAnalysis("commonResources/UnsafeFieldUsage2.class",
4741
"commonResources/Vehicle.class",
4842
"commonResources/Vehicle$Builder.class");
49-
assertNumOfBugs(2);
50-
assertBug("UnsafeFieldUsage2", "lambda$new$0", 8);
51-
assertBug("UnsafeFieldUsage2", "createVehicle", 16);
43+
assertBugTypeCount("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", 2);
44+
assertBugInMethodAtLine("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", "UnsafeFieldUsage2", "lambda$new$0", 8);
45+
assertBugInMethodAtLine("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", "UnsafeFieldUsage2", "createVehicle", 16);
5246
}
5347

5448
@Test
5549
void testUnsafeFieldUsages3() {
5650
performAnalysis("commonResources/UnsafeFieldUsage3.class",
5751
"commonResources/Vehicle.class",
5852
"commonResources/Vehicle$Builder.class");
59-
assertNumOfBugs(2);
60-
assertBug("UnsafeFieldUsage3", "lambda$new$0", 12);
61-
assertBug("UnsafeFieldUsage3", "createVehicle", 8);
53+
assertBugTypeCount("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", 2);
54+
assertBugInMethodAtLine("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", "UnsafeFieldUsage3", "lambda$new$0", 12);
55+
assertBugInMethodAtLine("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", "UnsafeFieldUsage3", "createVehicle", 8);
6256
}
6357

6458
@Test
6559
void testSynchronizedUnsafeFieldUsage() {
6660
performAnalysis("commonResources/SynchronizedUnsafeFieldUsage.class",
6761
"commonResources/Vehicle.class",
6862
"commonResources/Vehicle$Builder.class");
69-
assertNumOfBugs(2);
70-
assertBug("SynchronizedUnsafeFieldUsage", "lambda$new$0", 8);
71-
assertBug("SynchronizedUnsafeFieldUsage", "createVehicle", 16);
72-
}
73-
74-
private void assertNumOfBugs(int num) {
75-
final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
76-
.bugType("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD").build();
77-
assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher));
78-
}
79-
80-
private void assertBug(String className, String methodName, int line) {
81-
final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
82-
.bugType("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD")
83-
.inClass(className)
84-
.inMethod(methodName)
85-
.atLine(line)
86-
.build();
87-
assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
63+
assertBugTypeCount("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", 2);
64+
assertBugInMethodAtLine("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", "SynchronizedUnsafeFieldUsage", "lambda$new$0", 8);
65+
assertBugInMethodAtLine("AT_UNSAFE_RESOURCE_ACCESS_IN_THREAD", "SynchronizedUnsafeFieldUsage", "createVehicle", 16);
8866
}
8967
}

‎spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ResourceInMultipleThreadsDetector.java

+6-35
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818

1919
package edu.umd.cs.findbugs.detect;
2020

21-
import java.util.Arrays;
2221
import java.util.HashMap;
2322
import java.util.HashSet;
2423
import java.util.Map;
@@ -37,16 +36,15 @@
3736
import edu.umd.cs.findbugs.BugInstance;
3837
import edu.umd.cs.findbugs.BugReporter;
3938
import edu.umd.cs.findbugs.OpcodeStack;
40-
import edu.umd.cs.findbugs.ba.ClassMember;
4139
import edu.umd.cs.findbugs.ba.XField;
4240
import edu.umd.cs.findbugs.ba.XMethod;
4341
import edu.umd.cs.findbugs.bcel.BCELUtil;
4442
import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
4543
import edu.umd.cs.findbugs.classfile.CheckedAnalysisException;
46-
import edu.umd.cs.findbugs.classfile.Global;
4744
import edu.umd.cs.findbugs.classfile.MethodDescriptor;
48-
import edu.umd.cs.findbugs.classfile.engine.bcel.FinallyDuplicatesInfoFactory;
4945
import edu.umd.cs.findbugs.util.BootstrapMethodsUtil;
46+
import edu.umd.cs.findbugs.util.CollectionAnalysis;
47+
import edu.umd.cs.findbugs.util.MethodAnalysis;
5048
import edu.umd.cs.findbugs.util.MutableClasses;
5149

5250
public class ResourceInMultipleThreadsDetector extends OpcodeStackDetector {
@@ -146,17 +144,18 @@ private Optional<MethodDescriptor> getMethodFromBootstrap(JavaClass javaClass, C
146144

147145
private void collectFieldsUsedInThreads(int seen) throws CheckedAnalysisException {
148146
if ((seen == Const.PUTFIELD || seen == Const.PUTSTATIC) && getStack().getStackDepth() > 0
149-
&& !isDuplicatedLocation(getMethodDescriptor(), getPC())
147+
&& !MethodAnalysis.isDuplicatedLocation(getMethodDescriptor(), getPC())
150148
&& methodsUsedInThreads.contains(getMethodDescriptor())) {
151149
OpcodeStack.Item stackItem = getStack().getStackItem(0);
152-
if (stackItem.getReturnValueOf() != null && isSynchronizedCollection(stackItem.getReturnValueOf())) {
150+
if (stackItem.getReturnValueOf() != null && CollectionAnalysis.isSynchronizedCollection(stackItem.getReturnValueOf())) {
153151
synchronizedCollectionTypedFields.add(getXFieldOperand());
154152
} else if (!isAtomicTypedField(getXFieldOperand())
155153
&& !(Const.CONSTRUCTOR_NAME.equals(getMethodName()) || Const.STATIC_INITIALIZER_NAME.equals(getMethodName()))) {
156154
createOrUpdateFieldData(getXFieldOperand(), true, getMethod(), getXMethodOperand());
157155
}
158156
} else if ((seen == Const.INVOKEVIRTUAL || seen == Const.INVOKEINTERFACE || seen == Const.INVOKESPECIAL || seen == Const.INVOKESTATIC)
159-
&& getXMethodOperand() != null && getStack().getStackDepth() > 0 && !isDuplicatedLocation(getMethodDescriptor(), getPC())
157+
&& getXMethodOperand() != null && getStack().getStackDepth() > 0
158+
&& !MethodAnalysis.isDuplicatedLocation(getMethodDescriptor(), getPC())
160159
&& methodsUsedInThreads.contains(getMethodDescriptor())) {
161160
// The field is accessed always be the last item in the stack, because the earlier elements are the arguments
162161
XField xField = getStack().getStackItem(getStack().getStackDepth() - 1).getXField();
@@ -166,34 +165,6 @@ && getXMethodOperand() != null && getStack().getStackDepth() > 0 && !isDuplicate
166165
}
167166
}
168167

169-
/**
170-
* Check if the method is a synchronized collection method.
171-
* @todo: This method should be moved to a utility class when VNA03-J detector is merged
172-
*
173-
* @param classMember the class member
174-
* @return {@code true} if the method is a synchronized collection method, {@code false} otherwise
175-
*/
176-
private static boolean isSynchronizedCollection(ClassMember classMember) {
177-
Set<String> interestingCollectionMethodNames = new HashSet<>(Arrays.asList(
178-
"synchronizedCollection", "synchronizedSet", "synchronizedSortedSet",
179-
"synchronizedNavigableSet", "synchronizedList", "synchronizedMap",
180-
"synchronizedSortedMap", "synchronizedNavigableMap"));
181-
return "java.util.Collections".equals(classMember.getClassName()) && interestingCollectionMethodNames.contains(classMember.getName());
182-
}
183-
184-
/**
185-
* Check if the location is duplicated in the method.
186-
* @todo: This method should be moved to a utility class when VNA03-J detector is merged
187-
*
188-
* @return {@code true} if the location is duplicated in the method, {@code false} otherwise
189-
* @throws CheckedAnalysisException if an error occurs during the analysis
190-
*/
191-
private static boolean isDuplicatedLocation(MethodDescriptor methodDescriptor, int pc) throws CheckedAnalysisException {
192-
FinallyDuplicatesInfoFactory.FinallyDuplicatesInfo methodAnalysis = Global.getAnalysisCache().getMethodAnalysis(
193-
FinallyDuplicatesInfoFactory.FinallyDuplicatesInfo.class, methodDescriptor);
194-
return methodAnalysis.getDuplicates(pc).stream().anyMatch(duplicatePc -> duplicatePc < pc);
195-
}
196-
197168
private boolean isAtomicTypedField(XField xField) {
198169
return xField.getSignature().contains("java/util/concurrent/atomic") || synchronizedCollectionTypedFields.contains(xField);
199170
}

‎spotbugs/src/main/java/edu/umd/cs/findbugs/util/CollectionAnalysis.java

+51
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
/*
2+
* SpotBugs - Find bugs in Java programs
3+
*
4+
* This library is free software; you can redistribute it and/or
5+
* modify it under the terms of the GNU Lesser General Public
6+
* License as published by the Free Software Foundation; either
7+
* version 2.1 of the License, or (at your option) any later version.
8+
*
9+
* This library is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
12+
* Lesser General Public License for more details.
13+
*
14+
* You should have received a copy of the GNU Lesser General Public
15+
* License along with this library; if not, write to the Free Software
16+
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
17+
*/
18+
19+
package edu.umd.cs.findbugs.util;
20+
21+
import java.util.Arrays;
22+
import java.util.HashSet;
23+
import java.util.Set;
24+
25+
import edu.umd.cs.findbugs.ba.ClassMember;
26+
27+
/**
28+
* Utility class for analyzing collections.
29+
*/
30+
public final class CollectionAnalysis {
31+
32+
/**
33+
* Private constructor to prevent instantiation, because it is a utility class.
34+
*/
35+
private CollectionAnalysis() {
36+
}
37+
38+
/**
39+
* Check if a class member is a synchronized collection.
40+
*
41+
* @param classMember the class member
42+
* @return {@code true} if the class member is a synchronized collection, {@code false} otherwise
43+
*/
44+
public static boolean isSynchronizedCollection(ClassMember classMember) {
45+
Set<String> interestingCollectionMethodNames = new HashSet<>(Arrays.asList(
46+
"synchronizedCollection", "synchronizedSet", "synchronizedSortedSet",
47+
"synchronizedNavigableSet", "synchronizedList", "synchronizedMap",
48+
"synchronizedSortedMap", "synchronizedNavigableMap"));
49+
return "java.util.Collections".equals(classMember.getClassName()) && interestingCollectionMethodNames.contains(classMember.getName());
50+
}
51+
}

‎spotbugs/src/main/java/edu/umd/cs/findbugs/util/MethodAnalysis.java

+49
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/*
2+
* SpotBugs - Find bugs in Java programs
3+
*
4+
* This library is free software; you can redistribute it and/or
5+
* modify it under the terms of the GNU Lesser General Public
6+
* License as published by the Free Software Foundation; either
7+
* version 2.1 of the License, or (at your option) any later version.
8+
*
9+
* This library is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
12+
* Lesser General Public License for more details.
13+
*
14+
* You should have received a copy of the GNU Lesser General Public
15+
* License along with this library; if not, write to the Free Software
16+
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
17+
*/
18+
19+
package edu.umd.cs.findbugs.util;
20+
21+
import edu.umd.cs.findbugs.classfile.CheckedAnalysisException;
22+
import edu.umd.cs.findbugs.classfile.Global;
23+
import edu.umd.cs.findbugs.classfile.MethodDescriptor;
24+
import edu.umd.cs.findbugs.classfile.engine.bcel.FinallyDuplicatesInfoFactory;
25+
26+
/**
27+
* Utility class for method analysis.
28+
*/
29+
public final class MethodAnalysis {
30+
31+
/**
32+
* Private constructor to prevent instantiation, because it is a utility class.
33+
*/
34+
private MethodAnalysis() {
35+
}
36+
37+
/**
38+
* Check if the location is duplicated in the method.
39+
* Locations in finally blocks will be duplicated in the bytecode level.
40+
*
41+
* @return {@code true} if the location is duplicated in the method, {@code false} otherwise
42+
* @throws CheckedAnalysisException if an error occurs during the analysis
43+
*/
44+
public static boolean isDuplicatedLocation(MethodDescriptor methodDescriptor, int pc) throws CheckedAnalysisException {
45+
FinallyDuplicatesInfoFactory.FinallyDuplicatesInfo methodAnalysis =
46+
Global.getAnalysisCache().getMethodAnalysis(FinallyDuplicatesInfoFactory.FinallyDuplicatesInfo.class, methodDescriptor);
47+
return methodAnalysis.getDuplicates(pc).stream().anyMatch(duplicatePc -> duplicatePc < pc);
48+
}
49+
}

0 commit comments

Comments
 (0)