From cd5fd4cabfb908532c82bf9edf49991d721a8b17 Mon Sep 17 00:00:00 2001 From: Tomasz Tylenda Date: Tue, 15 Jul 2025 18:00:19 +0200 Subject: [PATCH 1/3] SONARJAVA-5697 S2441 and S2118: Fix FP with missing semantics of Serializable --- .../checks/helpers/ExpressionsHelper.java | 28 +++++++++++++------ ...alizableWriteCheckMissingImportSample.java | 13 +++++++++ ...jectInSessionCheckMissingImportSample.java | 14 ++++++++++ .../NonSerializableWriteCheck.java | 2 +- .../SerializableObjectInSessionCheck.java | 2 +- .../NonSerializableWriteCheckTest.java | 9 ++++++ .../SerializableObjectInSessionCheckTest.java | 9 ++++++ 7 files changed, 66 insertions(+), 11 deletions(-) create mode 100644 java-checks-test-sources/default/src/main/files/non-compiling/checks/serialization/NonSerializableWriteCheckMissingImportSample.java create mode 100644 java-checks-test-sources/default/src/main/files/non-compiling/checks/serialization/SerializableObjectInSessionCheckMissingImportSample.java diff --git a/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java b/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java index f7a97c37359..488bcba7f9a 100644 --- a/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java +++ b/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java @@ -47,6 +47,7 @@ import static org.sonar.java.checks.helpers.ReassignmentFinder.getInitializerOrExpression; import static org.sonar.java.checks.helpers.ReassignmentFinder.getReassignments; +import static org.sonar.java.model.JUtils.hasUnknownTypeInHierarchy; public class ExpressionsHelper { @@ -169,20 +170,26 @@ public List valuePath() { } } - public static boolean isNotSerializable(ExpressionTree expression) { + /** + * Checks if the expression is non-serializable. + * + * @param defaultOnUnknown It will be returned if the result cannot be determined + * due to incomplete semantics. + */ + public static boolean isNotSerializable(ExpressionTree expression, boolean defaultOnUnknown) { Type symbolType = expression.symbolType(); if (symbolType.isUnknown()) { return false; } - return isNonSerializable(symbolType) - || isAssignedToNonSerializable(expression); + return isNonSerializable(symbolType, defaultOnUnknown) + || isAssignedToNonSerializable(expression, defaultOnUnknown); } - private static boolean isNonSerializable(Type type) { + private static boolean isNonSerializable(Type type, boolean defaultOnUnknown) { if (type.isArray()) { - return isNonSerializable(((Type.ArrayType) type).elementType()); + return isNonSerializable(((Type.ArrayType) type).elementType(), defaultOnUnknown); } - if (type.typeArguments().stream().anyMatch(ExpressionsHelper::isNonSerializable)) { + if (type.typeArguments().stream().anyMatch(t -> isNonSerializable(t, defaultOnUnknown))) { return true; } if (type.isPrimitive() || @@ -197,16 +204,19 @@ private static boolean isNonSerializable(Type type) { type.isSubtypeOf("java.util.Enumeration")) { return false; } + if(hasUnknownTypeInHierarchy(type.symbol())) { + return defaultOnUnknown; + } Type erasedType = type.erasure(); - return erasedType.equals(type) || isNonSerializable(erasedType); + return erasedType.equals(type) || isNonSerializable(erasedType, defaultOnUnknown); } - private static boolean isAssignedToNonSerializable(ExpressionTree expression) { + private static boolean isAssignedToNonSerializable(ExpressionTree expression, boolean defaultOnUnknown) { return ExpressionUtils.extractIdentifierSymbol(expression) .filter(symbol -> initializedAndAssignedExpressionStream(symbol) .map(ExpressionTree::symbolType) .filter(Predicate.not(Type::isUnknown)) - .anyMatch(ExpressionsHelper::isNonSerializable)) + .anyMatch(t -> isNonSerializable(t, defaultOnUnknown))) .isPresent(); } diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/serialization/NonSerializableWriteCheckMissingImportSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/serialization/NonSerializableWriteCheckMissingImportSample.java new file mode 100644 index 00000000000..d29583b1289 --- /dev/null +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/serialization/NonSerializableWriteCheckMissingImportSample.java @@ -0,0 +1,13 @@ +package checks.serialization; + +import java.io.IOException; +import java.io.ObjectOutputStream; + +class NonSerializableWriteCheckMissingImportSample { + public record R(String foo, Boolean bar) implements Serializable {} + + public void writeOut(ObjectOutputStream oos) throws IOException { + R r = new R("foo", true); + oos.writeObject(r); + } +} diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/serialization/SerializableObjectInSessionCheckMissingImportSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/serialization/SerializableObjectInSessionCheckMissingImportSample.java new file mode 100644 index 00000000000..1395122c7c3 --- /dev/null +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/serialization/SerializableObjectInSessionCheckMissingImportSample.java @@ -0,0 +1,14 @@ +package checks.serialization; + +import javax.servlet.http.HttpSession; + +class SerializableObjectInSessionCheckMissingImportSample { + public static record R(String foo, Boolean bar) implements Serializable {} + + private HttpSession session = null; + + public void usage() { + R r = new R("foo", true); + session.setAttribute("foo", r); + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/serialization/NonSerializableWriteCheck.java b/java-checks/src/main/java/org/sonar/java/checks/serialization/NonSerializableWriteCheck.java index 61ed608166f..f505cb1c261 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/serialization/NonSerializableWriteCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/serialization/NonSerializableWriteCheck.java @@ -82,7 +82,7 @@ private boolean isTestedSymbol(ExpressionTree tree) { private void visitMethodInvocation(MethodInvocationTree methodInvocation) { if (WRITE_OBJECT_MATCHER.matches(methodInvocation)) { ExpressionTree argument = methodInvocation.arguments().get(0); - if (!isTestedSymbol(argument) && ExpressionsHelper.isNotSerializable(argument)) { + if (!isTestedSymbol(argument) && ExpressionsHelper.isNotSerializable(argument, false)) { reportIssue(argument, "Make the \"" + argument.symbolType().fullyQualifiedName() + "\" class \"Serializable\" or don't write it."); } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheck.java index e6fcf261fdc..82b2db3a9d7 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheck.java @@ -44,7 +44,7 @@ protected MethodMatchers getMethodInvocationMatchers() { protected void onMethodInvocationFound(MethodInvocationTree mit) { ExpressionTree argument = mit.arguments().get(1); Type type = argument.symbolType(); - if (ExpressionsHelper.isNotSerializable(argument)) { + if (ExpressionsHelper.isNotSerializable(argument, false)) { String andParameters = type.isParameterized() ? " and its parameters" : ""; reportIssue(argument, "Make \"" + type.name() + "\"" + andParameters + " serializable or don't store it in the session."); } diff --git a/java-checks/src/test/java/org/sonar/java/checks/serialization/NonSerializableWriteCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/serialization/NonSerializableWriteCheckTest.java index 18639ad7118..b9437022920 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/serialization/NonSerializableWriteCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/serialization/NonSerializableWriteCheckTest.java @@ -20,6 +20,7 @@ import org.sonar.java.checks.verifier.CheckVerifier; import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; +import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath; class NonSerializableWriteCheckTest { @@ -31,6 +32,14 @@ void test() { .verifyIssues(); } + @Test + void test_missing_import() { + CheckVerifier.newVerifier() + .onFile(nonCompilingTestSourcesPath("checks/serialization/NonSerializableWriteCheckMissingImportSample.java")) + .withCheck(new NonSerializableWriteCheck()) + .verifyNoIssues(); + } + @Test void unresolved() { CheckVerifier.newVerifier() diff --git a/java-checks/src/test/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheckTest.java index 8f7a9cb7660..f725d53b4e8 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheckTest.java @@ -20,6 +20,7 @@ import org.sonar.java.checks.verifier.CheckVerifier; import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; +import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath; class SerializableObjectInSessionCheckTest { @@ -31,6 +32,14 @@ void test() { .verifyIssues(); } + @Test + void test_missing_import() { + CheckVerifier.newVerifier() + .onFile(nonCompilingTestSourcesPath("checks/serialization/SerializableObjectInSessionCheckMissingImportSample.java")) + .withCheck(new SerializableObjectInSessionCheck()) + .verifyNoIssues(); + } + @Test void unresolved() { CheckVerifier.newVerifier() From 3ae6eed065e7da539f9c2cb77e08c49a4da53657 Mon Sep 17 00:00:00 2001 From: Tomasz Tylenda Date: Thu, 17 Jul 2025 11:59:59 +0200 Subject: [PATCH 2/3] Remove defaultOnError - it is always false --- .../checks/helpers/ExpressionsHelper.java | 24 +++++++++---------- .../NonSerializableWriteCheck.java | 2 +- .../SerializableObjectInSessionCheck.java | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java b/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java index 488bcba7f9a..f93647c64bb 100644 --- a/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java +++ b/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java @@ -173,23 +173,23 @@ public List valuePath() { /** * Checks if the expression is non-serializable. * - * @param defaultOnUnknown It will be returned if the result cannot be determined - * due to incomplete semantics. + *

If the result cannot be determined due to incomplete semantics, + * the method returns false. */ - public static boolean isNotSerializable(ExpressionTree expression, boolean defaultOnUnknown) { + public static boolean isNotSerializable(ExpressionTree expression) { Type symbolType = expression.symbolType(); if (symbolType.isUnknown()) { return false; } - return isNonSerializable(symbolType, defaultOnUnknown) - || isAssignedToNonSerializable(expression, defaultOnUnknown); + return isNonSerializable(symbolType) + || isAssignedToNonSerializable(expression); } - private static boolean isNonSerializable(Type type, boolean defaultOnUnknown) { + private static boolean isNonSerializable(Type type) { if (type.isArray()) { - return isNonSerializable(((Type.ArrayType) type).elementType(), defaultOnUnknown); + return isNonSerializable(((Type.ArrayType) type).elementType()); } - if (type.typeArguments().stream().anyMatch(t -> isNonSerializable(t, defaultOnUnknown))) { + if (type.typeArguments().stream().anyMatch(ExpressionsHelper::isNonSerializable)) { return true; } if (type.isPrimitive() || @@ -205,18 +205,18 @@ private static boolean isNonSerializable(Type type, boolean defaultOnUnknown) { return false; } if(hasUnknownTypeInHierarchy(type.symbol())) { - return defaultOnUnknown; + return false; } Type erasedType = type.erasure(); - return erasedType.equals(type) || isNonSerializable(erasedType, defaultOnUnknown); + return erasedType.equals(type) || isNonSerializable(erasedType); } - private static boolean isAssignedToNonSerializable(ExpressionTree expression, boolean defaultOnUnknown) { + private static boolean isAssignedToNonSerializable(ExpressionTree expression) { return ExpressionUtils.extractIdentifierSymbol(expression) .filter(symbol -> initializedAndAssignedExpressionStream(symbol) .map(ExpressionTree::symbolType) .filter(Predicate.not(Type::isUnknown)) - .anyMatch(t -> isNonSerializable(t, defaultOnUnknown))) + .anyMatch(ExpressionsHelper::isNonSerializable)) .isPresent(); } diff --git a/java-checks/src/main/java/org/sonar/java/checks/serialization/NonSerializableWriteCheck.java b/java-checks/src/main/java/org/sonar/java/checks/serialization/NonSerializableWriteCheck.java index f505cb1c261..61ed608166f 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/serialization/NonSerializableWriteCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/serialization/NonSerializableWriteCheck.java @@ -82,7 +82,7 @@ private boolean isTestedSymbol(ExpressionTree tree) { private void visitMethodInvocation(MethodInvocationTree methodInvocation) { if (WRITE_OBJECT_MATCHER.matches(methodInvocation)) { ExpressionTree argument = methodInvocation.arguments().get(0); - if (!isTestedSymbol(argument) && ExpressionsHelper.isNotSerializable(argument, false)) { + if (!isTestedSymbol(argument) && ExpressionsHelper.isNotSerializable(argument)) { reportIssue(argument, "Make the \"" + argument.symbolType().fullyQualifiedName() + "\" class \"Serializable\" or don't write it."); } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheck.java index 82b2db3a9d7..e6fcf261fdc 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/serialization/SerializableObjectInSessionCheck.java @@ -44,7 +44,7 @@ protected MethodMatchers getMethodInvocationMatchers() { protected void onMethodInvocationFound(MethodInvocationTree mit) { ExpressionTree argument = mit.arguments().get(1); Type type = argument.symbolType(); - if (ExpressionsHelper.isNotSerializable(argument, false)) { + if (ExpressionsHelper.isNotSerializable(argument)) { String andParameters = type.isParameterized() ? " and its parameters" : ""; reportIssue(argument, "Make \"" + type.name() + "\"" + andParameters + " serializable or don't store it in the session."); } From 9b96fe7aa348e9af2d1a0da7e0d732a4db0d3ce5 Mon Sep 17 00:00:00 2001 From: Tomasz Tylenda Date: Fri, 18 Jul 2025 14:58:28 +0200 Subject: [PATCH 3/3] Unit test for the helper --- .../checks/helpers/ExpressionsHelperTest.java | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/java-checks-common/src/test/java/org/sonar/java/checks/helpers/ExpressionsHelperTest.java b/java-checks-common/src/test/java/org/sonar/java/checks/helpers/ExpressionsHelperTest.java index 8a7a1f81710..d4ec69832e8 100644 --- a/java-checks-common/src/test/java/org/sonar/java/checks/helpers/ExpressionsHelperTest.java +++ b/java-checks-common/src/test/java/org/sonar/java/checks/helpers/ExpressionsHelperTest.java @@ -20,7 +20,10 @@ import javax.annotation.Nullable; import org.junit.jupiter.api.Test; import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.ExpressionStatementTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; import org.sonar.plugins.java.api.tree.MethodTree; import static org.assertj.core.api.Assertions.assertThat; @@ -149,4 +152,53 @@ private void assertValueResolution(String code, @Nullable T target) { Boolean value = ExpressionsHelper.getConstantValueAsBoolean(a).value(); assertThat(value).isEqualTo(target); } + + @Test + void isNonSerializable_nonSerializable() { + String code = newCode( + "static class C {}", + "private C c;", + "void f() {", + " System.out.println(c);", + "}" + ); + ExpressionTree expr = getCallArgument(code); + assertThat(ExpressionsHelper.isNotSerializable(expr)).isTrue(); + } + + @Test + void isNonSerializable_javaIoSerializable() { + String code = newCode( + "static class C implements java.io.Serializable {}", + "private C c;", + "void f() {", + " System.out.println(c);", + "}" + ); + ExpressionTree expr = getCallArgument(code); + assertThat(ExpressionsHelper.isNotSerializable(expr)).isFalse(); + } + + @Test + void isNonSerializable_missingImportSerializable() { + String code = newCode( + "static class C implements Serializable {}", + "private C c;", + "void f() {", + " System.out.println(c);", + "}" + ); + ExpressionTree expr = getCallArgument(code); + // We want "false" in case we cannot resolve implemented interfaces, + // to avoid FPs in the checks that use this helper. + assertThat(ExpressionsHelper.isNotSerializable(expr)).isFalse(); + } + + /** Returns the {@code c} argument to {@code System.out.println(c)}. */ + private static ExpressionTree getCallArgument(String code) { + var methodTree = (MethodTree) classTree(code).members().get(2); + var exprStmtTree = (ExpressionStatementTree) methodTree.block().body().get(0); + var methodInvocationTree = (MethodInvocationTree) exprStmtTree.expression(); + return methodInvocationTree.arguments().get(0); + } }