Skip to content

Commit 5fe4db7

Browse files
WohopsWohops
committed
SONARJAVA-1225 improve handling of collections
1 parent 87affd1 commit 5fe4db7

5 files changed

Lines changed: 113 additions & 18 deletions

File tree

‎its/ruling/src/test/resources/commons-beanutils/squid-S1948.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@
88
'commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils/BeanComparator.java':[
99
52,
1010
],
11+
'commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils/JDBCDynaClass.java':[
12+
64,
13+
],
1114
'commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils/LazyDynaBean.java':[
1215
145,
1316
154,

‎its/ruling/src/test/resources/jboss-ejb3-tutorial/squid-S1948.json‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,16 @@
33
45,
44
46,
55
],
6+
'jboss-ejb3-tutorial:composite/src/org/jboss/tutorial/composite/bean/Customer.java':[
7+
41,
8+
],
9+
'jboss-ejb3-tutorial:composite/src/org/jboss/tutorial/composite/bean/Flight.java':[
10+
45,
11+
],
12+
'jboss-ejb3-tutorial:relationships/src/org/jboss/tutorial/relationships/bean/Customer.java':[
13+
51,
14+
],
15+
'jboss-ejb3-tutorial:relationships/src/org/jboss/tutorial/relationships/bean/Flight.java':[
16+
51,
17+
],
618
}

‎java-checks/src/main/java/org/sonar/java/checks/serialization/SerializableFieldInSerializableClassCheck.java‎

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package org.sonar.java.checks.serialization;
2121

2222
import com.google.common.collect.ImmutableList;
23+
2324
import org.sonar.api.server.rule.RulesDefinition;
2425
import org.sonar.check.Priority;
2526
import org.sonar.check.Rule;
@@ -29,7 +30,9 @@
2930
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
3031
import org.sonar.plugins.java.api.semantic.Symbol;
3132
import org.sonar.plugins.java.api.semantic.Type;
33+
import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
3234
import org.sonar.plugins.java.api.tree.ClassTree;
35+
import org.sonar.plugins.java.api.tree.ExpressionTree;
3336
import org.sonar.plugins.java.api.tree.IdentifierTree;
3437
import org.sonar.plugins.java.api.tree.MethodTree;
3538
import org.sonar.plugins.java.api.tree.Modifier;
@@ -44,6 +47,7 @@
4447
import org.sonar.squidbridge.annotations.SqaleSubCharacteristic;
4548

4649
import javax.annotation.Nullable;
50+
4751
import java.util.List;
4852

