Skip to content

Commit d5bbf58

Browse files
SONARJAVA-4473 Check that mutable member storage is reachable by public calls for S2384 (#5130)
This is to be more precise and avoid false positives where the storage only happens in private methods, and calls to this private methods by public ones are protected in some way. We also want to reduce false negatives compared to the naive solution to entirely ignore private methods.
1 parent 85f296e commit d5bbf58

7 files changed

Lines changed: 310 additions & 14 deletions

File tree

its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S2384.json

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,12 @@
2727
45
2828
],
2929
"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java": [
30+
217,
3031
223
3132
],
33+
"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java": [
34+
148
35+
],
3236
"org.eclipse.jetty:jetty-project:jetty-jmx/src/main/java/org/eclipse/jetty/jmx/MetaData.java": [
3337
73
3438
],
@@ -84,7 +88,8 @@
8488
196
8589
],
8690
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java": [
87-
354
91+
354,
92+
441
8893
],
8994
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java": [
9095
343,

its/ruling/src/test/resources/eclipse-jetty/java-S2384.json

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,12 @@
2727
45
2828
],
2929
"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java": [
30+
217,
3031
223
3132
],
33+
"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java": [
34+
148
35+
],
3236
"org.eclipse.jetty:jetty-project:jetty-jmx/src/main/java/org/eclipse/jetty/jmx/MetaData.java": [
3337
73
3438
],
@@ -84,7 +88,8 @@
8488
196
8589
],
8690
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java": [
87-
354
91+
354,
92+
441
8893
],
8994
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java": [
9095
343,

its/ruling/src/test/resources/guava/java-S2384.json

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
4498
88
],
99
"com.google.guava:guava:src/com/google/common/collect/AbstractBiMap.java": [
10+
63,
1011
69,
1112
96
1213
],
@@ -31,6 +32,12 @@
3132
"com.google.guava:guava:src/com/google/common/collect/HashBiMap.java": [
3233
684
3334
],
35+
"com.google.guava:guava:src/com/google/common/collect/ImmutableEnumMap.java": [
36+
52
37+
],
38+
"com.google.guava:guava:src/com/google/common/collect/ImmutableEnumSet.java": [
39+
57
40+
],
3441
"com.google.guava:guava:src/com/google/common/collect/ImmutableMapEntrySet.java": [
3542
42
3643
],
@@ -78,9 +85,11 @@
7885
1146
7986
],
8087
"com.google.guava:guava:src/com/google/common/collect/TreeRangeSet.java": [
81-
268
88+
268,
89+
433
8290
],
8391
"com.google.guava:guava:src/com/google/common/collect/WellBehavedMap.java": [
92+
43,
8493
57
8594
],
8695
"com.google.guava:guava:src/com/google/common/escape/ArrayBasedEscaperMap.java": [

its/ruling/src/test/resources/sonar-server/java-S2384.json

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,7 @@
162162
126
163163
],
164164
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/exceptions/BadRequestException.java": [
165+
38,
165166
52
166167
],
167168
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/favorite/ws/FavoritesWs.java": [
@@ -212,6 +213,7 @@
212213
],
213214
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/issue/Result.java": [
214215
70,
216+
95,
215217
119
216218
],
217219
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/issue/index/IssueIteratorForSingleChunk.java": [
@@ -276,6 +278,9 @@
276278
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/metric/ws/MetricsWs.java": [
277279
31
278280
],
281+
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/metric/ws/SearchAction.java": [
282+
98
283+
],
279284
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/notification/DefaultNotificationManager.java": [
280285
59
281286
],
@@ -293,6 +298,7 @@
293298
28
294299
],
295300
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/permission/ApplyPermissionTemplateQuery.java": [
301+
34,
296302
47
297303
],
298304
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/permission/index/PermissionIndexer.java": [
@@ -347,9 +353,15 @@
347353
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/project/Visibility.java": [
348354
68
349355
],
356+
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/project/ws/GhostsAction.java": [
357+
149
358+
],
350359
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/project/ws/ProjectsWs.java": [
351360
32
352361
],
362+
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/project/ws/ProvisionedAction.java": [
363+
150
364+
],
353365
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/project/ws/SearchMyProjectsData.java": [
354366
106,
355367
111,

java-checks-test-sources/default/src/main/java/checks/MutableMembersUsageCheckSample.java

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -564,16 +564,58 @@ public List<String> modifiable() {
564564

565565
class SetThroughPrivateMethods {
566566
private byte[] buf;
567-
private SetThroughPrivateMethods(final byte[] buf) {
568-
this.buf = buf; // Compliant: the constructor is private
567+
private Map<String, String> map;
568+
private List<Integer> list;
569+
570+
private SetThroughPrivateMethods(final byte[] buf, final Map<String, String> map) {
571+
this.map = map; // Noncompliant
572+
this.buf = buf; // Compliant: callers have to go through `of` which makes a clone of the parameter
573+
this.list = new ArrayList<>(); // Compliant: not set from a parameter of this method
569574
}
570575

571576
public static SetThroughPrivateMethods of(final byte[] buf) {
572-
return new SetThroughPrivateMethods(buf.clone());
577+
return new SetThroughPrivateMethods(buf.clone(), Collections.emptyMap());
578+
}
579+
580+
public static SetThroughPrivateMethods of(final byte[] buf, final int off, final int len) {
581+
return of(buf);
573582
}
574583

575-
public void set(final byte[] buf) {
584+
private void set(final byte[] buf) {
585+
// It is possible to call `set` from outside the class using `setBuf`
576586
this.buf = buf; // Noncompliant
577587
}
578588

589+
private void setBuffer(final byte[] buf) {
590+
set(buf);
591+
}
592+
593+
public void setBuf(byte[] buf) {
594+
setBuffer(buf);
595+
}
596+
597+
private void internalSet(byte[] buf) {
598+
this.buf = buf; // Compliant: this is only callable by private methods
599+
}
600+
601+
private void internalSetBuffer(byte[] buf) {
602+
internalSet(buf);
603+
}
604+
605+
static SetThroughPrivateMethods ofMap(final Map<String, String> map) {
606+
return new SetThroughPrivateMethods(null, map);
607+
}
608+
609+
public void cycle(List<Integer> l){
610+
cycleA(l, 5);
611+
}
612+
private void cycleA(List<Integer> l, int i){
613+
cycleB(l, i);
614+
}
615+
private void cycleB(List<Integer> l, int i){
616+
if (i > 0) {
617+
cycleA(l, i - 1);
618+
}
619+
list = l; // Noncompliant
620+
}
579621
}

0 commit comments

Comments
 (0)