Skip to content

Commit 4c9b635

Browse files
Balogh, ÁdámKengoTODA
Balogh, Ádám
and
KengoTODA
authored
Fix for false positives EI_EXPOSE_REP in case of unmodifiable collections (#2141)
* Fix for false positives `EI_EXPOSE_REP` in case of unmodifiable collections The methods `of` and `copyOf` of `List`, `Map` and `Set` return an unmodifiable collection. Thus if final fields are initiablized using these methods then returning them is not dangerous. * Added handling of methods from java.util.Collestions Co-authored-by: Kengo TODA <skypencil@gmail.com>
1 parent 06a1eeb commit 4c9b635

6 files changed

Lines changed: 312 additions & 8 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
1111
- Bump up logback to `1.4.0`
1212
- Bump up log4j2 binding to `2.18.0`
1313
- Fixed InvalidInputException in Eclipse while bug reporting ([#2134](https://github.com/spotbugs/spotbugs/issues/2134))
14+
- Fixed false positives `EI_EXPOSE_REP` thrown in case of fields initialized by the `of` or `copyOf` method of a `List`, `Map` or `Set` ([#1771](https://github.com/spotbugs/spotbugs/issues/1771))
1415
- Fixed CFGBuilderException thrown when `dup_x2` is used to swap the reference and wide-value (double, long) in the stack ([#2146](https://github.com/spotbugs/spotbugs/pull/2146))
1516

1617
## 4.7.1 - 2022-06-26

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package edu.umd.cs.findbugs.detect;
2+
3+
import org.junit.Before;
4+
import org.junit.Test;
5+
6+
import edu.umd.cs.findbugs.AbstractIntegrationTest;
7+
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
8+
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
9+
10+
import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
11+
import static org.junit.Assume.assumeFalse;
12+
import static org.junit.Assume.assumeThat;
13+
import static org.hamcrest.MatcherAssert.assertThat;
14+
import static org.hamcrest.Matchers.is;
15+
import static org.hamcrest.number.OrderingComparison.greaterThanOrEqualTo;
16+
17+
public class Issue1771Test extends AbstractIntegrationTest {
18+
@Before
19+
public void verifyJavaVersion() {
20+
assumeFalse(System.getProperty("java.specification.version").startsWith("1."));
21+
int javaVersion = Integer.parseInt(System.getProperty("java.specification.version"));
22+
assumeThat(javaVersion, is(greaterThanOrEqualTo(11)));
23+
}
24+
25+
@Test
26+
public void test() {
27+
performAnalysis("../java11/ghIssues/Issue1771.class");
28+
BugInstanceMatcher matcher = new BugInstanceMatcherBuilder()
29+
.bugType("EI_EXPOSE_REP").build();
30+
assertThat(getBugCollection(), containsExactly(0, matcher));
31+
}
32+
}

‎spotbugs/src/main/java/edu/umd/cs/findbugs/OpcodeStack.java‎

Lines changed: 63 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
import org.apache.bcel.classfile.Method;
6161
import org.apache.bcel.generic.BasicType;
6262
import org.apache.bcel.generic.Type;
63+
import org.apache.commons.lang3.tuple.Pair;
6364

6465
import edu.umd.cs.findbugs.OpcodeStack.Item.SpecialKind;
6566
import edu.umd.cs.findbugs.StackMapAnalyzer.JumpInfoFromStackMap;
@@ -129,6 +130,38 @@ public class OpcodeStack {
129130

130131
private static final boolean DEBUG2 = DEBUG;
131132

133+
private static final Map<Pair<String, String>, String> IMMUTABLE_RETURNER_MAP = new HashMap<>();
134+
static {
135+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptyList"), "Ljava/util/Collections$EmptyList;");
136+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptyMap"), "Ljava/util/Collections$EmptyMap;");
137+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptyNavigableMap"), "Ljava/util/Collections$EmptyNavigableMap;");
138+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptySortedMap"), "Ljava/util/Collections$EmptyNavigableMap;");
139+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptySet"), "Ljava/util/Collections$EmptySet;");
140+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptyNavigableSet"), "Ljava/util/Collections$EmptyNavigableSet;");
141+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptySortedSet"), "Ljava/util/Collections$EmptyNavigableSet;");
142+
143+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "singletonList"), "Ljava/util/Collections$SingletonList;");
144+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "singletonMap"), "Ljava/util/Collections$SingletonMap;");
145+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "singleton"), "Ljava/util/Collections$SingletonSet;");
146+
147+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableList"), "Ljava/util/Collections$UnmodifiableList;");
148+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableMap"), "Ljava/util/Collections$UnmodifiableMap;");
149+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableNavigableMap"), "Ljava/util/Collections$UnmodifiableNavigableMap;");
150+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableSortedMap"), "Ljava/util/Collections$UnmodifiableSortedMap;");
151+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableSet"), "Ljava/util/Collections$UnmodifiableSet;");
152+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableNavigableSet"), "Ljava/util/Collections$UnmodifiableNavigableSet;");
153+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableSortedSet"), "Ljava/util/Collections$UnmodifiableSortedSet;");
154+
155+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/List", "of"), "Ljava/util/ImmutableCollections$AbstractImmutableList;");
156+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/List", "copyOf"), "Ljava/util/ImmutableCollections$AbstractImmutableList;");
157+
158+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Map", "of"), "Ljava/util/ImmutableCollections$AbstractImmutableMap;");
159+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Map", "copyOf"), "Ljava/util/ImmutableCollections$AbstractImmutableMap;");
160+
161+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Set", "of"), "Ljava/util/ImmutableCollections$AbstractImmutableSet;");
162+
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Set", "copyOf"), "Ljava/util/ImmutableCollections$AbstractImmutableSet;");
163+
}
164+
132165
@StaticConstant
133166
static final HashMap<String, String> boxedTypes = new HashMap<>();
134167

@@ -2624,15 +2657,35 @@ private void processMethodCall(DismantleBytecode dbc, int seen) {
26242657
Item result = new Item(JAVA_UTIL_ARRAYS_ARRAY_LIST);
26252658
push(result);
26262659
return;
2627-
} else if (seen == Const.INVOKESTATIC && "(Ljava/util/List;)Ljava/util/List;".equals(signature)
2628-
&& "java/util/Collections".equals(clsName)) {
2629-
Item requestParameter = pop();
2630-
if (JAVA_UTIL_ARRAYS_ARRAY_LIST.equals(requestParameter.getSignature())) {
2631-
Item result = new Item(JAVA_UTIL_ARRAYS_ARRAY_LIST);
2660+
} else if (seen == Const.INVOKESTATIC) {
2661+
Item requestParameter = null;
2662+
if ("(Ljava/util/List;)Ljava/util/List;".equals(signature)
2663+
&& "java/util/Collections".equals(clsName)) {
2664+
requestParameter = top();
2665+
}
2666+
String returnTypeName = IMMUTABLE_RETURNER_MAP.get(Pair.of(clsName, methodName));
2667+
if (returnTypeName != null) {
2668+
SignatureParser sp = new SignatureParser(signature);
2669+
for (int i = 0; i < sp.getNumParameters(); ++i) {
2670+
pop();
2671+
}
2672+
Item result;
2673+
if (requestParameter != null && JAVA_UTIL_ARRAYS_ARRAY_LIST.equals(requestParameter.getSignature())) {
2674+
result = new Item("Ljava/util/Collections$UnmodifiableRandomAccessList");
2675+
} else {
2676+
result = new Item(returnTypeName);
2677+
}
26322678
push(result);
26332679
return;
2680+
} else if (requestParameter != null) {
2681+
pop();
2682+
if (JAVA_UTIL_ARRAYS_ARRAY_LIST.equals(requestParameter.getSignature())) {
2683+
Item result = new Item(JAVA_UTIL_ARRAYS_ARRAY_LIST);
2684+
push(result);
2685+
return;
2686+
}
2687+
push(requestParameter); // fall back to standard logic
26342688
}
2635-
push(requestParameter); // fall back to standard logic
26362689
}
26372690

26382691
pushByInvoke(dbc, seen != Const.INVOKESTATIC);
@@ -3172,6 +3225,10 @@ private Item pop() {
31723225
return stack.remove(stack.size() - 1);
31733226
}
31743227

3228+
private Item top() {
3229+
return stack.get(stack.size() - 1);
3230+
}
3231+
31753232
public void replace(int stackOffset, Item value) {
31763233
if (stackOffset < 0 || stackOffset >= stack.size()) {
31773234
AnalysisContext.logError("Can't get replace stack offset " + stackOffset + " from " + stack.toString() + " @ " + v.getPC()

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import edu.umd.cs.findbugs.OpcodeStack;
3737
import edu.umd.cs.findbugs.ba.AnalysisContext;
3838
import edu.umd.cs.findbugs.ba.XField;
39+
import edu.umd.cs.findbugs.ba.type.TypeFrameModelingVisitor;
3940
import edu.umd.cs.findbugs.classfile.CheckedAnalysisException;
4041
import edu.umd.cs.findbugs.classfile.ClassDescriptor;
4142
import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
@@ -196,7 +197,7 @@ public void sawOpcode(int seen) {
196197
field.isPublic() ||
197198
AnalysisContext.currentXFactory().isEmptyArrayField(field) ||
198199
field.getName().indexOf("EMPTY") != -1 ||
199-
!MutableClasses.mutableSignature(field.getSignature())) {
200+
!MutableClasses.mutableSignature(TypeFrameModelingVisitor.getType(field).getSignature())) {
200201
return;
201202
}
202203
bugAccumulator.accumulateBug(new BugInstance(this, (staticMethod ? "MS" : "EI") + "_EXPOSE_"

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

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,25 @@ public class MutableClasses {
3939
"com.google.common.collect.ImmutableSortedMap",
4040
"com.google.common.collect.ImmutableSortedMultiset",
4141
"com.google.common.collect.ImmutableSortedSet",
42-
"com.google.common.collect.ImmutableTable"));
42+
"com.google.common.collect.ImmutableTable",
43+
"java.util.Collections$EmptyList",
44+
"java.util.Collections$EmptyMap",
45+
"java.util.Collections$EmptyNavigableMap",
46+
"java.util.Collections$EmptySet",
47+
"java.util.Collections$EmptyNavigableSet",
48+
"java.util.Collections$SingletonList",
49+
"java.util.Collections$SingletonMap",
50+
"java.util.Collections$SingletonSet",
51+
"java.util.Collections$UnmodifiableList",
52+
"java.util.Collections$UnmodifiableMap",
53+
"java.util.Collections$UnmodifiableNavigableMap",
54+
"java.util.Collections$UnmodifiableSortedMap",
55+
"java.util.Collections$UnmodifiableSet",
56+
"java.util.Collections$UnmodifiableNavigableSet",
57+
"java.util.Collections$UnmodifiableSortedSet",
58+
"java.util.ImmutableCollections$AbstractImmutableList",
59+
"java.util.ImmutableCollections$AbstractImmutableMap",
60+
"java.util.ImmutableCollections$AbstractImmutableSet"));
4361

4462
private static final Set<String> KNOWN_IMMUTABLE_PACKAGES = new HashSet<>(Arrays.asList(
4563
"java.math", "java.time"));

‎spotbugsTestCases/src/java11/Issue1771.java‎

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
package ghIssues;
2+
3+
import java.util.ArrayList;
4+
import java.util.Collections;
5+
import java.util.HashMap;
6+
import java.util.HashSet;
7+
import java.util.List;
8+
import java.util.Map;
9+
import java.util.NavigableMap;
10+
import java.util.NavigableSet;
11+
import java.util.Set;
12+
import java.util.SortedSet;
13+
import java.util.SortedMap;
14+
import java.util.TreeMap;
15+
import java.util.TreeSet;
16+
17+
public class Issue1771 {
18+
public Issue1771(List<String> list, Map<String, String> map, Set<String> set) {
19+
this.list = List.copyOf(list);
20+
this.map = Map.copyOf(map);
21+
this.set = Set.copyOf(set);
22+
23+
list2 = List.of("foo", "bar");
24+
map2 = Map.of("FOO", "foo", "BAR", "bar");
25+
set2 = Set.of("foo", "bar");
26+
27+
List<String> l = new ArrayList<>();
28+
l.add("foo");
29+
l.add("bar");
30+
31+
Map<String, String> m = new HashMap();
32+
m.put("FOO", "foo");
33+
m.put("BAR", "bar");
34+
35+
NavigableMap<String, String> nm = new TreeMap();
36+
nm.put("FOO", "foo");
37+
nm.put("BAR", "bar");
38+
39+
SortedMap<String, String> sm = new TreeMap();
40+
sm.put("FOO", "foo");
41+
sm.put("BAR", "bar");
42+
43+
Set<String> s = new HashSet();
44+
s.add("foo");
45+
s.add("bar");
46+
47+
NavigableSet<String> ns = new TreeSet();
48+
ns.add("foo");
49+
ns.add("bar");
50+
51+
SortedSet<String> ss = new TreeSet();
52+
ss.add("foo");
53+
ss.add("bar");
54+
55+
list3 = Collections.unmodifiableList(l);
56+
map3 = Collections.unmodifiableMap(m);
57+
navigableMap3 = Collections.unmodifiableNavigableMap(nm);
58+
sortedMap3 = Collections.unmodifiableSortedMap(sm);
59+
set3 = Collections.unmodifiableSet(s);
60+
navigableSet3 = Collections.unmodifiableNavigableSet(ns);
61+
sortedSet3 = Collections.unmodifiableSortedSet(ss);
62+
63+
list4 = Collections.singletonList("foo");
64+
map4 = Collections.singletonMap("FOO", "foo");
65+
set4 = Collections.singleton("foo");
66+
67+
list5 = Collections.emptyList();
68+
map5 = Collections.emptyMap();
69+
navigableMap5 = Collections.emptyNavigableMap();
70+
sortedMap5 = Collections.emptySortedMap();
71+
set5 = Collections.emptySet();
72+
navigableSet5 = Collections.emptyNavigableSet();
73+
sortedSet5 = Collections.emptySortedSet();
74+
}
75+
76+
private final List<String> list;
77+
private final Map<String, String> map;
78+
private final Set<String> set;
79+
80+
private final List<String> list2;
81+
private final Map<String, String> map2;
82+
private final Set<String> set2;
83+
84+
private final List<String> list3;
85+
private final Map<String, String> map3;
86+
private final NavigableMap<String, String> navigableMap3;
87+
private final SortedMap<String, String> sortedMap3;
88+
private final Set<String> set3;
89+
private final NavigableSet<String> navigableSet3;
90+
private final SortedSet<String> sortedSet3;
91+
92+
private final List<String> list4;
93+
private final Map<String, String> map4;
94+
private final Set<String> set4;
95+
96+
private final List<String> list5;
97+
private final Map<String, String> map5;
98+
private final NavigableMap<String, String> navigableMap5;
99+
private final SortedMap<String, String> sortedMap5;
100+
private final Set<String> set5;
101+
private final NavigableSet<String> navigableSet5;
102+
private final SortedSet<String> sortedSet5;
103+
104+
public List<String> getList() {
105+
return list;
106+
}
107+
108+
public Map<String, String> getMap() {
109+
return map;
110+
}
111+
112+
public Set<String> getSet() {
113+
return set;
114+
}
115+
116+
public List<String> getList2() {
117+
return list2;
118+
}
119+
120+
public Map<String, String> getMap2() {
121+
return map2;
122+
}
123+
124+
public Set<String> getSet2() {
125+
return set2;
126+
}
127+
128+
public List<String> getList3() {
129+
return list3;
130+
}
131+
132+
public Map<String, String> getMap3() {
133+
return map3;
134+
}
135+
136+
public NavigableMap<String, String> getNavigableMap3() {
137+
return navigableMap3;
138+
}
139+
140+
public SortedMap<String, String> getSortedMap3() {
141+
return sortedMap3;
142+
}
143+
144+
public Set<String> getSet3() {
145+
return set3;
146+
}
147+
148+
public NavigableSet<String> getNavigableSet3() {
149+
return navigableSet3;
150+
}
151+
152+
public SortedSet<String> getSortedSet3() {
153+
return sortedSet3;
154+
}
155+
156+
public List<String> getList4() {
157+
return list4;
158+
}
159+
160+
public Map<String, String> getMap4() {
161+
return map4;
162+
}
163+
164+
public Set<String> getSet4() {
165+
return set4;
166+
}
167+
168+
public List<String> getList5() {
169+
return list5;
170+
}
171+
172+
public Map<String, String> getMap5() {
173+
return map5;
174+
}
175+
176+
public NavigableMap<String, String> getNavigableMap5() {
177+
return navigableMap5;
178+
}
179+
180+
public SortedMap<String, String> getSortedMap5() {
181+
return sortedMap5;
182+
}
183+
184+
public Set<String> getSet5() {
185+
return set5;
186+
}
187+
188+
public NavigableSet<String> getNavigableSet5() {
189+
return navigableSet5;
190+
}
191+
192+
public SortedSet<String> getSortedSet5() {
193+
return sortedSet5;
194+
}
195+
}

0 commit comments

Comments
 (0)