4953
@Rule(
@@ -79,14 +83,47 @@ public void visitNode(Tree tree) {
7983
private void checkVariableMember(VariableTree variableTree) {
8084
if (!isExcluded(variableTree)) {
8185
IdentifierTree simpleName = variableTree.simpleName();
82-
reportIssue(simpleName, "Make \"" + simpleName.name() + "\" transient or serializable.");
86+
if (isCollectionOfSerializable(variableTree.type())) {
87+
if (!ModifiersUtils.hasModifier(variableTree.modifiers(), Modifier.PRIVATE)) {
88+
reportIssue(simpleName, "Make \"" + simpleName.name() + "\" private or transient.");
89+
} else if (isUnserializableCollection(variableTree.type().symbolType())
90+
|| initializerIsUnserializableCollection(variableTree.initializer())) {
91+
reportIssue(simpleName);
92+
}
93+
checkCollectionAssignments(variableTree.symbol().usages());
94+
} else {
95+
reportIssue(simpleName);
96+
}
8397
}
8498
}
8599

100+
private static boolean initializerIsUnserializableCollection(ExpressionTree initializer) {
101+
return initializer != null && isUnserializableCollection(initializer.symbolType());
102+
}
103+
104+
private static boolean isUnserializableCollection(Type type) {
105+
return !type.symbol().isInterface() && isSubtypeOfCollectionApi(type) && !implementsSerializable(type);
106+
}
107+
108+
private void checkCollectionAssignments(List<IdentifierTree> usages) {
109+
for (IdentifierTree usage : usages) {
110+
Tree parentTree = usage.parent();
111+
if (parentTree.is(Tree.Kind.ASSIGNMENT)) {
112+
AssignmentExpressionTree assignment = (AssignmentExpressionTree) parentTree;
113+
ExpressionTree expression = assignment.expression();
114+
if (usage.equals(assignment.variable()) && !expression.is(Tree.Kind.NULL_LITERAL) && isUnserializableCollection(expression.symbolType())) {
115+
reportIssue(usage);
116+
}
117+
}
118+
}
119+
}
120+
121+
private void reportIssue(IdentifierTree tree) {
122+
reportIssue(tree, "Make \"" + tree.name() + "\" transient or serializable.");
123+
}
124+
86125
private static boolean isExcluded(VariableTree variableTree) {
87-
return isStatic(variableTree)
88-
|| isTransientSerializableOrInjected(variableTree)
89-
|| isCollectionOfSerializable(variableTree.type());
126+
return isStatic(variableTree) || isTransientSerializableOrInjected(variableTree);
90127
}
91128

92129
private static boolean isCollectionOfSerializable(TypeTree typeTree) {

‎java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S1948.html‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
<p>Fields in a <code>Serializable</code> class must themselves be either <code>Serializable</code> or <code>transient</code> even if the class is never explicitly serialized or deserialized. That's because under load, most J2EE application frameworks flush objects to disk, and an allegedly <code>Serializable</code> object with non-transient, non-serializable data members could cause program crashes, and open the door to attackers.</p>
2+
<p>This rule raises an issue on non-<code>Serializable</code> fields, and on collection fields when they are not <code>private</code> (because they could be assigned non-<code>Serializable</code> values externally), and when they are assigned non-<code>Serializable</code> types within the class.</p>
23

34
<h2>Noncompliant Code Example</h2>
45
<pre>

‎java-checks/src/test/files/checks/serialization/SerializableFieldInSerializableClassCheck.java‎

Lines changed: 56 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package javax.inject;
22
import java.io.Serializable;
3+
import java.util.ArrayList;
4+
import java.util.HashMap;
35
import java.util.List;
46
import java.util.Map;
57

@@ -40,23 +42,49 @@ class Person5 implements Serializable {
4042
}
4143

4244
class Person6<E, F extends Serializable> implements Serializable {
43-
List<Person6> persons; // Compliant
44-
List things; // Noncompliant {{Make "things" transient or serializable.}}
45-
List<MyObject> objects; // Noncompliant {{Make "objects" transient or serializable.}}
46-
List<? extends MyObject> otherObjects; // Noncompliant {{Make "otherObjects" transient or serializable.}}
47-
List<? extends Person6> otherPersons; // Compliant
48-
List<? extends E> otherThings; // Noncompliant {{Make "otherThings" transient or serializable.}}
49-
List<? extends F> otherSerializableThings; // Compliant
50-
List<?> otherUnknown; // Noncompliant {{Make "otherUnknown" transient or serializable.}}
51-
List<? super F> super1;
52-
List<? super E> super2; // Noncompliant
45+
private List<Person6> persons; // Compliant
46+
private List things; // Noncompliant {{Make "things" transient or serializable.}}
47+
private List<MyObject> objects; // Noncompliant {{Make "objects" transient or serializable.}}
48+
private List<? extends MyObject> otherObjects; // Noncompliant {{Make "otherObjects" transient or serializable.}}
49+
private List<? extends Person6> otherPersons; // Compliant
50+
private List<? extends E> otherThings; // Noncompliant {{Make "otherThings" transient or serializable.}}
51+
private List<? extends F> otherSerializableThings; // Compliant
52+
private List<?> otherUnknown; // Noncompliant {{Make "otherUnknown" transient or serializable.}}
53+
private List<? super F> super1;
54+
private List<? super E> super2; // Noncompliant
55+
56+
public List<Person6> persons1; // Noncompliant {{Make "persons1" private or transient.}}
57+
transient public List<Person6> persons2; // Compliant - transient
58+
private List<Person6> persons3 = new ArrayList<>(); // Compliant - ArrayList is serializable
59+
private List<Person6> persons4 = new MyNonSerializableList<>(); // Noncompliant
5360
}
5461

5562
class Person7 implements Serializable {
56-
Map<Object, Object> both; // Noncompliant {{Make "both" transient or serializable.}}
57-
Map<String, Object> right; // Noncompliant {{Make "right" transient or serializable.}}
58-
Map<Object, String> left; // Noncompliant {{Make "left" transient or serializable.}}
59-
Map<String, String> ok; // Compliant
63+
private Map<Object, Object> both; // Noncompliant {{Make "both" transient or serializable.}}
64+
private Map<String, Object> right; // Noncompliant {{Make "right" transient or serializable.}}
65+
private Map<Object, String> left; // Noncompliant {{Make "left" transient or serializable.}}
66+
private Map<String, String> ok; // Compliant
67+
68+
private Map<String, String> nok1 = new MyNonSerializableMap<>(); // Noncompliant
69+
private MyNonSerializableMap<String, String> nok2; // Noncompliant
70+
71+
void foo() {
72+
ok = new MyNonSerializableMap<>(); // Noncompliant
73+
nok2 = new MyNonSerializableMap<>(); // Noncompliant
74+
ok = nok2; // Noncompliant
75+
ok = null; // Compliant
76+
ok = bar(); // Compliant
77+
ok = MyAbstractNonSerializableMap.foo(); // Noncompliant
78+
ok = new HashMap<>(); // Compliant
79+
ok = unknown(); // Compliant
80+
if (ok.isEmpty()) {
81+
Object myVar = ok;
82+
}
83+
}
84+
85+
Map bar() {
86+
return null;
87+
}
6088
}
6189

6290
class Person8 implements Serializable {
@@ -66,3 +94,17 @@ class Person8 implements Serializable {
6694
class MyObject {
6795

6896
}
97+
98+
class MyNonSerializableList<E> implements List<E> {
99+
MyNonSerializableList() {}
100+
}
101+
102+
class MyNonSerializableMap<K, V> implements Map<K, V> {
103+
MyNonSerializableMap() {}
104+
}
105+
106+
abstract class MyAbstractNonSerializableMap<K,V> extends MyNonSerializableMap<K,V> {
107+
static MyAbstractNonSerializableMap foo() {
108+
return null;
109+
}
110+
}

0 commit comments

Comments
 (0)