Incorrect Synchronization

Description

Incorrect Synchronization is a concurrency weakness where software attempts to synchronize access to a shared resource but does so incorrectly, failing to properly protect the resource from concurrent access issues. Unlike missing synchronization (CWE-820) where no protection is attempted, incorrect synchronization means the developer tried to implement thread safety but made mistakes in the implementation. Common errors include using the wrong lock object, failing to hold locks for the full duration of compound operations, using non-atomic check-then-act sequences, or releasing locks too early before the protected operation is complete.

Risk

Incorrect synchronization creates a false sense of security—developers believe the code is thread-safe when it is not. The resulting race conditions may be harder to identify because the synchronization code suggests the issue was considered. Attackers who understand concurrency bugs can exploit these flaws to corrupt data, bypass security checks, or manipulate program state. The intermittent nature of race conditions makes them difficult to reproduce and debug, allowing vulnerabilities to persist undetected in production. Security-critical code with incorrect synchronization may allow privilege escalation, authentication bypass, or unauthorized data access through carefully timed attacks.

Solution

Ensure synchronization covers the entire critical section, not just individual operations. Protect compound operations (check-then-act) with a single lock held throughout. Use the same lock object consistently for all access to a given shared resource. Verify that lock acquisition and release are properly paired, including in exception handlers. Consider using higher-level synchronization abstractions that are harder to misuse. Employ static analysis tools specifically designed to detect synchronization errors. Review code for patterns like double-checked locking and ensure they are implemented correctly for the language and platform. Test with concurrency testing tools and stress tests that exercise race conditions.

Common Consequences

Impact	Details
Integrity	Scope: Integrity Modify Application Data - Incorrect synchronization allows concurrent modifications that corrupt shared data.
Confidentiality	Scope: Confidentiality Read Application Data - Race conditions may expose sensitive data or intermediate states to unauthorized readers.
Other	Scope: Other Alter Execution Logic - Timing-based exploitation can manipulate control flow through synchronization gaps.

Example Code

Vulnerable Code

// Vulnerable: Synchronization on wrong object
public class VulnerableWrongLock {
    private List<String> items = new ArrayList<>();
    private Object lock1 = new Object();
    private Object lock2 = new Object();

    public void addItem(String item) {
        synchronized (lock1) {  // Uses lock1
            items.add(item);
        }
    }

    public String getItem(int index) {
        synchronized (lock2) {  // Vulnerable: Uses different lock!
            return items.get(index);
        }
    }
    // No actual protection - different locks don't exclude each other
}

// Vulnerable: Check-then-act not atomic
public class VulnerableCheckThenAct {
    private Map<String, Object> cache = new HashMap<>();
    private Object lock = new Object();

    public Object getOrCreate(String key) {
        // Vulnerable: Lock released between check and put
        synchronized (lock) {
            if (cache.containsKey(key)) {
                return cache.get(key);
            }
        }
        // Gap here - another thread can insert
        Object newValue = createExpensiveObject(key);

        synchronized (lock) {
            cache.put(key, newValue);  // May overwrite other thread's value
        }
        return newValue;
    }
}

// Vulnerable: Double-checked locking implemented incorrectly
class VulnerableDCL {
private:
    static VulnerableDCL* instance;
    static std::mutex mtx;

public:
    static VulnerableDCL* getInstance() {
        if (instance == nullptr) {  // First check without lock
            std::lock_guard<std::mutex> lock(mtx);
            if (instance == nullptr) {  // Second check with lock
                instance = new VulnerableDCL();
                // Vulnerable: Without memory barriers, another thread
                // may see non-null instance before construction completes
            }
        }
        return instance;
    }
};

# Vulnerable: Lock not held during entire compound operation
import threading

class VulnerableBalance:
    def __init__(self):
        self.balance = 0
        self.lock = threading.Lock()

    def transfer(self, amount, target):
        # Vulnerable: Separate locks for check and modify
        with self.lock:
            if self.balance >= amount:
                current = self.balance

        # Gap - another thread can modify balance here!

        with self.lock:
            self.balance = current - amount

        with target.lock:
            target.balance += amount

// Vulnerable: Unlocking too early
pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
int shared_state = 0;
int dependent_state = 0;

void vulnerable_update(int new_value) {
    pthread_mutex_lock(&mutex);
    shared_state = new_value;
    pthread_mutex_unlock(&mutex);  // Vulnerable: Released too early

    // dependent_state should be updated atomically with shared_state
    dependent_state = compute_dependent(new_value);
    // Another thread may see inconsistent shared_state/dependent_state
}

// Vulnerable: Synchronizing on non-final field
public class VulnerableNonFinalLock {
    private Object lock = new Object();  // Not final!

