From a19b02915fb973e35f6df6a6a8035130c2f94ea6 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 12 Oct 2026 13:21:12 +0200 Subject: [PATCH 1/8] SONARJAVA-7154: Implemented rule S9428 MessageDigest instances should not be stored as fields Detects MessageDigest instances retained in type fields, which can share mutable digest state across invocations or threads. --- ...MessageDigestInstanceFieldCheckSample.java | 59 +++++++++++++ .../MessageDigestInstanceFieldCheck.java | 59 +++++++++++++ .../MessageDigestInstanceFieldCheckTest.java | 42 ++++++++++ .../org/sonar/l10n/java/rules/java/S9428.html | 83 +++++++++++++++++++ .../org/sonar/l10n/java/rules/java/S9428.json | 33 ++++++++ .../main/resources/profiles/Sonar_way/S9428 | 0 6 files changed, 276 insertions(+) create mode 100644 java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java create mode 100644 java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java create mode 100644 java-checks/src/test/java/org/sonar/java/checks/MessageDigestInstanceFieldCheckTest.java create mode 100644 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.html create mode 100644 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.json create mode 100644 sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9428 diff --git a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java new file mode 100644 index 00000000000..b63d5e980cb --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java @@ -0,0 +1,59 @@ +package checks; + +import java.security.MessageDigest; +import java.util.function.Consumer; + +class MessageDigestInstanceFieldCheckSample { + + private MessageDigest requestDigest; // Noncompliant {{Make this MessageDigest a local variable.}} + private static MessageDigest sharedDigest; // Noncompliant {{Make this MessageDigest a local variable.}} + private final MessageDigest configuredDigest; // Noncompliant {{Make this MessageDigest a local variable.}} + private CustomDigest customDigest; // Noncompliant {{Make this MessageDigest a local variable.}} + private String digest = "SHA-256"; // Compliant + private ThreadLocal threadDigest = new ThreadLocal<>(); // Compliant + private Object deferredDigest; // Compliant + + MessageDigestInstanceFieldCheckSample() throws Exception { + configuredDigest = MessageDigest.getInstance("SHA-256"); + MessageDigest constructorDigest = MessageDigest.getInstance("SHA-256"); // Compliant + } + + byte[] hash(byte[] payload) throws Exception { + MessageDigest digest = MessageDigest.getInstance("SHA-256"); // Compliant + digest.update(payload); + return digest.digest(); + } + + void process(byte[][] payloads) throws Exception { + Consumer consumer = payload -> { + MessageDigest digest = MessageDigest.getInstance("SHA-256"); // Compliant + digest.update(payload); + }; + for (byte[] payload : payloads) { + MessageDigest digest = MessageDigest.getInstance("SHA-256"); // Compliant + consumer.accept(digest.digest(payload)); + } + } + + void recover() { + try { + throw new IllegalArgumentException(); + } catch (IllegalArgumentException exception) { + MessageDigest digest = null; // Compliant + } + } + + class NestedHasher { + private MessageDigest digest; // Noncompliant {{Make this MessageDigest a local variable.}} + } + + abstract static class CustomDigest extends MessageDigest { + } +} + +interface DigestRegistry { + MessageDigest DEFAULT_DIGEST = null; // Noncompliant {{Make this MessageDigest a local variable.}} +} + +record HashHolder(MessageDigest digest) { // Noncompliant {{Make this MessageDigest a local variable.}} +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java new file mode 100644 index 00000000000..2b3f2d9ce4b --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java @@ -0,0 +1,59 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import java.util.List; +import org.sonar.check.Rule; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.JavaFileScannerContext; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.VariableTree; + +@Rule(key = "S9428") +public class MessageDigestInstanceFieldCheck extends IssuableSubscriptionVisitor { + + private static final String MESSAGE_DIGEST = "java.security.MessageDigest"; + private static final String MESSAGE = "Make this MessageDigest a local variable."; + + private boolean hasSemantic; + + @Override + public void setContext(JavaFileScannerContext context) { + super.setContext(context); + hasSemantic = context.getSemanticModel() != null; + } + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.VARIABLE); + } + + @Override + public void visitNode(Tree tree) { + if (!hasSemantic) { + return; + } + VariableTree variable = (VariableTree) tree; + Symbol symbol = variable.symbol(); + Type type = symbol.type(); + if (symbol.owner().isTypeSymbol() && !type.isUnknown() && type.isSubtypeOf(MESSAGE_DIGEST)) { + reportIssue(variable.simpleName(), MESSAGE); + } + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/MessageDigestInstanceFieldCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/MessageDigestInstanceFieldCheckTest.java new file mode 100644 index 00000000000..97cb62239f7 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/MessageDigestInstanceFieldCheckTest.java @@ -0,0 +1,42 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + +class MessageDigestInstanceFieldCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/MessageDigestInstanceFieldCheckSample.java")) + .withCheck(new MessageDigestInstanceFieldCheck()) + .verifyIssues(); + } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/MessageDigestInstanceFieldCheckSample.java")) + .withCheck(new MessageDigestInstanceFieldCheck()) + .withoutSemantic() + .verifyNoIssues(); + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.html new file mode 100644 index 00000000000..7856aaa0221 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.html @@ -0,0 +1,83 @@ +

This rule raises an issue when a cryptographic digest/hash object is declared as a class-level variable (either instance-level or shared across all +instances).

+

In Java, this specifically refers to MessageDigest instances.

+

Why is this an issue?

+

Cryptographic hash computation objects are stateful components that calculate hash values by maintaining internal state as data is incrementally +processed through their methods.

+

The problem is that these hash computation objects are not thread-safe. When stored as shared state (such as class-level fields or instance +variables), the same object becomes accessible to multiple threads or multiple method invocations. If two threads invoke methods on the same hash +computation object simultaneously, or if the object is reused between method calls without proper synchronization, the internal state becomes +corrupted.

+

This corruption leads to incorrect hash calculations. The resulting hash values may be completely wrong or inconsistent, which defeats the purpose +of using cryptographic hashing in the first place.

+

While it’s technically possible to synchronize access to a shared hash computation object, this approach adds complexity and potential performance +bottlenecks. The synchronization overhead often negates any perceived performance benefits of reusing the object.

+

Creating a new hash computation object for each use is relatively inexpensive. Modern runtime environments optimize object allocation efficiently, +and the cost of instantiation is negligible compared to the actual hash computation work.

+

In Java, this specifically refers to the MessageDigest class from the Java Security API, which provides methods like +update() and digest() for hash computation.

+

What is the potential impact?

+

When hash calculations produce incorrect results due to unsynchronized access to shared hash computation objects or functions, the consequences can +be severe:

+
    +
  • Data integrity failures: File integrity checks or data verification mechanisms may incorrectly report that data has been + corrupted when it hasn’t, or worse, fail to detect actual corruption.
    • +
  • Authentication bypasses: Password hashing or authentication token generation may produce wrong values, potentially allowing + unauthorized access or preventing legitimate users from authenticating.
    • +
  • Digital signature failures: Cryptographic signatures may be computed incorrectly, causing signature verification to fail or + producing signatures that don’t actually correspond to the signed data.
    • +
  • Cache or deduplication errors: Systems using content hashes for caching or deduplication may incorrectly identify different + data as identical or fail to recognize identical data.
    • +
+

These issues are particularly insidious because they may occur intermittently based on timing, making them difficult to detect during testing but +capable of causing production failures.

+

How to fix it

+

Create the MessageDigest instance locally within the method that needs it, rather than storing it as a field. This ensures each +operation has its own instance with isolated state.

+

Code examples

+

Noncompliant code example

+
+import java.security.MessageDigest;
+
+public class HashCalculator {
+    private final MessageDigest digest; // Noncompliant
+
+    public HashCalculator() throws Exception {
+        digest = MessageDigest.getInstance("SHA-256");
+    }
+
+    public byte[] computeHash(byte[] data) {
+        digest.reset();
+        digest.update(data);
+        return digest.digest();
+    }
+}
+
+

Compliant solution

+
+import java.security.MessageDigest;
+
+public class HashCalculator {
+    public byte[] computeHash(byte[] data) throws Exception {
+        MessageDigest digest = MessageDigest.getInstance("SHA-256");
+        digest.update(data);
+        return digest.digest();
+    }
+}
+
+

Resources

+

Documentation

+ +

Standards

+ diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.json new file mode 100644 index 00000000000..97f0e5f43b8 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.json @@ -0,0 +1,33 @@ +{ + "title": "MessageDigest instances should not be stored as fields", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5 min" + }, + "tags": [ + "concurrency", + "cryptography", + "pitfall" + ], + "defaultSeverity": "Critical", + "ruleSpecification": "RSPEC-9428", + "sqKey": "S9428", + "scope": "All", + "quickfix": "unknown", + "code": { + "impacts": { + "SECURITY": "MEDIUM", + "RELIABILITY": "HIGH", + "MAINTAINABILITY": "MEDIUM" + }, + "attribute": "COMPLETE" + }, + "securityStandards": { + "CWE": [ + "362", + "567" + ] + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9428 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9428 new file mode 100644 index 00000000000..e69de29bb2d From 558a4d956a1967a5c291adf0f77a313955293b20 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 12 Oct 2026 13:33:17 +0200 Subject: [PATCH 2/8] SONARJAVA-7154 Fix S9428 test sample compilation Add the required constructor to CustomDigest, avoid checked exceptions in a java.util.function lambda, and use numeric CWE ids in the rule metadata. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../checks/MessageDigestInstanceFieldCheckSample.java | 10 ++++++++-- .../org/sonar/l10n/java/rules/java/S9428.json | 6 +++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java index b63d5e980cb..6ed9cd76691 100644 --- a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java @@ -1,7 +1,6 @@ package checks; import java.security.MessageDigest; -import java.util.function.Consumer; class MessageDigestInstanceFieldCheckSample { @@ -25,7 +24,7 @@ byte[] hash(byte[] payload) throws Exception { } void process(byte[][] payloads) throws Exception { - Consumer consumer = payload -> { + PayloadConsumer consumer = payload -> { MessageDigest digest = MessageDigest.getInstance("SHA-256"); // Compliant digest.update(payload); }; @@ -47,7 +46,14 @@ class NestedHasher { private MessageDigest digest; // Noncompliant {{Make this MessageDigest a local variable.}} } + interface PayloadConsumer { + void accept(byte[] payload) throws Exception; + } + abstract static class CustomDigest extends MessageDigest { + protected CustomDigest(String algorithm) { + super(algorithm); + } } } diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.json index 97f0e5f43b8..9c8021a2753 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.json +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9428.json @@ -4,7 +4,7 @@ "status": "ready", "remediation": { "func": "Constant\/Issue", - "constantCost": "5 min" + "constantCost": "5min" }, "tags": [ "concurrency", @@ -26,8 +26,8 @@ }, "securityStandards": { "CWE": [ - "362", - "567" + 362, + 567 ] } } From e73dd8c8ec19082af464c7a8cd0f0bac3ab0431a Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 12 Oct 2026 13:54:57 +0200 Subject: [PATCH 3/8] SONARJAVA-7154 Update ruling for S9428 and ignore safely used fields Do not report private MessageDigest fields whose usages are only assignments, state-independent calls such as clone(), or located in synchronized code. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../expected/eclipse-jetty/java-S9428.json | 5 ++ .../resources/expected/guava/java-S9428.json | 5 ++ ...MessageDigestInstanceFieldCheckSample.java | 69 +++++++++++++++++++ .../MessageDigestInstanceFieldCheck.java | 64 ++++++++++++++++- 4 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 its/ruling/src/test/resources/expected/eclipse-jetty/java-S9428.json create mode 100644 its/ruling/src/test/resources/expected/guava/java-S9428.json diff --git a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9428.json b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9428.json new file mode 100644 index 00000000000..65c9f16c845 --- /dev/null +++ b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9428.json @@ -0,0 +1,5 @@ +{ +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java": [ +190 +] +} diff --git a/its/ruling/src/test/resources/expected/guava/java-S9428.json b/its/ruling/src/test/resources/expected/guava/java-S9428.json new file mode 100644 index 00000000000..c1fa866d259 --- /dev/null +++ b/its/ruling/src/test/resources/expected/guava/java-S9428.json @@ -0,0 +1,5 @@ +{ +"com.google.guava:guava:src/com/google/common/hash/MessageDigestHashFunction.java": [ +121 +] +} diff --git a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java index 6ed9cd76691..97822b59539 100644 --- a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java @@ -17,6 +17,13 @@ class MessageDigestInstanceFieldCheckSample { MessageDigest constructorDigest = MessageDigest.getInstance("SHA-256"); // Compliant } + void update(byte[] payload) { + requestDigest.update(payload); + sharedDigest.reset(); + this.configuredDigest.digest(payload); + customDigest.update(payload); + } + byte[] hash(byte[] payload) throws Exception { MessageDigest digest = MessageDigest.getInstance("SHA-256"); // Compliant digest.update(payload); @@ -44,6 +51,68 @@ void recover() { class NestedHasher { private MessageDigest digest; // Noncompliant {{Make this MessageDigest a local variable.}} +// ^^^^^^ + + byte[] hash() { + return digest.digest(); + } + } + + static class PrototypeHolder { + private final MessageDigest prototype; // Compliant, only used as a prototype for cloning + private MessageDigest unused; // Compliant + + PrototypeHolder() throws Exception { + this.prototype = MessageDigest.getInstance("SHA-256"); + } + + MessageDigest newDigest() throws Exception { + if (prototype.getDigestLength() > 0) { + return (MessageDigest) this.prototype.clone(); + } + return MessageDigest.getInstance(prototype.getAlgorithm()); + } + } + + static class NonPrivatePrototypeHolder { + final MessageDigest prototype = null; // Noncompliant + + Object newDigest() throws Exception { + return prototype.clone(); + } + } + + static class SynchronizedHolder { + private static MessageDigest lockedDigest; // Compliant, every usage is synchronized + private MessageDigest methodDigest; // Compliant, every usage is synchronized + private MessageDigest lambdaDigest; // Noncompliant + private MessageDigest partiallyLockedDigest; // Noncompliant + private static final Object LOCK = new Object(); + + byte[] hash(byte[] payload) throws Exception { + synchronized (LOCK) { + if (lockedDigest == null) { + lockedDigest = MessageDigest.getInstance("MD5"); + } + lockedDigest.reset(); + lockedDigest.update(payload); + partiallyLockedDigest.update(payload); + return lockedDigest.digest(); + } + } + + synchronized byte[] hashInMethod(byte[] payload) { + methodDigest.update(payload); + return methodDigest.digest(); + } + + synchronized Runnable deferred(byte[] payload) { + return () -> lambdaDigest.update(payload); + } + + byte[] unlocked() { + return partiallyLockedDigest.digest(); + } } interface PayloadConsumer { diff --git a/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java index 2b3f2d9ce4b..f1740896e7d 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java @@ -18,10 +18,18 @@ import java.util.List; import org.sonar.check.Rule; +import org.sonar.java.model.ModifiersUtils; 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.AssignmentExpressionTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.Modifier; import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.VariableTree; @@ -31,6 +39,12 @@ public class MessageDigestInstanceFieldCheck extends IssuableSubscriptionVisitor private static final String MESSAGE_DIGEST = "java.security.MessageDigest"; private static final String MESSAGE = "Make this MessageDigest a local variable."; + private static final MethodMatchers STATELESS_METHODS = MethodMatchers.create() + .ofSubTypes(MESSAGE_DIGEST) + .names("clone", "getAlgorithm", "getDigestLength", "getProvider", "toString") + .addWithoutParametersMatcher() + .build(); + private boolean hasSemantic; @Override @@ -52,8 +66,56 @@ public void visitNode(Tree tree) { VariableTree variable = (VariableTree) tree; Symbol symbol = variable.symbol(); Type type = symbol.type(); - if (symbol.owner().isTypeSymbol() && !type.isUnknown() && type.isSubtypeOf(MESSAGE_DIGEST)) { + if (symbol.owner().isTypeSymbol() && !type.isUnknown() && type.isSubtypeOf(MESSAGE_DIGEST) && !isOnlyUsedSafely(symbol)) { reportIssue(variable.simpleName(), MESSAGE); } } + + /** + * A private field is considered safe when none of its usages can alter the digest state concurrently: each usage is either + * an assignment of the field, a call to a method which does not depend on the digest state (e.g. {@code clone()}), + * or is located in a {@code synchronized} block or method. + */ + private static boolean isOnlyUsedSafely(Symbol symbol) { + return symbol.isPrivate() && symbol.usages().stream().allMatch(MessageDigestInstanceFieldCheck::isSafeUsage); + } + + private static boolean isSafeUsage(IdentifierTree usage) { + Tree expression = usage; + Tree parent = usage.parent(); + if (parent.is(Tree.Kind.MEMBER_SELECT) && ((MemberSelectExpressionTree) parent).identifier() == usage) { + expression = parent; + parent = parent.parent(); + } + return isAssignedTo(expression, parent) || isReceiverOfStatelessCall(expression, parent) || isInSynchronizedContext(usage); + } + + private static boolean isAssignedTo(Tree expression, Tree parent) { + return parent.is(Tree.Kind.ASSIGNMENT) && ((AssignmentExpressionTree) parent).variable() == expression; + } + + private static boolean isReceiverOfStatelessCall(Tree expression, Tree parent) { + if (!parent.is(Tree.Kind.MEMBER_SELECT) || ((MemberSelectExpressionTree) parent).expression() != expression) { + return false; + } + Tree invocation = parent.parent(); + return invocation.is(Tree.Kind.METHOD_INVOCATION) && STATELESS_METHODS.matches((MethodInvocationTree) invocation); + } + + private static boolean isInSynchronizedContext(Tree tree) { + Tree current = tree.parent(); + while (current != null) { + if (current.is(Tree.Kind.SYNCHRONIZED_STATEMENT)) { + return true; + } + if (current.is(Tree.Kind.METHOD, Tree.Kind.CONSTRUCTOR)) { + return ModifiersUtils.hasModifier(((MethodTree) current).modifiers(), Modifier.SYNCHRONIZED); + } + if (current.is(Tree.Kind.LAMBDA_EXPRESSION, Tree.Kind.CLASS, Tree.Kind.ENUM, Tree.Kind.RECORD, Tree.Kind.INTERFACE)) { + return false; + } + current = current.parent(); + } + return false; + } } From 698820c33aae8e1a0c96c8058ebe4780673fb5e3 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 12 Oct 2026 14:06:02 +0200 Subject: [PATCH 4/8] SONARJAVA-7154 S9428: require a common monitor for synchronized exemption and restrict stateless-call exemption A private field is now ignored only if all its non-assignment usages are state-independent calls (prototype pattern), or are all guarded by the same known monitor, which must be shared by all instances for static fields. Co-Authored-By: Claude Opus 5.5 (1M context) --- ...MessageDigestInstanceFieldCheckSample.java | 120 ++++++++++++++++++ .../MessageDigestInstanceFieldCheck.java | 102 ++++++++++++--- 2 files changed, 202 insertions(+), 20 deletions(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java index 97822b59539..8d8860cf480 100644 --- a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java @@ -115,6 +115,126 @@ byte[] unlocked() { } } + static class MonitorHolder { + private static final Object LOCK = new Object(); + private static Object mutableLock = new Object(); + private final Object lockA = new Object(); + private final Object lockB = new Object(); + private static MessageDigest staticInMethodDigest; // Noncompliant + private static MessageDigest staticSynchronizedDigest; // Compliant, every usage holds the class monitor + private static MessageDigest classLiteralDigest; // Compliant, every usage holds the class monitor + private MessageDigest mixedLockDigest; // Noncompliant + private MessageDigest twoLocksDigest; // Noncompliant + private MessageDigest instanceLockDigest; // Compliant, every usage holds lockA + private static MessageDigest staticInstanceLockDigest; // Noncompliant + private MessageDigest thisLockDigest; // Compliant, every usage holds this + private MessageDigest innerClassDigest; // Noncompliant + private MessageDigest mutableLockDigest; // Noncompliant + private MessageDigest localLockDigest; // Noncompliant + private MessageDigest otherInstanceLockDigest; // Noncompliant + private MessageDigest clonedOutsideLockDigest; // Noncompliant + private MessageDigest clonedInsideLockDigest; // Compliant, every usage holds LOCK + private MessageDigest unsynchronizedDigest; // Noncompliant + + synchronized void staticInMethod(byte[] payload) { + staticInMethodDigest.update(payload); + } + + static synchronized void staticSynchronized(byte[] payload) { + staticSynchronizedDigest.update(payload); + } + + static void classLiteral(byte[] payload) { + synchronized (MonitorHolder.class) { + classLiteralDigest.update(payload); + } + } + + synchronized void classLiteralInMethod(byte[] payload) { + synchronized (MonitorHolder.class) { + classLiteralDigest.update(payload); + } + } + + void mixedLock(byte[] payload) { + synchronized (LOCK) { + mixedLockDigest.update(payload); + } + } + + synchronized byte[] mixedLockInMethod() { + return mixedLockDigest.digest(); + } + + void twoLocks(byte[] payload) { + synchronized (lockA) { + twoLocksDigest.update(payload); + instanceLockDigest.update(payload); + staticInstanceLockDigest.update(payload); + } + synchronized (this.lockB) { + twoLocksDigest.update(payload); + } + synchronized (this.lockA) { + instanceLockDigest.update(payload); + } + } + + void thisLock(byte[] payload) { + synchronized (this) { + thisLockDigest.update(payload); + } + } + + synchronized byte[] thisLockInMethod() { + return thisLockDigest.digest(); + } + + void mutableLock(byte[] payload) { + synchronized (mutableLock) { + mutableLockDigest.update(payload); + } + } + + void localLock(byte[] payload) { + Object lock = new Object(); + synchronized (lock) { + localLockDigest.update(payload); + } + } + + void otherInstanceLock(MonitorHolder other, byte[] payload) { + synchronized (other.lockA) { + otherInstanceLockDigest.update(payload); + } + } + + MessageDigest clonedOutsideLock(byte[] payload) throws Exception { + synchronized (LOCK) { + clonedOutsideLockDigest.update(payload); + } + return (MessageDigest) clonedOutsideLockDigest.clone(); + } + + MessageDigest clonedInsideLock(byte[] payload) throws Exception { + synchronized ((LOCK)) { + clonedInsideLockDigest = MessageDigest.getInstance("SHA-256"); + clonedInsideLockDigest.update(payload); + return (MessageDigest) clonedInsideLockDigest.clone(); + } + } + + MonitorHolder() { + unsynchronizedDigest.reset(); + } + + class Inner { + synchronized void update(byte[] payload) { + innerClassDigest.update(payload); + } + } + } + interface PayloadConsumer { void accept(byte[] payload) throws Exception; } diff --git a/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java index f1740896e7d..0b5b82a9df7 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java @@ -17,7 +17,12 @@ package org.sonar.java.checks; import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; +import javax.annotation.Nullable; import org.sonar.check.Rule; +import org.sonar.java.model.ExpressionUtils; import org.sonar.java.model.ModifiersUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; @@ -25,11 +30,13 @@ import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; import org.sonar.plugins.java.api.tree.IdentifierTree; import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; import org.sonar.plugins.java.api.tree.MethodInvocationTree; import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.Modifier; +import org.sonar.plugins.java.api.tree.SynchronizedStatementTree; import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.VariableTree; @@ -72,29 +79,44 @@ public void visitNode(Tree tree) { } /** - * A private field is considered safe when none of its usages can alter the digest state concurrently: each usage is either - * an assignment of the field, a call to a method which does not depend on the digest state (e.g. {@code clone()}), - * or is located in a {@code synchronized} block or method. + * A private field is considered safe when none of its usages can alter the digest state concurrently. Assignments of the field + * are ignored, and the remaining usages must either all be calls which do not alter the digest state (e.g. {@code clone()}, + * when the field is only used as a prototype), or all be guarded by the same known monitor. For a static field, this monitor + * must be shared by all instances: a static final lock field, a class literal or a static synchronized method. */ private static boolean isOnlyUsedSafely(Symbol symbol) { - return symbol.isPrivate() && symbol.usages().stream().allMatch(MessageDigestInstanceFieldCheck::isSafeUsage); + if (!symbol.isPrivate()) { + return false; + } + List readUsages = symbol.usages().stream() + .filter(usage -> !isAssignedTo(usage)) + .toList(); + if (readUsages.stream().allMatch(MessageDigestInstanceFieldCheck::isReceiverOfStatelessCall)) { + return true; + } + Set> monitors = readUsages.stream() + .map(MessageDigestInstanceFieldCheck::monitorOf) + .collect(Collectors.toSet()); + return monitors.size() == 1 && monitors.iterator().next().filter(monitor -> monitor.canGuard(symbol)).isPresent(); } - private static boolean isSafeUsage(IdentifierTree usage) { - Tree expression = usage; + private static Tree fieldExpression(IdentifierTree usage) { Tree parent = usage.parent(); if (parent.is(Tree.Kind.MEMBER_SELECT) && ((MemberSelectExpressionTree) parent).identifier() == usage) { - expression = parent; - parent = parent.parent(); + return parent; } - return isAssignedTo(expression, parent) || isReceiverOfStatelessCall(expression, parent) || isInSynchronizedContext(usage); + return usage; } - private static boolean isAssignedTo(Tree expression, Tree parent) { + private static boolean isAssignedTo(IdentifierTree usage) { + Tree expression = fieldExpression(usage); + Tree parent = expression.parent(); return parent.is(Tree.Kind.ASSIGNMENT) && ((AssignmentExpressionTree) parent).variable() == expression; } - private static boolean isReceiverOfStatelessCall(Tree expression, Tree parent) { + private static boolean isReceiverOfStatelessCall(IdentifierTree usage) { + Tree expression = fieldExpression(usage); + Tree parent = expression.parent(); if (!parent.is(Tree.Kind.MEMBER_SELECT) || ((MemberSelectExpressionTree) parent).expression() != expression) { return false; } @@ -102,20 +124,60 @@ private static boolean isReceiverOfStatelessCall(Tree expression, Tree parent) { return invocation.is(Tree.Kind.METHOD_INVOCATION) && STATELESS_METHODS.matches((MethodInvocationTree) invocation); } - private static boolean isInSynchronizedContext(Tree tree) { + private static Optional monitorOf(Tree tree) { Tree current = tree.parent(); - while (current != null) { + while (!current.is(Tree.Kind.CONSTRUCTOR, Tree.Kind.LAMBDA_EXPRESSION, Tree.Kind.CLASS, Tree.Kind.ENUM, Tree.Kind.RECORD, Tree.Kind.INTERFACE)) { if (current.is(Tree.Kind.SYNCHRONIZED_STATEMENT)) { - return true; + return lockMonitor(((SynchronizedStatementTree) current).expression()); } - if (current.is(Tree.Kind.METHOD, Tree.Kind.CONSTRUCTOR)) { - return ModifiersUtils.hasModifier(((MethodTree) current).modifiers(), Modifier.SYNCHRONIZED); - } - if (current.is(Tree.Kind.LAMBDA_EXPRESSION, Tree.Kind.CLASS, Tree.Kind.ENUM, Tree.Kind.RECORD, Tree.Kind.INTERFACE)) { - return false; + if (current.is(Tree.Kind.METHOD)) { + return methodMonitor((MethodTree) current); } current = current.parent(); } - return false; + return Optional.empty(); + } + + private static Optional methodMonitor(MethodTree method) { + if (!ModifiersUtils.hasModifier(method.modifiers(), Modifier.SYNCHRONIZED)) { + return Optional.empty(); + } + Symbol owner = method.symbol().owner(); + return Optional.of(new Monitor(owner, method.symbol().isStatic() ? null : owner)); + } + + private static Optional lockMonitor(ExpressionTree lock) { + ExpressionTree expression = ExpressionUtils.skipParentheses(lock); + if (expression.is(Tree.Kind.IDENTIFIER) && "this".equals(((IdentifierTree) expression).name())) { + Symbol owner = expression.symbolType().symbol(); + return Optional.of(new Monitor(owner, owner)); + } + if (expression.is(Tree.Kind.MEMBER_SELECT)) { + MemberSelectExpressionTree memberSelect = (MemberSelectExpressionTree) expression; + if ("class".equals(memberSelect.identifier().name())) { + return Optional.of(new Monitor(memberSelect.expression().symbolType().symbol(), null)); + } + if (!ExpressionUtils.isSelectOnThisOrSuper(memberSelect)) { + return Optional.empty(); + } + expression = memberSelect.identifier(); + } + if (expression.is(Tree.Kind.IDENTIFIER)) { + Symbol lockSymbol = ((IdentifierTree) expression).symbol(); + if (lockSymbol.isVariableSymbol() && lockSymbol.owner().isTypeSymbol() && lockSymbol.isFinal()) { + return Optional.of(new Monitor(lockSymbol, lockSymbol.isStatic() ? null : lockSymbol.owner())); + } + } + return Optional.empty(); + } + + /** + * Object whose lock guards a usage. {@code instanceOwner} is the class whose instances each hold their own monitor, or + * {@code null} when the monitor is shared by all instances. + */ + private record Monitor(Symbol symbol, @Nullable Symbol instanceOwner) { + boolean canGuard(Symbol field) { + return instanceOwner == null || (!field.isStatic() && instanceOwner.equals(field.owner())); + } } } From 93cc88876f86bfb999f8694f50027a9dbddf335b Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 12 Oct 2026 14:35:58 +0200 Subject: [PATCH 5/8] SONARJAVA-7154 S9428: only accept instance-monitor guarding for accesses through this Co-Authored-By: Claude Opus 5.5 (1M context) --- ...MessageDigestInstanceFieldCheckSample.java | 34 +++++++++++++++++++ .../MessageDigestInstanceFieldCheck.java | 10 ++++-- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java index 8d8860cf480..e81913d2376 100644 --- a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java @@ -235,6 +235,40 @@ synchronized void update(byte[] payload) { } } + static class QualifiedAccessHolder { + private final Object lockA = new Object(); + private MessageDigest otherMethodDigest; // Noncompliant {{Make this MessageDigest a local variable.}} +// ^^^^^^^^^^^^^^^^^ + private MessageDigest otherLockDigest; // Noncompliant {{Make this MessageDigest a local variable.}} +// ^^^^^^^^^^^^^^^ + private MessageDigest thisDigest; // Compliant, every usage is on this and holds the instance monitor + private static MessageDigest staticDigest; // Compliant, every usage holds the class monitor + + synchronized void merge(QualifiedAccessHolder other, byte[] payload) { + otherMethodDigest.update(payload); + other.otherMethodDigest.update(payload); + } + + void mergeWithLock(QualifiedAccessHolder other, byte[] payload) { + synchronized (lockA) { + otherLockDigest.update(payload); + other.otherLockDigest.update(payload); + } + } + + synchronized void updateThis(byte[] payload) { + this.thisDigest.update(payload); + thisDigest.reset(); + } + + static void updateStatic(byte[] payload) { + synchronized (QualifiedAccessHolder.class) { + QualifiedAccessHolder.staticDigest.update(payload); + staticDigest.reset(); + } + } + } + interface PayloadConsumer { void accept(byte[] payload) throws Exception; } diff --git a/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java index 0b5b82a9df7..09a747edc8e 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java @@ -82,7 +82,8 @@ public void visitNode(Tree tree) { * A private field is considered safe when none of its usages can alter the digest state concurrently. Assignments of the field * are ignored, and the remaining usages must either all be calls which do not alter the digest state (e.g. {@code clone()}, * when the field is only used as a prototype), or all be guarded by the same known monitor. For a static field, this monitor - * must be shared by all instances: a static final lock field, a class literal or a static synchronized method. + * must be shared by all instances: a static final lock field, a class literal or a static synchronized method. When the monitor + * belongs to an instance, the field must be accessed through {@code this}. */ private static boolean isOnlyUsedSafely(Symbol symbol) { if (!symbol.isPrivate()) { @@ -95,7 +96,7 @@ private static boolean isOnlyUsedSafely(Symbol symbol) { return true; } Set> monitors = readUsages.stream() - .map(MessageDigestInstanceFieldCheck::monitorOf) + .map(usage -> monitorOf(usage).filter(monitor -> monitor.instanceOwner() == null || isAccessedOnThis(usage))) .collect(Collectors.toSet()); return monitors.size() == 1 && monitors.iterator().next().filter(monitor -> monitor.canGuard(symbol)).isPresent(); } @@ -108,6 +109,11 @@ private static Tree fieldExpression(IdentifierTree usage) { return usage; } + private static boolean isAccessedOnThis(IdentifierTree usage) { + Tree expression = fieldExpression(usage); + return expression == usage || ExpressionUtils.isSelectOnThisOrSuper((MemberSelectExpressionTree) expression); + } + private static boolean isAssignedTo(IdentifierTree usage) { Tree expression = fieldExpression(usage); Tree parent = expression.parent(); From 6cef1f06e7095bbb64cb518a2d38227bce962f5c Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Tue, 13 Oct 2026 16:23:00 +0200 Subject: [PATCH 6/8] SONARJAVA-7154 S9428: simplify rule and report on the whole field declaration Drop the synchronized-usage exemption, which filtered no ruling issue and contradicts the rule description discouraging synchronization of shared digests. Keep only the prototype exemption, remove the semantic guard and report on the whole variable declaration. Co-Authored-By: Claude Opus 5.5 (1M context) --- ...MessageDigestInstanceFieldCheckSample.java | 200 +++--------------- .../MessageDigestInstanceFieldCheck.java | 121 +---------- .../MessageDigestInstanceFieldCheckTest.java | 2 +- 3 files changed, 42 insertions(+), 281 deletions(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java index e81913d2376..191481f0de0 100644 --- a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java @@ -5,9 +5,13 @@ class MessageDigestInstanceFieldCheckSample { private MessageDigest requestDigest; // Noncompliant {{Make this MessageDigest a local variable.}} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ private static MessageDigest sharedDigest; // Noncompliant {{Make this MessageDigest a local variable.}} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ private final MessageDigest configuredDigest; // Noncompliant {{Make this MessageDigest a local variable.}} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ private CustomDigest customDigest; // Noncompliant {{Make this MessageDigest a local variable.}} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ private String digest = "SHA-256"; // Compliant private ThreadLocal threadDigest = new ThreadLocal<>(); // Compliant private Object deferredDigest; // Compliant @@ -51,7 +55,7 @@ void recover() { class NestedHasher { private MessageDigest digest; // Noncompliant {{Make this MessageDigest a local variable.}} -// ^^^^^^ +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ byte[] hash() { return digest.digest(); @@ -82,190 +86,46 @@ Object newDigest() throws Exception { } } - static class SynchronizedHolder { - private static MessageDigest lockedDigest; // Compliant, every usage is synchronized - private MessageDigest methodDigest; // Compliant, every usage is synchronized - private MessageDigest lambdaDigest; // Noncompliant - private MessageDigest partiallyLockedDigest; // Noncompliant - private static final Object LOCK = new Object(); + static class EscapingPrototypeHolder { + private MessageDigest returnedDigest; // Noncompliant + private MessageDigest passedDigest; // Noncompliant + private MessageDigest aliasedDigest; // Noncompliant - byte[] hash(byte[] payload) throws Exception { - synchronized (LOCK) { - if (lockedDigest == null) { - lockedDigest = MessageDigest.getInstance("MD5"); - } - lockedDigest.reset(); - lockedDigest.update(payload); - partiallyLockedDigest.update(payload); - return lockedDigest.digest(); - } + MessageDigest returned() throws Exception { + returnedDigest.clone(); + return returnedDigest; } - synchronized byte[] hashInMethod(byte[] payload) { - methodDigest.update(payload); - return methodDigest.digest(); + void passed() throws Exception { + passedDigest.clone(); + consume(passedDigest); } - synchronized Runnable deferred(byte[] payload) { - return () -> lambdaDigest.update(payload); + void aliased() throws Exception { + aliasedDigest.clone(); + MessageDigest alias = aliasedDigest; // Compliant + alias.update(new byte[0]); } - byte[] unlocked() { - return partiallyLockedDigest.digest(); + void consume(MessageDigest digest) { } } - static class MonitorHolder { + static class SynchronizedHolder { private static final Object LOCK = new Object(); - private static Object mutableLock = new Object(); - private final Object lockA = new Object(); - private final Object lockB = new Object(); - private static MessageDigest staticInMethodDigest; // Noncompliant - private static MessageDigest staticSynchronizedDigest; // Compliant, every usage holds the class monitor - private static MessageDigest classLiteralDigest; // Compliant, every usage holds the class monitor - private MessageDigest mixedLockDigest; // Noncompliant - private MessageDigest twoLocksDigest; // Noncompliant - private MessageDigest instanceLockDigest; // Compliant, every usage holds lockA - private static MessageDigest staticInstanceLockDigest; // Noncompliant - private MessageDigest thisLockDigest; // Compliant, every usage holds this - private MessageDigest innerClassDigest; // Noncompliant - private MessageDigest mutableLockDigest; // Noncompliant - private MessageDigest localLockDigest; // Noncompliant - private MessageDigest otherInstanceLockDigest; // Noncompliant - private MessageDigest clonedOutsideLockDigest; // Noncompliant - private MessageDigest clonedInsideLockDigest; // Compliant, every usage holds LOCK - private MessageDigest unsynchronizedDigest; // Noncompliant - - synchronized void staticInMethod(byte[] payload) { - staticInMethodDigest.update(payload); - } - - static synchronized void staticSynchronized(byte[] payload) { - staticSynchronizedDigest.update(payload); - } - - static void classLiteral(byte[] payload) { - synchronized (MonitorHolder.class) { - classLiteralDigest.update(payload); - } - } - - synchronized void classLiteralInMethod(byte[] payload) { - synchronized (MonitorHolder.class) { - classLiteralDigest.update(payload); - } - } - - void mixedLock(byte[] payload) { - synchronized (LOCK) { - mixedLockDigest.update(payload); - } - } - - synchronized byte[] mixedLockInMethod() { - return mixedLockDigest.digest(); - } - - void twoLocks(byte[] payload) { - synchronized (lockA) { - twoLocksDigest.update(payload); - instanceLockDigest.update(payload); - staticInstanceLockDigest.update(payload); - } - synchronized (this.lockB) { - twoLocksDigest.update(payload); - } - synchronized (this.lockA) { - instanceLockDigest.update(payload); - } - } - - void thisLock(byte[] payload) { - synchronized (this) { - thisLockDigest.update(payload); - } - } + private static MessageDigest lockedDigest; // Noncompliant + private MessageDigest methodDigest; // Noncompliant - synchronized byte[] thisLockInMethod() { - return thisLockDigest.digest(); - } - - void mutableLock(byte[] payload) { - synchronized (mutableLock) { - mutableLockDigest.update(payload); - } - } - - void localLock(byte[] payload) { - Object lock = new Object(); - synchronized (lock) { - localLockDigest.update(payload); - } - } - - void otherInstanceLock(MonitorHolder other, byte[] payload) { - synchronized (other.lockA) { - otherInstanceLockDigest.update(payload); - } - } - - MessageDigest clonedOutsideLock(byte[] payload) throws Exception { + byte[] hash(byte[] payload) throws Exception { synchronized (LOCK) { - clonedOutsideLockDigest.update(payload); - } - return (MessageDigest) clonedOutsideLockDigest.clone(); - } - - MessageDigest clonedInsideLock(byte[] payload) throws Exception { - synchronized ((LOCK)) { - clonedInsideLockDigest = MessageDigest.getInstance("SHA-256"); - clonedInsideLockDigest.update(payload); - return (MessageDigest) clonedInsideLockDigest.clone(); - } - } - - MonitorHolder() { - unsynchronizedDigest.reset(); - } - - class Inner { - synchronized void update(byte[] payload) { - innerClassDigest.update(payload); - } - } - } - - static class QualifiedAccessHolder { - private final Object lockA = new Object(); - private MessageDigest otherMethodDigest; // Noncompliant {{Make this MessageDigest a local variable.}} -// ^^^^^^^^^^^^^^^^^ - private MessageDigest otherLockDigest; // Noncompliant {{Make this MessageDigest a local variable.}} -// ^^^^^^^^^^^^^^^ - private MessageDigest thisDigest; // Compliant, every usage is on this and holds the instance monitor - private static MessageDigest staticDigest; // Compliant, every usage holds the class monitor - - synchronized void merge(QualifiedAccessHolder other, byte[] payload) { - otherMethodDigest.update(payload); - other.otherMethodDigest.update(payload); - } - - void mergeWithLock(QualifiedAccessHolder other, byte[] payload) { - synchronized (lockA) { - otherLockDigest.update(payload); - other.otherLockDigest.update(payload); + lockedDigest.update(payload); + return lockedDigest.digest(); } } - synchronized void updateThis(byte[] payload) { - this.thisDigest.update(payload); - thisDigest.reset(); - } - - static void updateStatic(byte[] payload) { - synchronized (QualifiedAccessHolder.class) { - QualifiedAccessHolder.staticDigest.update(payload); - staticDigest.reset(); - } + synchronized byte[] hashInMethod(byte[] payload) { + methodDigest.update(payload); + return methodDigest.digest(); } } @@ -282,7 +142,9 @@ protected CustomDigest(String algorithm) { interface DigestRegistry { MessageDigest DEFAULT_DIGEST = null; // Noncompliant {{Make this MessageDigest a local variable.}} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ } record HashHolder(MessageDigest digest) { // Noncompliant {{Make this MessageDigest a local variable.}} +// ^^^^^^^^^^^^^^^^^^^^ } diff --git a/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java index 09a747edc8e..33e118b3c39 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MessageDigestInstanceFieldCheck.java @@ -17,26 +17,14 @@ package org.sonar.java.checks; import java.util.List; -import java.util.Optional; -import java.util.Set; -import java.util.stream.Collectors; -import javax.annotation.Nullable; import org.sonar.check.Rule; -import org.sonar.java.model.ExpressionUtils; -import org.sonar.java.model.ModifiersUtils; 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.AssignmentExpressionTree; -import org.sonar.plugins.java.api.tree.ExpressionTree; import org.sonar.plugins.java.api.tree.IdentifierTree; import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; import org.sonar.plugins.java.api.tree.MethodInvocationTree; -import org.sonar.plugins.java.api.tree.MethodTree; -import org.sonar.plugins.java.api.tree.Modifier; -import org.sonar.plugins.java.api.tree.SynchronizedStatementTree; import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.VariableTree; @@ -52,14 +40,6 @@ public class MessageDigestInstanceFieldCheck extends IssuableSubscriptionVisitor .addWithoutParametersMatcher() .build(); - private boolean hasSemantic; - - @Override - public void setContext(JavaFileScannerContext context) { - super.setContext(context); - hasSemantic = context.getSemanticModel() != null; - } - @Override public List nodesToVisit() { return List.of(Tree.Kind.VARIABLE); @@ -67,38 +47,21 @@ public List nodesToVisit() { @Override public void visitNode(Tree tree) { - if (!hasSemantic) { - return; - } VariableTree variable = (VariableTree) tree; Symbol symbol = variable.symbol(); - Type type = symbol.type(); - if (symbol.owner().isTypeSymbol() && !type.isUnknown() && type.isSubtypeOf(MESSAGE_DIGEST) && !isOnlyUsedSafely(symbol)) { - reportIssue(variable.simpleName(), MESSAGE); + if (symbol.type().isSubtypeOf(MESSAGE_DIGEST) && symbol.owner().isTypeSymbol() && !isOnlyUsedAsPrototype(symbol)) { + reportIssue(variable, MESSAGE); } } /** - * A private field is considered safe when none of its usages can alter the digest state concurrently. Assignments of the field - * are ignored, and the remaining usages must either all be calls which do not alter the digest state (e.g. {@code clone()}, - * when the field is only used as a prototype), or all be guarded by the same known monitor. For a static field, this monitor - * must be shared by all instances: a static final lock field, a class literal or a static synchronized method. When the monitor - * belongs to an instance, the field must be accessed through {@code this}. + * A private field is only used as a prototype when, apart from being assigned, it is only the receiver of calls which do not + * depend on or alter the digest state, such as {@code clone()} or {@code getAlgorithm()}. */ - private static boolean isOnlyUsedSafely(Symbol symbol) { - if (!symbol.isPrivate()) { - return false; - } - List readUsages = symbol.usages().stream() - .filter(usage -> !isAssignedTo(usage)) - .toList(); - if (readUsages.stream().allMatch(MessageDigestInstanceFieldCheck::isReceiverOfStatelessCall)) { - return true; - } - Set> monitors = readUsages.stream() - .map(usage -> monitorOf(usage).filter(monitor -> monitor.instanceOwner() == null || isAccessedOnThis(usage))) - .collect(Collectors.toSet()); - return monitors.size() == 1 && monitors.iterator().next().filter(monitor -> monitor.canGuard(symbol)).isPresent(); + private static boolean isOnlyUsedAsPrototype(Symbol symbol) { + return symbol.isPrivate() && symbol.usages().stream() + .map(MessageDigestInstanceFieldCheck::fieldExpression) + .allMatch(expression -> isAssignedTo(expression) || isReceiverOfStatelessCall(expression)); } private static Tree fieldExpression(IdentifierTree usage) { @@ -109,19 +72,12 @@ private static Tree fieldExpression(IdentifierTree usage) { return usage; } - private static boolean isAccessedOnThis(IdentifierTree usage) { - Tree expression = fieldExpression(usage); - return expression == usage || ExpressionUtils.isSelectOnThisOrSuper((MemberSelectExpressionTree) expression); - } - - private static boolean isAssignedTo(IdentifierTree usage) { - Tree expression = fieldExpression(usage); + private static boolean isAssignedTo(Tree expression) { Tree parent = expression.parent(); return parent.is(Tree.Kind.ASSIGNMENT) && ((AssignmentExpressionTree) parent).variable() == expression; } - private static boolean isReceiverOfStatelessCall(IdentifierTree usage) { - Tree expression = fieldExpression(usage); + private static boolean isReceiverOfStatelessCall(Tree expression) { Tree parent = expression.parent(); if (!parent.is(Tree.Kind.MEMBER_SELECT) || ((MemberSelectExpressionTree) parent).expression() != expression) { return false; @@ -129,61 +85,4 @@ private static boolean isReceiverOfStatelessCall(IdentifierTree usage) { Tree invocation = parent.parent(); return invocation.is(Tree.Kind.METHOD_INVOCATION) && STATELESS_METHODS.matches((MethodInvocationTree) invocation); } - - private static Optional monitorOf(Tree tree) { - Tree current = tree.parent(); - while (!current.is(Tree.Kind.CONSTRUCTOR, Tree.Kind.LAMBDA_EXPRESSION, Tree.Kind.CLASS, Tree.Kind.ENUM, Tree.Kind.RECORD, Tree.Kind.INTERFACE)) { - if (current.is(Tree.Kind.SYNCHRONIZED_STATEMENT)) { - return lockMonitor(((SynchronizedStatementTree) current).expression()); - } - if (current.is(Tree.Kind.METHOD)) { - return methodMonitor((MethodTree) current); - } - current = current.parent(); - } - return Optional.empty(); - } - - private static Optional methodMonitor(MethodTree method) { - if (!ModifiersUtils.hasModifier(method.modifiers(), Modifier.SYNCHRONIZED)) { - return Optional.empty(); - } - Symbol owner = method.symbol().owner(); - return Optional.of(new Monitor(owner, method.symbol().isStatic() ? null : owner)); - } - - private static Optional lockMonitor(ExpressionTree lock) { - ExpressionTree expression = ExpressionUtils.skipParentheses(lock); - if (expression.is(Tree.Kind.IDENTIFIER) && "this".equals(((IdentifierTree) expression).name())) { - Symbol owner = expression.symbolType().symbol(); - return Optional.of(new Monitor(owner, owner)); - } - if (expression.is(Tree.Kind.MEMBER_SELECT)) { - MemberSelectExpressionTree memberSelect = (MemberSelectExpressionTree) expression; - if ("class".equals(memberSelect.identifier().name())) { - return Optional.of(new Monitor(memberSelect.expression().symbolType().symbol(), null)); - } - if (!ExpressionUtils.isSelectOnThisOrSuper(memberSelect)) { - return Optional.empty(); - } - expression = memberSelect.identifier(); - } - if (expression.is(Tree.Kind.IDENTIFIER)) { - Symbol lockSymbol = ((IdentifierTree) expression).symbol(); - if (lockSymbol.isVariableSymbol() && lockSymbol.owner().isTypeSymbol() && lockSymbol.isFinal()) { - return Optional.of(new Monitor(lockSymbol, lockSymbol.isStatic() ? null : lockSymbol.owner())); - } - } - return Optional.empty(); - } - - /** - * Object whose lock guards a usage. {@code instanceOwner} is the class whose instances each hold their own monitor, or - * {@code null} when the monitor is shared by all instances. - */ - private record Monitor(Symbol symbol, @Nullable Symbol instanceOwner) { - boolean canGuard(Symbol field) { - return instanceOwner == null || (!field.isStatic() && instanceOwner.equals(field.owner())); - } - } } diff --git a/java-checks/src/test/java/org/sonar/java/checks/MessageDigestInstanceFieldCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/MessageDigestInstanceFieldCheckTest.java index 97cb62239f7..48a73cbeed0 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/MessageDigestInstanceFieldCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/MessageDigestInstanceFieldCheckTest.java @@ -37,6 +37,6 @@ void test_without_semantic() { .onFile(mainCodeSourcesPath("checks/MessageDigestInstanceFieldCheckSample.java")) .withCheck(new MessageDigestInstanceFieldCheck()) .withoutSemantic() - .verifyNoIssues(); + .verifyIssues(); } } From ae92bb2b9135267ad68255d5073dca02d9124d8e Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Tue, 13 Oct 2026 16:44:52 +0200 Subject: [PATCH 7/8] SONARJAVA-7154 S9428: add missing issue location markers in test sample Co-Authored-By: Claude Opus 5.5 (1M context) --- .../java/checks/MessageDigestInstanceFieldCheckSample.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java index 191481f0de0..934d227efb0 100644 --- a/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/MessageDigestInstanceFieldCheckSample.java @@ -80,6 +80,7 @@ MessageDigest newDigest() throws Exception { static class NonPrivatePrototypeHolder { final MessageDigest prototype = null; // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Object newDigest() throws Exception { return prototype.clone(); @@ -88,8 +89,11 @@ Object newDigest() throws Exception { static class EscapingPrototypeHolder { private MessageDigest returnedDigest; // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ private MessageDigest passedDigest; // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ private MessageDigest aliasedDigest; // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ MessageDigest returned() throws Exception { returnedDigest.clone(); @@ -114,7 +118,9 @@ void consume(MessageDigest digest) { static class SynchronizedHolder { private static final Object LOCK = new Object(); private static MessageDigest lockedDigest; // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ private MessageDigest methodDigest; // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ byte[] hash(byte[] payload) throws Exception { synchronized (LOCK) { From f636f60a436c65ab808d906adbb0a8598e3b76d2 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 14 Oct 2026 09:33:18 +0200 Subject: [PATCH 8/8] Update ruling results for PR #6321 (#6342) Co-authored-by: github-actions[bot] --- .../src/test/resources/expected/eclipse-jetty/java-S9428.json | 2 +- its/ruling/src/test/resources/expected/guava/java-S9428.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9428.json b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9428.json index 65c9f16c845..b33019fcbed 100644 --- a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9428.json +++ b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9428.json @@ -1,5 +1,5 @@ { -"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java": [ +"eclipse-jetty:jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java": [ 190 ] } diff --git a/its/ruling/src/test/resources/expected/guava/java-S9428.json b/its/ruling/src/test/resources/expected/guava/java-S9428.json index c1fa866d259..2ace408419b 100644 --- a/its/ruling/src/test/resources/expected/guava/java-S9428.json +++ b/its/ruling/src/test/resources/expected/guava/java-S9428.json @@ -1,5 +1,5 @@ { -"com.google.guava:guava:src/com/google/common/hash/MessageDigestHashFunction.java": [ +"guava:src/com/google/common/hash/MessageDigestHashFunction.java": [ 121 ] }