SONARJAVA-5153: S1989 should not raise issue if exception is caught by try/catch block (#5314)

Author: anton-haubner-sonarsource

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

@@ -1,17 +1,21 @@
 package checks;
 
-import play.Logger;
-
+import io.vavr.control.Try;
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.io.OutputStream;
+import java.io.Writer;
 import java.net.InetAddress;
+import java.util.function.Consumer;
 import javax.naming.NamingException;
 import javax.servlet.ServletException;
 import javax.servlet.annotation.WebServlet;
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-import java.util.function.Consumer;
-import io.vavr.control.Try;
+import play.Logger;
+
+// @formatter:off
 
 // http://localhost:9090/securityapp/s1989/noncompliantvavr
 @WebServlet(urlPatterns = "/s1989/noncompliantvavr")
@@ -146,3 +150,187 @@ protected void doPut(jakarta.servlet.http.HttpServletRequest request, jakarta.se
     }
   }
 }
+
+// @formatter:on
+
+class ShouldNotRaiseIfOuterTryCatchesException extends HttpServlet {
+  @Override
+  protected void doGet(HttpServletRequest request, HttpServletResponse response) {
+    try (Writer writer = response.getWriter()) {
+      try {
+      } catch (ArrayIndexOutOfBoundsException e) {
+      }
+
+      // This is a regression test:
+      // There used to be an FP here because the handling of the inner catch erased the information about the outer one.
+      // There is no issue here, because the IOException thrown by the method below is caught by the outer try-catch
+      writer.write("Just writing stuff."); // Compliant
+    } catch (IOException e) {
+    }
+  }
+}
+
+class ShouldHandleNestedTryCatchConstructs {
+  static class ShouldDetectUncaughtExceptionInDoublyNestedTryCatch extends HttpServlet {
+    @Override
+    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException {
+      try {
+        try {
+          throwIOException(); // Noncompliant {{Handle the following exception that could be thrown by "throwIOException": IOException.}}
+        } catch (ArrayIndexOutOfBoundsException e) {
+        }
+      } catch (IllegalArgumentException e) {
+      }
+    }
+  }
+
+  static class ShouldNotRaiseForCaughtExceptionInDoublyNestedTryCatch extends HttpServlet {
+    @Override
+    protected void doGet(HttpServletRequest request, HttpServletResponse response) {
+      try {
+        try {
+          throwIOException(); // Compliant
+        } catch (ArrayIndexOutOfBoundsException e) {
+        }
+      } catch (IOException e) {
+      }
+    }
+
+    @Override
+    protected void doPost(HttpServletRequest request, HttpServletResponse response) {
+      try {
+        try {
+          throwIOException(); // Compliant
+        } catch (IOException e) {
+        }
+      } catch (ArrayIndexOutOfBoundsException e) {
+      }
+    }
+  }
+
+  static class ShouldDetectUncaughtExceptionIfTryCatchIsOnLowerLevel extends HttpServlet {
+    @Override
+    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException {
+      try {
+        throwIOException(); // Compliant
+      } catch (IOException e) {
+      }
+
+      throwIOException(); // Noncompliant {{Handle the following exception that could be thrown by "throwIOException": IOException.}}
+    }
+
+    @Override
+    protected void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException {
+      throwIOException(); // Noncompliant {{Handle the following exception that could be thrown by "throwIOException": IOException.}}
+
+      try {
+        throwIOException(); // Compliant
+      } catch (IOException e) {
+      }
+    }
+
+    @Override
+    protected void doPut(HttpServletRequest req, HttpServletResponse resp) throws IOException {
+      try {
+        throwIOException(); // Noncompliant {{Handle the following exception that could be thrown by "throwIOException": IOException.}}
+
+        try {
+          throwIOException(); // Compliant
+        } catch (IOException e) {
+        }
+      } catch (ArrayIndexOutOfBoundsException e) {
+
+      }
+    }
+  }
+
+  static class ShouldDetectForMixedExceptions extends HttpServlet {
+    @Override
+    protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
+      try {
+        try {
+          throwIOException(); // Compliant: Caught by inner try-catch
+          throwServletException(); // Compliant: Caught by outer try-catch
+        } catch (IOException e) {
+
+        }
+
+        throwIOException(); // Noncompliant {{Handle the following exception that could be thrown by "throwIOException": IOException.}}
+      } catch (ServletException e) {
+
+      }
+    }
+  }
+
+  static class ShouldDetectAcrossTryWithResources extends HttpServlet {
+    @Override
+    protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException {
+      try {
+        throwServletException();
+        try (OutputStream stream = new ByteArrayOutputStream()) {
+          stream.write(42); // Noncompliant {{Handle the following exception that could be thrown by "write": IOException.}}
+        }
+      } catch (ServletException e) {
+
+      }
+    }
+
+    @Override
+    protected void doGet(HttpServletRequest req, HttpServletResponse resp) {
+      try {
+        try (OutputStream stream = new ByteArrayOutputStream()) {
+          stream.write(42); // Compliant: Caught by outer try-catch
+        }
+      } catch (IOException e) {
+
+      }
+    }
+
+    @Override
+    protected void doHead(HttpServletRequest req, HttpServletResponse resp) throws IOException {
+      try (OutputStream stream = new ByteArrayOutputStream()) {
+        try {
+          stream.write(42); // Compliant
+        } catch (IOException e) {
+
+        }
+      }
+    }
+  }
+
+  private static void throwIOException() throws IOException {
+    throw new IOException();
+  }
+
+  private static void throwServletException() throws ServletException {
+    throw new ServletException();
+  }
+}
+
+class ShouldHandleMultipleCatchBlocksInSeries extends HttpServlet {
+  @Override
+  protected void doGet(HttpServletRequest req, HttpServletResponse resp) {
+    try {
+      throwIOException(); // Compliant: Caught by second catch block
+    } catch (IllegalArgumentException e) {
+
+    } catch (IOException e) {
+
+    }
+  }
+
+  @Override
+  protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException {
+    try {
+      throwIOException(); // Noncompliant {{Handle the following exception that could be thrown by "throwIOException": IOException.}}
+    } catch (IllegalArgumentException e) {
+
+    } catch (IllegalStateException e) {
+
+    }
+  }
+
+  private static void throwIOException() throws IOException {
+    throw new IOException();
+  }
+}

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

@@ -60,10 +60,10 @@ public void visitNode(Tree tree) {
       shouldCheck.push(IS_SERVLET_DO_METHOD.matches((MethodTree) tree));
     } else if (shouldCheck()) {
       if (tree.is(Tree.Kind.TRY_STATEMENT)) {
-        tryCatches.add(getCaughtExceptions(((TryStatementTree) tree).catches()));
+        tryCatches.push(getCaughtExceptions(((TryStatementTree) tree).catches()));
       } else if (tree.is(Tree.Kind.CATCH)) {
         tryCatches.pop();
-        tryCatches.add(Collections.emptyList());
+        tryCatches.push(Collections.emptyList());
       } else if (tree.is(Tree.Kind.THROW_STATEMENT)) {
         addIssueIfNotCaught(((ThrowStatementTree) tree).expression().symbolType(), tree);
       } else if (tree.is(Tree.Kind.METHOD_INVOCATION)) {