    public void method1() {
        synchronized (lock) {  // Uses current lock reference
            // Critical section
        }
    }

    public void setLock(Object newLock) {
        lock = newLock;  // Changing the lock object!
        // Now method1 uses different lock than before
    }
}

// Vulnerable: Forgetting to unlock in error path
func vulnerableOperation() error {
    mu.Lock()

    result, err := riskyOperation()
    if err != nil {
        return err  // Vulnerable: Lock not released on error!
    }

    mu.Unlock()
    return nil
}

Fixed Code

// Fixed: Use same lock for all access
public class FixedSameLock {
    private List<String> items = new ArrayList<>();
    private final Object lock = new Object();  // Single, final lock

    public void addItem(String item) {
        synchronized (lock) {
            items.add(item);
        }
    }

    public String getItem(int index) {
        synchronized (lock) {  // Same lock
            return items.get(index);
        }
    }
}

// Fixed: Atomic check-then-act
public class FixedCheckThenAct {
    private Map<String, Object> cache = new HashMap<>();
    private final Object lock = new Object();

    public Object getOrCreate(String key) {
        synchronized (lock) {
            // Fixed: Entire operation under one lock
            if (cache.containsKey(key)) {
                return cache.get(key);
            }
            Object newValue = createExpensiveObject(key);
            cache.put(key, newValue);
            return newValue;
        }
    }
}

// Or use ConcurrentHashMap with computeIfAbsent
import java.util.concurrent.ConcurrentHashMap;

public class BetterCheckThenAct {
    private ConcurrentHashMap<String, Object> cache = new ConcurrentHashMap<>();

    public Object getOrCreate(String key) {
        return cache.computeIfAbsent(key, this::createExpensiveObject);
    }
}

// Fixed: Correct double-checked locking with memory ordering
#include <atomic>
#include <mutex>

class FixedDCL {
private:
    static std::atomic<FixedDCL*> instance;
    static std::mutex mtx;

public:
    static FixedDCL* getInstance() {
        FixedDCL* tmp = instance.load(std::memory_order_acquire);
        if (tmp == nullptr) {
            std::lock_guard<std::mutex> lock(mtx);
            tmp = instance.load(std::memory_order_relaxed);
            if (tmp == nullptr) {
                tmp = new FixedDCL();
                instance.store(tmp, std::memory_order_release);
            }
        }
        return tmp;
    }
};

// Or use C++11 magic statics (simpler and correct)
class BetterSingleton {
public:
    static BetterSingleton& getInstance() {
        static BetterSingleton instance;  // Thread-safe in C++11
        return instance;
    }
};

# Fixed: Hold lock for entire compound operation
import threading

class FixedBalance:
    def __init__(self):
        self.balance = 0
        self.lock = threading.Lock()

    def transfer(self, amount, target):
        # Fixed: Use ordered locking to prevent deadlock
        # Always lock in consistent order (by id)
        first, second = (self, target) if id(self) < id(target) else (target, self)

        with first.lock:
            with second.lock:
                # Fixed: Entire operation is atomic
                if self.balance >= amount:
                    self.balance -= amount
                    target.balance += amount
                    return True
                return False

// Fixed: Lock held for entire dependent operation
pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
int shared_state = 0;
int dependent_state = 0;

void fixed_update(int new_value) {
    pthread_mutex_lock(&mutex);
    // Fixed: Both updates under same lock
    shared_state = new_value;
    dependent_state = compute_dependent(new_value);
    pthread_mutex_unlock(&mutex);
}

// Fixed: Use final lock object
public class FixedFinalLock {
    private final Object lock = new Object();  // Final - cannot change

    public void method1() {
        synchronized (lock) {
            // Critical section - always uses same lock
        }
    }

    // No setter for lock - it's immutable
}

// Fixed: Always unlock, even on error
func fixedOperation() error {
    mu.Lock()
    defer mu.Unlock()  // Fixed: Always unlocks

    result, err := riskyOperation()
    if err != nil {
        return err  // Lock released by defer
    }

    return nil
}

---

Related CWEs

• CWE-662: Improper Synchronization (parent)
• CWE-820: Missing Synchronization (sibling)
• CWE-572: Call to Thread run() instead of start() (child)
• CWE-574: EJB Bad Practices: Use of Synchronization Primitives (child)
• CWE-362: Concurrent Execution Using Shared Resource with Improper Synchronization (related)

---

References

1. MITRE Corporation. "CWE-821: Incorrect Synchronization." https://cwe.mitre.org/data/definitions/821.html
2. CERT Java Secure Coding. "LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code."
3. Goetz, Brian. "Java Concurrency in Practice." Addison-Wesley, 2006.