SONARJAVA-5553 Raise only when returned mutable actually reaches outside the class for S2384 (#5136)

Author: romainbrenguier

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

@@ -88,8 +88,7 @@
 196
 ],
 "org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java": [
-354,
-441
+354
 ],
 "org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java": [
 343,

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

@@ -88,8 +88,7 @@
 196
 ],
 "org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java": [
-354,
-441
+354
 ],
 "org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java": [
 343,

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

@@ -278,9 +278,6 @@
 "org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/metric/ws/MetricsWs.java": [
 31
 ],
-"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/metric/ws/SearchAction.java": [
-98
-],
 "org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/notification/DefaultNotificationManager.java": [
 59
 ],
@@ -353,15 +350,9 @@
 "org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/project/Visibility.java": [
 68
 ],
-"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/project/ws/GhostsAction.java": [
-149
-],
 "org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/project/ws/ProjectsWs.java": [
 32
 ],
-"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/project/ws/ProvisionedAction.java": [
-150
-],
 "org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/project/ws/SearchMyProjectsData.java": [
 106,
 111,

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

@@ -606,16 +606,51 @@ static SetThroughPrivateMethods ofMap(final Map<String, String> map) {
     return new SetThroughPrivateMethods(null, map);
   }
 
-  public void cycle(List<Integer> l){
+  public void cycle(List<Integer> l) {
     cycleA(l, 5);
   }
-  private void cycleA(List<Integer> l, int i){
+
+  private void cycleA(List<Integer> l, int i) {
     cycleB(l, i);
   }
-  private void cycleB(List<Integer> l, int i){
+
+  private void cycleB(List<Integer> l, int i) {
     if (i > 0) {
       cycleA(l, i - 1);
     }
     list = l; // Noncompliant
   }
 }
+
+class ReturnedAndPassedThrough {
+  private byte[] secureData = new byte[10];
+  private byte[] data = new byte[10];
+
+  private byte[] getDataInternal() {
+    return data; // Noncompliant
+  }
+
+  private byte[] getSecureDataInternal() {
+    return data; // Compliant: only called by `getSecureData()` which performs a copy.
+  }
+
+  public byte[] getSecureData() {
+    return Arrays.copyOf(getSecureDataInternal(), secureData.length);
+  }
+
+  public byte[] getData() {
+    if (secureData.length > 0) {
+      return getDataInternal();
+    } else {
+      return getData();
+    }
+  }
+
+  public byte[] getDataWithTwoReturns() {
+    if (data.length > 5) {
+      return data; // Noncompliant
+    }
+    return data; // Noncompliant
+  }
+}
+

java-checks/src/main/java/org/sonar/java/checks/MutableMembersUsageCheck.java

@@ -93,11 +93,14 @@ public class MutableMembersUsageCheck extends BaseTreeVisitor implements JavaFil
 
   private final Deque<String> methodSignatureStack = new ArrayDeque<>();
 
-  /** In the context of a method call, this means the argument at {@code argumentIndex}, is given by the
+  /**
+   * In the context of a method call, this means the argument at {@code argumentIndex}, is given by the
    * calling method parameter at {@code parameterIndex}. For instance, in {@code int f(int a, int b) { g(0, 1, a); }}, we would have
-   * a mapping (0, 2): parameter 0 of `f`, i.e. a, is used as argument 2 of `g`. */
+   * a mapping (0, 2): parameter 0 of `f`, i.e. a, is used as argument 2 of `g`.
+   */
   @VisibleForTesting
-  record ArgumentParameterMapping(int parameterIndex, int argumentIndex) {}
+  record ArgumentParameterMapping(int parameterIndex, int argumentIndex) {
+  }
 
   /**
    * Maps index of arguments at the call site to parameters of the calling method.
@@ -150,10 +153,22 @@ private static class MutableDataPropagationGraph {
      */
     private final Set<String> nonPrivateMethods = new HashSet<>();
 
+    /**
+     * Maps method that return the result of another method
+     */
+    private final Map<String, List<String>> passingThroughMethod = new HashMap<>();
+
+    /**
+     * Map methods that return private mutable values to the identifier of the mutable they are returning.
+     */
+    private final Map<String, List<IdentifierTree>> methodsReturningPrivateMutable = new HashMap<>();
+
     public void clear() {
       mutableStoredByMethod.clear();
       callGraph.clear();
       nonPrivateMethods.clear();
+      passingThroughMethod.clear();
+      methodsReturningPrivateMutable.clear();
     }
 
     private static final List<CallSite> EMPTY_CALL_SITE_LIST = new ArrayList<>();
@@ -220,6 +235,26 @@ public void reportMutableStoreReachableByOutsideCall(JavaFileScannerContext cont
       }
     }
 
+    private void reportMutableFieldReachingToOutside(JavaFileScannerContext context, JavaFileScanner check) {
+      Set<IdentifierTree> mutableValuesToReport = new HashSet<>();
+      ArrayDeque<String> queue = new ArrayDeque<>(nonPrivateMethods);
+      Set<String> explored = new HashSet<>();
+
+      while (!queue.isEmpty()) {
+        String current = queue.pop();
+        if (methodsReturningPrivateMutable.containsKey(current)) {
+          mutableValuesToReport.addAll(methodsReturningPrivateMutable.get(current));
+        }
+        if (explored.add(current)) {
+          queue.addAll(passingThroughMethod.getOrDefault(current, new ArrayList<>()));
+        }
+      }
+
+      for (IdentifierTree identifierTree : mutableValuesToReport) {
+        context.reportIssue(check, identifierTree, "Return a copy of \"" + identifierTree.name() + "\".");
+      }
+    }
+
     /**
      * Record whether the method is callable from outside the class.
      */
@@ -238,7 +273,6 @@ public void addMethodInvocation(Symbol.MethodSymbol methodSymbol, Arguments argu
         .add(new CallSite(methodSymbol.signature(), mutableParameters));
     }
 
-
     /**
      * Returns the indexes of arguments to the indexes of mutable parameters that correspond.
      */
@@ -263,6 +297,17 @@ public void addStore(String methodSignature, IdentifierTree assignedMember, int
       mutableStoredByMethod.computeIfAbsent(methodSignature, k -> new ArrayList<>())
         .add(new ParameterStore(assignedMember, parameterIndex));
     }
+
+    private void addPassingThroughMethod(String callingMethodSignature, String calledMethodSignature) {
+      passingThroughMethod.computeIfAbsent(callingMethodSignature, key -> new ArrayList<>())
+        .add(calledMethodSignature);
+    }
+
+    private void addMethodReturningPrivateMutable(String methodSignature, IdentifierTree mutableIdentifier) {
+      methodsReturningPrivateMutable
+        .computeIfAbsent(methodSignature, key -> new ArrayList<>())
+        .add(mutableIdentifier);
+    }
   }
 
   private final MutableDataPropagationGraph dataPropagationGraph = new MutableDataPropagationGraph();
@@ -272,6 +317,7 @@ public void scanFile(final JavaFileScannerContext context) {
     this.context = context;
     scan(context.getTree());
     dataPropagationGraph.reportMutableStoreReachableByOutsideCall(context, this);
+    dataPropagationGraph.reportMutableFieldReachingToOutside(context, this);
     dataPropagationGraph.clear();
   }
 
@@ -366,10 +412,15 @@ private void checkReturnedExpression(ExpressionTree expression) {
     }
     if (expression.is(Tree.Kind.IDENTIFIER)) {
       IdentifierTree identifierTree = (IdentifierTree) expression;
-      if (identifierTree.symbol().isPrivate() && !isOnlyAssignedImmutableVariable((Symbol.VariableSymbol) identifierTree.symbol())) {
-        context.reportIssue(this, identifierTree, "Return a copy of \"" + identifierTree.name() + "\".");
+      if (identifierTree.symbol().isPrivate()
+        && !isOnlyAssignedImmutableVariable((Symbol.VariableSymbol) identifierTree.symbol())
+        && !methodSignatureStack.isEmpty()) {
+        dataPropagationGraph.addMethodReturningPrivateMutable(methodSignatureStack.peek(), identifierTree);
       }
     }
+    if (expression instanceof MethodInvocationTree methodInvocationTree && !methodSignatureStack.isEmpty()) {
+      dataPropagationGraph.addPassingThroughMethod(methodSignatureStack.peek(), methodInvocationTree.methodSymbol().signature());
+    }
   }
 
   private static boolean isOnlyAssignedImmutableVariable(Symbol.VariableSymbol symbol) {