Skip to content

Commit

Permalink
Fix for false positives EI_EXPOSE_REP in case of unmodifiable colle…
Browse files Browse the repository at this point in the history
…ctions (#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>
  • Loading branch information
Balogh, Ádám and KengoTODA committed Sep 1, 2022
1 parent 06a1eeb commit 4c9b635
Show file tree
Hide file tree
Showing 6 changed files with 312 additions and 8 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
- Bump up logback to `1.4.0`
- Bump up log4j2 binding to `2.18.0`
- Fixed InvalidInputException in Eclipse while bug reporting ([#2134](https://github.com/spotbugs/spotbugs/issues/2134))
- 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))
- 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))

## 4.7.1 - 2022-06-26
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package edu.umd.cs.findbugs.detect;

import org.junit.Before;
import org.junit.Test;

import edu.umd.cs.findbugs.AbstractIntegrationTest;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;

import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
import static org.junit.Assume.assumeFalse;
import static org.junit.Assume.assumeThat;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.number.OrderingComparison.greaterThanOrEqualTo;

public class Issue1771Test extends AbstractIntegrationTest {
@Before
public void verifyJavaVersion() {
assumeFalse(System.getProperty("java.specification.version").startsWith("1."));
int javaVersion = Integer.parseInt(System.getProperty("java.specification.version"));
assumeThat(javaVersion, is(greaterThanOrEqualTo(11)));
}

@Test
public void test() {
performAnalysis("../java11/ghIssues/Issue1771.class");
BugInstanceMatcher matcher = new BugInstanceMatcherBuilder()
.bugType("EI_EXPOSE_REP").build();
assertThat(getBugCollection(), containsExactly(0, matcher));
}
}
69 changes: 63 additions & 6 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/OpcodeStack.java
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import org.apache.bcel.classfile.Method;
import org.apache.bcel.generic.BasicType;
import org.apache.bcel.generic.Type;
import org.apache.commons.lang3.tuple.Pair;

import edu.umd.cs.findbugs.OpcodeStack.Item.SpecialKind;
import edu.umd.cs.findbugs.StackMapAnalyzer.JumpInfoFromStackMap;
Expand Down Expand Up @@ -129,6 +130,38 @@ public class OpcodeStack {

private static final boolean DEBUG2 = DEBUG;

private static final Map<Pair<String, String>, String> IMMUTABLE_RETURNER_MAP = new HashMap<>();
static {
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptyList"), "Ljava/util/Collections$EmptyList;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptyMap"), "Ljava/util/Collections$EmptyMap;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptyNavigableMap"), "Ljava/util/Collections$EmptyNavigableMap;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptySortedMap"), "Ljava/util/Collections$EmptyNavigableMap;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptySet"), "Ljava/util/Collections$EmptySet;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptyNavigableSet"), "Ljava/util/Collections$EmptyNavigableSet;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "emptySortedSet"), "Ljava/util/Collections$EmptyNavigableSet;");

IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "singletonList"), "Ljava/util/Collections$SingletonList;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "singletonMap"), "Ljava/util/Collections$SingletonMap;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "singleton"), "Ljava/util/Collections$SingletonSet;");

IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableList"), "Ljava/util/Collections$UnmodifiableList;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableMap"), "Ljava/util/Collections$UnmodifiableMap;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableNavigableMap"), "Ljava/util/Collections$UnmodifiableNavigableMap;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableSortedMap"), "Ljava/util/Collections$UnmodifiableSortedMap;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableSet"), "Ljava/util/Collections$UnmodifiableSet;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableNavigableSet"), "Ljava/util/Collections$UnmodifiableNavigableSet;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Collections", "unmodifiableSortedSet"), "Ljava/util/Collections$UnmodifiableSortedSet;");

IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/List", "of"), "Ljava/util/ImmutableCollections$AbstractImmutableList;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/List", "copyOf"), "Ljava/util/ImmutableCollections$AbstractImmutableList;");

IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Map", "of"), "Ljava/util/ImmutableCollections$AbstractImmutableMap;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Map", "copyOf"), "Ljava/util/ImmutableCollections$AbstractImmutableMap;");

IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Set", "of"), "Ljava/util/ImmutableCollections$AbstractImmutableSet;");
IMMUTABLE_RETURNER_MAP.put(Pair.of("java/util/Set", "copyOf"), "Ljava/util/ImmutableCollections$AbstractImmutableSet;");
}

@StaticConstant
static final HashMap<String, String> boxedTypes = new HashMap<>();

