VNX-821: Incorrect Synchronization

Java code synchronizes, but on a monitor that does not give the intended mutual exclusion. Reported: synchronized (new X()) (a fresh lock each call, so nothing is excluded); synchronized on a local variable assigned new ... in the same method; synchronized (Thread.currentThread()) (each thread locks itself); synchronized on a string literal, Boolean.TRUE/FALSE, Integer/Long/String.valueOf or .intern(), or a String/Integer/Long/Short/Byte/Boolean/Character field or variable, final or not (interned and cached values are shared with unrelated code); synchronized (getClass()) (a subclass instance locks a different Class object); synchronized on a java.util.concurrent Lock/ReentrantLock/ReadWriteLock/StampedLock/Semaphore object, field or local (its intrinsic monitor is unrelated to lock()/unlock()); synchronized on a non-final field that a method other than a constructor reassigns (threads end up holding different monitors); and a write to a static non-final field inside a non-static synchronized method, which locks the instance while the data is shared by every instance. Not reported: synchronized (this), on a final Object or collection field, on X.class, on a non-final field assigned only in a constructor, on a local obtained from a shared map or a lock-stripe array, static synchronized methods, a static write inside a synchronized (X.class) block, comments and strings.

Field	Value
Rule ID	`VNX-821`
Severity	Low
SARIF level	`note`
Kind	`sast`
Languages	Java
CWE	CWE-821
CAPEC	CAPEC-66
ATT&CK	T1190
CWSS vector	`CWSS:1.0/TI:L/AP:RU/AL:S/IC:N/FC:LT/RP:RU/RL:S/AV:L/AS:N/IN:A/SC:A/BI:L/DI:H/EX:L/EC:N/P:L`
Tags	`lock-sync` `cwe-821` `concurrency`
Source	`internal/sast/rules/vnx-821.rego`

Overview

VNX-821 maps to CWE-821: Incorrect Synchronization. The code takes a lock, but the lock object makes the synchronization useless or shares it with unrelated code.

What Gets Flagged

synchronized (new Object()) { n--; }          // FLAGGED: a new monitor every call

Object lock = new Object();
synchronized (lock) { ... }                   // FLAGGED: local lock

synchronized ("cache") { size++; }            // FLAGGED: interned literal shared JVM-wide
synchronized (Boolean.TRUE) { ... }           // FLAGGED
synchronized (getClass()) { ... }             // FLAGGED: subclasses lock a different object

private final ReentrantLock lock = new ReentrantLock();
synchronized (lock) { ... }                   // FLAGGED: use lock.lock()/unlock()

private final String LOCK = "LOCK";
synchronized (LOCK) { ... }                   // FLAGGED: interned String

private static int total;
public synchronized void add(int n) {
    total += n;                               // FLAGGED: instance lock, static data
}

Not flagged: synchronized (this), synchronized (lock) on a private final Object lock = new Object(), synchronized (Foo.class), static synchronized methods.

// FLAGGED: every thread locks itself
synchronized (Thread.currentThread()) { entries.add(amount); }

Not flagged: a non-final lock field assigned only in the constructor, and total++ inside synchronized (Registry.class) within an instance-synchronized method.

Remediation

Lock on a dedicated private final Object field (static when the data is static).
Use lock()/unlock() on java.util.concurrent locks, never their monitor.
Lock static state on the class (Foo.class) or a static final lock.

References

Suppressing this rule

Put a nosec comment on the line, or on line 1 to cover the whole file. Use the comment marker of the file’s language (#, //, -- or ;):

# nosec vnx-821
// nosec vnx-821

Or record an ignore rule: vulnetix ignore add --rule vnx-821 --reason "...". See Suppressing findings.