diff --git a/java-checks-test-sources/default/src/main/java/checks/SynchronizedLockCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/SynchronizedLockCheckSample.java deleted file mode 100644 index ecb8eb4112f..00000000000 --- a/java-checks-test-sources/default/src/main/java/checks/SynchronizedLockCheckSample.java +++ /dev/null @@ -1,42 +0,0 @@ -package checks; - -import java.util.concurrent.TimeUnit; -import java.util.concurrent.locks.Condition; -import java.util.concurrent.locks.Lock; - -class SynchronizedLockCheckSample { - void foo() { - Lock lock = new MyLockImpl(); - synchronized (lock) { // Noncompliant {{Synchronize on this "Lock" object using "acquire/release".}} -// ^^^^ - } - synchronized (new MyLockImpl()) { // Noncompliant {{Synchronize on this "Lock" object using "acquire/release".}} - } - synchronized (new UselessIncrementCheck()) { // Compliant - } - } -} - -class MyLockImpl implements Lock { - @Override - public void lock() { - } - @Override - public void lockInterruptibly() throws InterruptedException { - } - @Override - public boolean tryLock() { - return false; - } - @Override - public boolean tryLock(long time, TimeUnit unit) throws InterruptedException { - return false; - } - @Override - public void unlock() { - } - @Override - public Condition newCondition() { - return null; - } -} diff --git a/java-checks-test-sources/default/src/main/java/checks/SynchronizedOnConcurrentObjectCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/SynchronizedOnConcurrentObjectCheckSample.java new file mode 100644 index 00000000000..05233a7a1e3 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/SynchronizedOnConcurrentObjectCheckSample.java @@ -0,0 +1,134 @@ +package checks; + +import java.util.concurrent.ArrayBlockingQueue; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.Future; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.Semaphore; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.locks.Condition; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; + +class SynchronizedOnConcurrentObjectCheckSample { + + private final ReentrantLock reentrantLock = new ReentrantLock(); + private final ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock(); + private final Lock lock = new ReentrantLock(); + private final Semaphore semaphore = new Semaphore(1); + private final CountDownLatch latch = new CountDownLatch(1); + private final CyclicBarrier barrier = new CyclicBarrier(2); + private final BlockingQueue blockingQueue = new ArrayBlockingQueue<>(10); + private final ArrayBlockingQueue arrayBlockingQueue = new ArrayBlockingQueue<>(10); + private final LinkedBlockingQueue linkedBlockingQueue = new LinkedBlockingQueue<>(); + private final AtomicBoolean atomicBoolean = new AtomicBoolean(); + private final AtomicInteger atomicInteger = new AtomicInteger(); + private final CustomLock customLock = new CustomLock(); + private final CustomLockImpl customLockImpl = new CustomLockImpl(); + + private final Object objectLock = new Object(); + private final ConcurrentHashMap concurrentMap = new ConcurrentHashMap<>(); + private Future future; + + void noncompliant() { + synchronized (reentrantLock) { // Noncompliant {{Use the "ReentrantLock" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^ + } + + synchronized (lock) { // Noncompliant {{Use the "Lock" API for synchronization instead of a "synchronized" block.}} + // ^^^^ + } + + synchronized (rwLock) { // Noncompliant {{Use the "ReentrantReadWriteLock" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^ + } + + synchronized (semaphore) { // Noncompliant {{Use the "Semaphore" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^ + } + + synchronized (latch) { // Noncompliant {{Use the "CountDownLatch" API for synchronization instead of a "synchronized" block.}} + // ^^^^^ + } + + synchronized (barrier) { // Noncompliant {{Use the "CyclicBarrier" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^ + } + + synchronized (blockingQueue) { // Noncompliant {{Use the "BlockingQueue" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^ + } + + synchronized (arrayBlockingQueue) { // Noncompliant {{Use the "ArrayBlockingQueue" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^^^^^^ + } + + synchronized (linkedBlockingQueue) { // Noncompliant {{Use the "LinkedBlockingQueue" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^^^^^^^ + } + + synchronized (atomicBoolean) { // Noncompliant {{Use the "AtomicBoolean" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^ + } + + synchronized (atomicInteger) { // Noncompliant {{Use the "AtomicInteger" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^ + } + + synchronized (customLock) { // Noncompliant {{Use the "CustomLock" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^ + } + + synchronized (customLockImpl) { // Noncompliant {{Use the "CustomLockImpl" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^^ + } + } + + void compliant() { + synchronized (objectLock) { + } + + synchronized (concurrentMap) { + } + + synchronized (future) { + } + + reentrantLock.lock(); + try { + } finally { + reentrantLock.unlock(); + } + + rwLock.writeLock().lock(); + try { + } finally { + rwLock.writeLock().unlock(); + } + } + + void example() { + var lock2 = new ReentrantLock(); + synchronized (lock2) { // Noncompliant + } + } + + static class CustomLock extends ReentrantLock { + } + + // Custom Lock implementation outside java.util.concurrent.locks — caught via isSubtypeOf(Lock) + static class CustomLockImpl implements Lock { + @Override public void lock() {} + @Override public void lockInterruptibly() throws InterruptedException {} + @Override public boolean tryLock() { return false; } + @Override public boolean tryLock(long time, TimeUnit unit) throws InterruptedException { return false; } + @Override public void unlock() {} + @Override public Condition newCondition() { return null; } + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/SynchronizedLockCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SynchronizedLockCheck.java deleted file mode 100644 index 23e523142b3..00000000000 --- a/java-checks/src/main/java/org/sonar/java/checks/SynchronizedLockCheck.java +++ /dev/null @@ -1,45 +0,0 @@ -/* - * 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.sonar.check.Rule; -import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; -import org.sonar.plugins.java.api.tree.ExpressionTree; -import org.sonar.plugins.java.api.tree.SynchronizedStatementTree; -import org.sonar.plugins.java.api.tree.Tree; -import org.sonar.plugins.java.api.tree.Tree.Kind; - -import java.util.Collections; -import java.util.List; - -@Rule(key = "S2442") -public class SynchronizedLockCheck extends IssuableSubscriptionVisitor { - - @Override - public List nodesToVisit() { - return Collections.singletonList(Kind.SYNCHRONIZED_STATEMENT); - } - - @Override - public void visitNode(Tree tree) { - ExpressionTree expression = ((SynchronizedStatementTree) tree).expression(); - if (expression.symbolType().isSubtypeOf("java.util.concurrent.locks.Lock")) { - reportIssue(expression, "Synchronize on this \"Lock\" object using \"acquire/release\"."); - } - } - -} diff --git a/java-checks/src/main/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheck.java new file mode 100644 index 00000000000..3f9237e7f82 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheck.java @@ -0,0 +1,73 @@ +/* + * 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.Collections; +import java.util.List; +import java.util.Set; +import org.sonar.check.Rule; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.SynchronizedStatementTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.Tree.Kind; + +@Rule(key = "S2442") +public class SynchronizedOnConcurrentObjectCheck extends IssuableSubscriptionVisitor { + + private static final String CONCURRENT_LOCKS_PREFIX = "java.util.concurrent.locks."; + private static final String CONCURRENT_ATOMIC_PREFIX = "java.util.concurrent.atomic."; + private static final Set CONCURRENT_SYNC_TYPES = Set.of( + "java.util.concurrent.Semaphore", + "java.util.concurrent.CountDownLatch", + "java.util.concurrent.CyclicBarrier", + "java.util.concurrent.Exchanger", + "java.util.concurrent.Phaser", + "java.util.concurrent.BlockingQueue", + "java.util.concurrent.BlockingDeque", + "java.util.concurrent.TransferQueue"); + + @Override + public List nodesToVisit() { + return Collections.singletonList(Kind.SYNCHRONIZED_STATEMENT); + } + + @Override + public void visitNode(Tree tree) { + ExpressionTree expression = ((SynchronizedStatementTree) tree).expression(); + Type type = expression.symbolType(); + if (isSynchronizationPrimitive(type)) { + reportIssue(expression, String.format( + "Use the \"%s\" API for synchronization instead of a \"synchronized\" block.", type.name())); + } + } + + private static boolean isSynchronizationPrimitive(Type type) { + return type.isSubtypeOf("java.util.concurrent.locks.Lock") + || isKnownSyncPrimitive(type) + || type.symbol().superTypes().stream().anyMatch(SynchronizedOnConcurrentObjectCheck::isKnownSyncPrimitive); + } + + private static boolean isKnownSyncPrimitive(Type type) { + String fqn = type.fullyQualifiedName(); + return fqn.startsWith(CONCURRENT_LOCKS_PREFIX) + || fqn.startsWith(CONCURRENT_ATOMIC_PREFIX) + || CONCURRENT_SYNC_TYPES.contains(fqn); + } + +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/SynchronizedLockCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheckTest.java similarity index 75% rename from java-checks/src/test/java/org/sonar/java/checks/SynchronizedLockCheckTest.java rename to java-checks/src/test/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheckTest.java index c253513ee0f..72ac10b1a2b 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/SynchronizedLockCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheckTest.java @@ -21,22 +21,23 @@ import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; -class SynchronizedLockCheckTest { +class SynchronizedOnConcurrentObjectCheckTest { @Test void test() { CheckVerifier.newVerifier() - .onFile(mainCodeSourcesPath("checks/SynchronizedLockCheckSample.java")) - .withCheck(new SynchronizedLockCheck()) + .onFile(mainCodeSourcesPath("checks/SynchronizedOnConcurrentObjectCheckSample.java")) + .withCheck(new SynchronizedOnConcurrentObjectCheck()) .verifyIssues(); } @Test void test_without_semantic() { CheckVerifier.newVerifier() - .onFile(mainCodeSourcesPath("checks/SynchronizedLockCheckSample.java")) - .withCheck(new SynchronizedLockCheck()) + .onFile(mainCodeSourcesPath("checks/SynchronizedOnConcurrentObjectCheckSample.java")) + .withCheck(new SynchronizedOnConcurrentObjectCheck()) .withoutSemantic() .verifyIssues(); } + } diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2442.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2442.html index 762a327bc9e..6c99fb21be6 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2442.html +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2442.html @@ -1,29 +1,83 @@ +

This is an issue when using synchronized blocks on objects from the java.util.concurrent package, such as +ReentrantLock, Semaphore, CountDownLatch, CyclicBarrier, BlockingQueue, or atomic +types like AtomicInteger.

Why is this an issue?

-

java.util.concurrent.locks.Lock offers far more powerful and flexible locking operations than are available with -synchronized blocks. So synchronizing on a Lock instance throws away the power of the object, as it overrides its better -locking mechanisms. Instead, such objects should be locked and unlocked using one of their lock and unlock method -variants.

-

Noncompliant code example

+

Concurrent programming libraries provide their own high-level synchronization mechanisms. These are designed to be more flexible and powerful than +the language’s basic object locking mechanism.

+

When you use the basic locking mechanism on such an object, you are acquiring a lock that is built into the object itself at the language runtime +level. However, this built-in lock is completely separate from the object’s own synchronization protocol. The two mechanisms do not interact with each +other at all.

+

For example:

+
    +
  • Using basic locking on a reentrant lock object does NOT call the object’s lock acquisition or release methods
    • +
  • Using basic locking on a semaphore object does NOT call the object’s acquire or release methods
    • +
  • Using basic locking on a countdown latch object does NOT call the object’s await or countdown methods
    • +
  • Using basic locking on an atomic variable does NOT use its compare-and-swap operations
    • +
+

This means that threads using the basic locking mechanism on these objects will not coordinate with threads using the object’s proper API methods. +Different threads might think they have exclusive access when they actually don’t, leading to race conditions and data corruption.

+

This pattern typically indicates a misunderstanding of how concurrent library classes work. The developer likely intended to use the object’s own +synchronization mechanism but mistakenly used the basic locking mechanism instead.

+

In Java, this refers to classes from the java.util.concurrent package (including java.util.concurrent.locks and +java.util.concurrent.atomic) and using the synchronized keyword. Specific examples include using +synchronized(reentrantLock) instead of calling lock()/unlock() on a ReentrantLock, or using +synchronized(semaphore) instead of calling acquire()/release() on a Semaphore, or using +synchronized(atomicInt) instead of calling compareAndSet()/getAndUpdate() on an AtomicInteger.

+

What is the potential impact?

+

When language-level locking mechanisms are used on concurrent data structures that implement their own internal synchronization, the code fails to +provide the expected thread-safety guarantees:

+
    +
  • Race conditions: Multiple threads may access shared resources simultaneously, even though the code appears to prevent this
    • +
  • Data corruption: Concurrent modifications to shared data can lead to inconsistent state
    • +
  • Logic errors: The application may behave incorrectly in multi-threaded scenarios, with bugs that are difficult to reproduce and + diagnose
    • +
  • False sense of security: The presence of explicit locking blocks may give developers and reviewers false confidence that the + code is thread-safe
    • +
+

These issues are particularly dangerous because they often only manifest under specific timing conditions, making them hard to detect during +testing.

+

How to fix it

+

Replace the synchronized block with the correct API methods for the specific java.util.concurrent class you’re using. +Always use try-finally blocks when acquiring locks to ensure they are released even if an exception occurs.

+

Code examples

+

Noncompliant code example

-Lock lock = new MyLockImpl();
-synchronized(lock) {  // Noncompliant
-  // ...
+private final ReentrantLock lock = new ReentrantLock();
+
+public void doWork() {
+    synchronized (lock) {  // Noncompliant
+        criticalSection();
+    }
 }
-

Compliant solution

+

Compliant solution

-Lock lock = new MyLockImpl();
-if (lock.tryLock()) {
-  try {
-    // ...
-  } finally {
-    lock.unlock();
-  }
+private final ReentrantLock lock = new ReentrantLock();
+
+public void doWork() {
+    lock.lock();
+    try {
+        criticalSection();
+    } finally {
+        lock.unlock();
+    }
 }

Resources

+

Standards

  • CERT, LCK03-J. - Do not synchronize on the intrinsic locks of high-level concurrency objects
+

Documentation

+ +

Related rules

+
    +
  • {rule:java:S2445} - Blocks should be synchronized on "private final" fields
    • +
diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2442.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2442.json index 7e021c312d7..ee55f99a9c5 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2442.json +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2442.json @@ -1,5 +1,5 @@ { - "title": "Synchronizing on a \"Lock\" object should be avoided", + "title": "Synchronizing on a \"java.util.concurrent\" object should be avoided", "type": "CODE_SMELL", "code": { "impacts": {