From b1109b58a1ff79a23e6cab070b0873fe657e70fa Mon Sep 17 00:00:00 2001 From: Marco Kaufmann Date: Tue, 14 Feb 2023 10:38:36 +0100 Subject: [PATCH] SONARJAVA-4406 FP on S2142 when the InterruptedException is caught in an inner try-catch --- .../checks/InterruptedExceptionCheck.java | 106 +++++++++++++----- .../checks/InterruptedExceptionCheck.java | 43 +++++-- .../java/checks/helpers/MethodTreeUtils.java | 2 +- 3 files changed, 118 insertions(+), 33 deletions(-) diff --git a/java-checks-test-sources/src/main/java/checks/InterruptedExceptionCheck.java b/java-checks-test-sources/src/main/java/checks/InterruptedExceptionCheck.java index 307bfd299ec..2b8ce0daa43 100644 --- a/java-checks-test-sources/src/main/java/checks/InterruptedExceptionCheck.java +++ b/java-checks-test-sources/src/main/java/checks/InterruptedExceptionCheck.java @@ -25,7 +25,7 @@ public void run1 () { }catch (java.io.IOException e) { LOGGER.log(Level.WARN, "Interrupted!", e); }catch (InterruptedException e) { // Noncompliant [[sc=13;ec=35]] {{Either re-interrupt this method or rethrow the "InterruptedException" that can be caught here.}} - LOGGER.log(Level.WARN, "Interrupted!", e); + LOGGER.log(Level.WARN, "Interrupted!", e); } } @@ -57,10 +57,10 @@ public void runInterrupted() { try { throw new InterruptedException(); } catch (InterruptedException e) { - LOGGER.log(Level.WARN, "Interrupted!", e); - // clean up state... - Thread.currentThread().interrupt(); - } + LOGGER.log(Level.WARN, "Interrupted!", e); + // clean up state... + Thread.currentThread().interrupt(); + } try { while (true) { throw new InterruptedException(); @@ -71,7 +71,7 @@ public void runInterrupted() { // clean up state... new Interruptable().interrupt(); } - } + } public Object getNextTask(BlockingQueue queue) { boolean interrupted = false; @@ -96,14 +96,14 @@ class Interruptable { static final Log LOGGER = null; void interrupt() { - + } private static void waitForNextExecution(Set running, LongSupplier waitTimeoutMillis) { try { Thread.sleep(waitTimeoutMillis.getAsLong()); } catch (InterruptedException e) { //Compliant - cancelAllSubTasksAndInterrupt(running); + cancelAllSubTasksAndInterrupt(running); } } @@ -113,12 +113,12 @@ private static void cancelAllSubTasksAndInterrupt(Set subTasks) { } Thread.currentThread().interrupt(); } - + private static void waitForNextExecution1(Set running, LongSupplier waitTimeoutMillis) { try { Thread.sleep(waitTimeoutMillis.getAsLong()); } catch (InterruptedException e) { // Noncompliant, too many levels - cancelAllSubTasksAndInterrupt1(running); + cancelAllSubTasksAndInterrupt1(running); } } @@ -126,7 +126,7 @@ private static void cancelAllSubTasksAndInterrupt1(Set subTasks) { cancelAllSubTasksAndInterrupt2(subTasks); } - private static void cancelAllSubTasksAndInterrupt2(Set subTasks) { + private static void cancelAllSubTasksAndInterrupt2(Set subTasks) { cancelAllSubTasksAndInterrupt3(subTasks); } @@ -134,7 +134,7 @@ private static void cancelAllSubTasksAndInterrupt3(Set subTasks) { cancelAllSubTasksAndInterrupt4(subTasks); if (LOGGER != null )throw new RuntimeException(); } - + private static void cancelAllSubTasksAndInterrupt4(Set subTasks) { for (Runnable task : subTasks) { System.out.println("--- waitForNextExecution: Service interrupted. Cancel execution of task {}."); @@ -275,7 +275,6 @@ public void throwNewInterruptedExceptionFromFunction() throws InterruptedExcepti } public void throwNewCustomizedInterruptedExceptionFromFunction() throws InterruptedException { - try { throwsInterruptedException(); } catch (InterruptedException e) { // Compliant @@ -302,7 +301,6 @@ public void throwNewCustomizedInterruptedExceptionFromFunction() throws Interrup } public void rethrowSameException() throws InterruptedException { - try { throwsInterruptedException(); } catch (InterruptedException e) { // Compliant @@ -340,7 +338,6 @@ public void rethrowSameException() throws InterruptedException { } public void cutControlFlow() throws InterruptedException { - try { throwsInterruptedException(); } catch (InterruptedException e) { // Noncompliant, because neither foo nor bar belong to the control flow of this catch block. @@ -359,11 +356,74 @@ void bar() throws InterruptedException { try { throwsInterruptedException(); } catch (InterruptedException e) { // Noncompliant, because neither foo, nor bar belong to the control flow of this catch block. - Runnable foo = () -> Thread.currentThread().interrupt(); - Action bar = () -> { - throw new InterruptedException(); - }; + Runnable foo = () -> Thread.currentThread().interrupt(); + Action bar = () -> { + throw new InterruptedException(); + }; + } + } + + void falsePositivesSonarjava4406() { + try { + try { + throwsInterruptedException(); + } catch (InterruptedException ie) { // Noncompliant, because interruption state is lost. + } + } catch (Exception e) { // Compliant, because inner try does not throw an InterruptedException + } + + try { + try { + throwsInterruptedException(); + } catch (InterruptedException ie) { // Compliant + Thread.currentThread().interrupt(); + } + } catch (Exception e) { // Compliant, because inner try does not throw an InterruptedException } + + try { + try { + throwsInterruptedException(); + } catch (InterruptedException ie) { // Compliant + throw new InterruptedException(); + } + } catch (InterruptedException e) { // Noncompliant, because explicitly catch InterruptedException + } + + try { + try { + throwsInterruptedException(); + } catch (RuntimeException ie) { // Compliant + } + } catch (Exception e) { // Noncompliant, because inner try may throw an InterruptedException + } + + } + + public void throwInterruptedExceptionFromResourceList() throws InterruptedException { + try { + try(AutoCloseable autocloseable = getAutoCloseableThrowsInterruptedException()) { + doSomething(); + } catch (IOException ie) { + doSomething(); + } + } catch (Exception e) { // Noncompliant + } + } + + public void throwInterruptedExceptionWithinThrowStatement() throws InterruptedException { + try { + throw getRuntimeExceptionThrowsInterruptedException(); + } catch (Exception e) { // Noncompliant + } + } + + private AutoCloseable getAutoCloseableThrowsInterruptedException() throws InterruptedException { + throw getInterruptedException(); + } + + private RuntimeException getRuntimeExceptionThrowsInterruptedException() throws InterruptedException { + throw getInterruptedException(); } @FunctionalInterface @@ -414,15 +474,11 @@ private static void interruptByThrowingCustomizedInterruptedExceptionL4() throws throw new CustomizedInterruptedException(); } - public void throwsException() throws Exception { + public static void throwsException() throws Exception { throw new Exception(); } - public void throwsInterruptedException() throws InterruptedException { - throw new InterruptedException(); - } - - public void throwsCustomizedInterruptedException() throws InterruptedException { + public static void throwsInterruptedException() throws InterruptedException { throw new InterruptedException(); } diff --git a/java-checks/src/main/java/org/sonar/java/checks/InterruptedExceptionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/InterruptedExceptionCheck.java index d205532574b..07ab8190505 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/InterruptedExceptionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/InterruptedExceptionCheck.java @@ -33,12 +33,12 @@ import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.MethodMatchers; -import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.BaseTreeVisitor; import org.sonar.plugins.java.api.tree.BlockTree; import org.sonar.plugins.java.api.tree.CatchTree; import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; import org.sonar.plugins.java.api.tree.MethodInvocationTree; import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.ThrowStatementTree; @@ -84,6 +84,7 @@ public void visitNode(Tree tree) { TryStatementTree tryStatementTree = (TryStatementTree) tree; withinInterruptingFinally.addFirst(isFinallyInterrupting(tryStatementTree.finallyBlock())); + for (CatchTree catchTree : tryStatementTree.catches()) { VariableTree catchParameter = catchTree.parameter(); List caughtTypes = getCaughtTypes(catchParameter); @@ -102,14 +103,14 @@ public void visitNode(Tree tree) { } private void reportIfThrowInterruptInBlock(BlockTree blockTree, CatchTree catchTree) { - MethodTreeUtils.MethodInvocationCollector collector = new MethodTreeUtils.MethodInvocationCollector(InterruptedExceptionCheck::throwInterruptedException); + InterruptingStatementCollector collector = new InterruptingStatementCollector(); blockTree.accept(collector); List invocationInterrupting = collector.getInvocationTree(); if (!invocationInterrupting.isEmpty() && wasNotInterrupted(catchTree)) { reportIssue(catchTree.parameter(), String.format(MESSAGE, "InterruptedException"), invocationInterrupting.stream() - .map(t -> new JavaFileScannerContext.Location("Method invocation throwing InterruptedException.", t)) + .map(t -> new JavaFileScannerContext.Location("Statement throwing InterruptedException.", t)) .collect(Collectors.toList()), null); } @@ -148,9 +149,36 @@ private static boolean isFinallyInterrupting(@Nullable BlockTree blockTree) { return blockVisitor.threadInterrupted; } - private static boolean throwInterruptedException(Symbol.MethodSymbol symbol) { - return !symbol.isUnknown() - && symbol.thrownTypes().stream().anyMatch(INTERRUPTING_TYPE_PREDICATE); + private static boolean isInterruptingExceptionExpression(ExpressionTree expressionTree) { + return INTERRUPTING_TYPE_PREDICATE.test(expressionTree.symbolType()); + } + + private static class InterruptingStatementCollector extends MethodTreeUtils.MethodInvocationCollector { + + public InterruptingStatementCollector() { + super( + symbol -> symbol.thrownTypes().stream().anyMatch(INTERRUPTING_TYPE_PREDICATE) + ); + } + + @Override + public void visitTryStatement(TryStatementTree tryStatementTree) { + // If inner `try` statement catches interrupting types: cut analysis of its try block, because possible + // interruptions thrown there are handled within the scope of the catch block. + // Yet, all other blocks (resources, catch, finally) of inner `try`s must still be analyzed, because possible + // interruptions thrown there must be handled within the scope of the outer try. + + boolean isCatchingAnyInterruptingTypes = tryStatementTree.catches().stream().anyMatch(catchTree -> + getCaughtTypes(catchTree.parameter()).stream().anyMatch(INTERRUPTING_TYPE_PREDICATE) + ); + + scan(tryStatementTree.resourceList()); + if (!isCatchingAnyInterruptingTypes) { + scan(tryStatementTree.block()); + } + scan(tryStatementTree.catches()); + scan(tryStatementTree.finallyBlock()); + } } private static class BlockVisitor extends BaseTreeVisitor { @@ -185,7 +213,7 @@ public void visitMethodInvocation(MethodInvocationTree tree) { @Override public void visitThrowStatement(ThrowStatementTree tree) { - if (threadInterrupted || INTERRUPTING_TYPE_PREDICATE.test(tree.expression().symbolType())) { + if (threadInterrupted || isInterruptingExceptionExpression(tree.expression())) { threadInterrupted = true; } else { super.visitThrowStatement(tree); @@ -202,4 +230,5 @@ public void visitLambdaExpression(LambdaExpressionTree tree) { // Cut visit on lambdas, because we only want to analyze actual control flow. } } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java index 12817e70e3f..6e4bb97a332 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java @@ -139,7 +139,7 @@ static boolean hasKind(@Nullable Tree tree, Tree.Kind kind) { } public static class MethodInvocationCollector extends BaseTreeVisitor { - private final List invocationTree = new ArrayList<>(); + protected final List invocationTree = new ArrayList<>(); private final Predicate collectPredicate; public MethodInvocationCollector(Predicate collectPredicate) {