Expand Down Expand Up @@ -2624,15 +2657,35 @@ private void processMethodCall(DismantleBytecode dbc, int seen) {
Item result = new Item(JAVA_UTIL_ARRAYS_ARRAY_LIST);
push(result);
return;
} else if (seen == Const.INVOKESTATIC && "(Ljava/util/List;)Ljava/util/List;".equals(signature)
&& "java/util/Collections".equals(clsName)) {
Item requestParameter = pop();
if (JAVA_UTIL_ARRAYS_ARRAY_LIST.equals(requestParameter.getSignature())) {
Item result = new Item(JAVA_UTIL_ARRAYS_ARRAY_LIST);
} else if (seen == Const.INVOKESTATIC) {
Item requestParameter = null;
if ("(Ljava/util/List;)Ljava/util/List;".equals(signature)
&& "java/util/Collections".equals(clsName)) {
requestParameter = top();
}
String returnTypeName = IMMUTABLE_RETURNER_MAP.get(Pair.of(clsName, methodName));
if (returnTypeName != null) {
SignatureParser sp = new SignatureParser(signature);
for (int i = 0; i < sp.getNumParameters(); ++i) {
pop();
}
Item result;
if (requestParameter != null && JAVA_UTIL_ARRAYS_ARRAY_LIST.equals(requestParameter.getSignature())) {
result = new Item("Ljava/util/Collections$UnmodifiableRandomAccessList");
} else {
result = new Item(returnTypeName);
}
push(result);
return;
} else if (requestParameter != null) {
pop();
if (JAVA_UTIL_ARRAYS_ARRAY_LIST.equals(requestParameter.getSignature())) {
Item result = new Item(JAVA_UTIL_ARRAYS_ARRAY_LIST);
push(result);
return;
}
push(requestParameter); // fall back to standard logic
}
push(requestParameter); // fall back to standard logic
}

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

private Item top() {
return stack.get(stack.size() - 1);
}

public void replace(int stackOffset, Item value) {
if (stackOffset < 0 || stackOffset >= stack.size()) {
AnalysisContext.logError("Can't get replace stack offset " + stackOffset + " from " + stack.toString() + " @ " + v.getPC()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import edu.umd.cs.findbugs.OpcodeStack;
import edu.umd.cs.findbugs.ba.AnalysisContext;
import edu.umd.cs.findbugs.ba.XField;
import edu.umd.cs.findbugs.ba.type.TypeFrameModelingVisitor;
import edu.umd.cs.findbugs.classfile.CheckedAnalysisException;
import edu.umd.cs.findbugs.classfile.ClassDescriptor;
import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
Expand Down Expand Up @@ -196,7 +197,7 @@ public void sawOpcode(int seen) {
field.isPublic() ||
AnalysisContext.currentXFactory().isEmptyArrayField(field) ||
field.getName().indexOf("EMPTY") != -1 ||
!MutableClasses.mutableSignature(field.getSignature())) {
!MutableClasses.mutableSignature(TypeFrameModelingVisitor.getType(field).getSignature())) {
return;
}
bugAccumulator.accumulateBug(new BugInstance(this, (staticMethod ? "MS" : "EI") + "_EXPOSE_"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,25 @@ public class MutableClasses {
"com.google.common.collect.ImmutableSortedMap",
"com.google.common.collect.ImmutableSortedMultiset",
"com.google.common.collect.ImmutableSortedSet",
"com.google.common.collect.ImmutableTable"));
"com.google.common.collect.ImmutableTable",
"java.util.Collections$EmptyList",
"java.util.Collections$EmptyMap",
"java.util.Collections$EmptyNavigableMap",
"java.util.Collections$EmptySet",
"java.util.Collections$EmptyNavigableSet",
"java.util.Collections$SingletonList",
"java.util.Collections$SingletonMap",
"java.util.Collections$SingletonSet",
"java.util.Collections$UnmodifiableList",
"java.util.Collections$UnmodifiableMap",
"java.util.Collections$UnmodifiableNavigableMap",
"java.util.Collections$UnmodifiableSortedMap",
"java.util.Collections$UnmodifiableSet",
"java.util.Collections$UnmodifiableNavigableSet",
"java.util.Collections$UnmodifiableSortedSet",
"java.util.ImmutableCollections$AbstractImmutableList",
"java.util.ImmutableCollections$AbstractImmutableMap",
"java.util.ImmutableCollections$AbstractImmutableSet"));

private static final Set<String> KNOWN_IMMUTABLE_PACKAGES = new HashSet<>(Arrays.asList(
"java.math", "java.time"));
Expand Down
195 changes: 195 additions & 0 deletions spotbugsTestCases/src/java11/Issue1771.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
package ghIssues;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.NavigableMap;
import java.util.NavigableSet;
import java.util.Set;
import java.util.SortedSet;
import java.util.SortedMap;
import java.util.TreeMap;
import java.util.TreeSet;

public class Issue1771 {
public Issue1771(List<String> list, Map<String, String> map, Set<String> set) {
this.list = List.copyOf(list);
this.map = Map.copyOf(map);
this.set = Set.copyOf(set);

list2 = List.of("foo", "bar");
map2 = Map.of("FOO", "foo", "BAR", "bar");
set2 = Set.of("foo", "bar");

List<String> l = new ArrayList<>();
l.add("foo");
l.add("bar");

Map<String, String> m = new HashMap();
m.put("FOO", "foo");
m.put("BAR", "bar");

NavigableMap<String, String> nm = new TreeMap();
nm.put("FOO", "foo");
nm.put("BAR", "bar");

SortedMap<String, String> sm = new TreeMap();
sm.put("FOO", "foo");
sm.put("BAR", "bar");

Set<String> s = new HashSet();
s.add("foo");
s.add("bar");

NavigableSet<String> ns = new TreeSet();
ns.add("foo");
ns.add("bar");

SortedSet<String> ss = new TreeSet();
ss.add("foo");
ss.add("bar");

list3 = Collections.unmodifiableList(l);
map3 = Collections.unmodifiableMap(m);
navigableMap3 = Collections.unmodifiableNavigableMap(nm);
sortedMap3 = Collections.unmodifiableSortedMap(sm);
set3 = Collections.unmodifiableSet(s);
navigableSet3 = Collections.unmodifiableNavigableSet(ns);
sortedSet3 = Collections.unmodifiableSortedSet(ss);

list4 = Collections.singletonList("foo");
map4 = Collections.singletonMap("FOO", "foo");
set4 = Collections.singleton("foo");

list5 = Collections.emptyList();
map5 = Collections.emptyMap();
navigableMap5 = Collections.emptyNavigableMap();
sortedMap5 = Collections.emptySortedMap();
set5 = Collections.emptySet();
navigableSet5 = Collections.emptyNavigableSet();
sortedSet5 = Collections.emptySortedSet();
}

private final List<String> list;
private final Map<String, String> map;
private final Set<String> set;

private final List<String> list2;
private final Map<String, String> map2;
private final Set<String> set2;

private final List<String> list3;
private final Map<String, String> map3;
private final NavigableMap<String, String> navigableMap3;
private final SortedMap<String, String> sortedMap3;
private final Set<String> set3;
private final NavigableSet<String> navigableSet3;
private final SortedSet<String> sortedSet3;

private final List<String> list4;
private final Map<String, String> map4;
private final Set<String> set4;

private final List<String> list5;
private final Map<String, String> map5;
private final NavigableMap<String, String> navigableMap5;
private final SortedMap<String, String> sortedMap5;
private final Set<String> set5;
private final NavigableSet<String> navigableSet5;
private final SortedSet<String> sortedSet5;

public List<String> getList() {
return list;
}

public Map<String, String> getMap() {
return map;
}

public Set<String> getSet() {
return set;
}

public List<String> getList2() {
return list2;
}

public Map<String, String> getMap2() {
return map2;
}

public Set<String> getSet2() {
return set2;
}

public List<String> getList3() {
return list3;
}

public Map<String, String> getMap3() {
return map3;
}

public NavigableMap<String, String> getNavigableMap3() {
return navigableMap3;
}

public SortedMap<String, String> getSortedMap3() {
return sortedMap3;
}

public Set<String> getSet3() {
return set3;
}

public NavigableSet<String> getNavigableSet3() {
return navigableSet3;
}

public SortedSet<String> getSortedSet3() {
return sortedSet3;
}

public List<String> getList4() {
return list4;
}

public Map<String, String> getMap4() {
return map4;
}

public Set<String> getSet4() {
return set4;
}

public List<String> getList5() {
return list5;
}

public Map<String, String> getMap5() {
return map5;
}

public NavigableMap<String, String> getNavigableMap5() {
return navigableMap5;
}

public SortedMap<String, String> getSortedMap5() {
return sortedMap5;
}

public Set<String> getSet5() {
return set5;
}

public NavigableSet<String> getNavigableSet5() {
return navigableSet5;
}

public SortedSet<String> getSortedSet5() {
return sortedSet5;
}
}

0 comments on commit 4c9b635

Please sign in to comment.