Improper Locking

Description

Improper Locking occurs when software incorrectly uses synchronization primitives to control access to shared resources. This includes failing to acquire locks before accessing shared data, not releasing locks after use, using wrong lock scope, holding locks for too long, lock ordering violations causing deadlocks, and attempting to lock already-held non-reentrant locks. Improper locking leads to race conditions, deadlocks, and data corruption.

Risk

Race conditions allow data corruption or security bypass. Deadlocks cause denial of service. Double locking on non-reentrant locks causes freezes. Missing unlocks lead to permanent resource unavailability. Lock contention degrades performance. Priority inversion causes system instability.

Solution

Always pair lock acquisitions with releases. Use RAII patterns for automatic unlock. Implement consistent lock ordering to prevent deadlocks. Use appropriate lock granularity. Prefer higher-level concurrency constructs. Use try-lock with timeouts where appropriate. Test with thread sanitizers.

Common Consequences

Impact	Details
Integrity	Scope: Data Corruption Unsynchronized access corrupts shared state.
Availability	Scope: Deadlock/DoS Locking errors cause system hangs.
Security	Scope: Race Conditions Timing vulnerabilities from improper synchronization.

Example Code + Solution Code

Vulnerable Code

// VULNERABLE: Improper locking in C
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>

pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
int shared_counter = 0;

// VULNERABLE: Missing lock
void increment_vulnerable() {
    // VULNERABLE: No lock - race condition
    shared_counter++;
}

// VULNERABLE: Lock never released on error path
int process_data_vulnerable(int* data) {
    pthread_mutex_lock(&lock);

    if (data == NULL) {
        // VULNERABLE: Lock not released on error
        return -1;
    }

    *data = shared_counter;
    pthread_mutex_unlock(&lock);
    return 0;
}

// VULNERABLE: Double lock (non-reentrant)
void outer_function_vulnerable() {
    pthread_mutex_lock(&lock);
    inner_function_vulnerable();  // Also tries to lock
    pthread_mutex_unlock(&lock);
}

void inner_function_vulnerable() {
    // VULNERABLE: Deadlock - lock already held
    pthread_mutex_lock(&lock);
    do_work();
    pthread_mutex_unlock(&lock);
}

// VULNERABLE: Lock ordering violation causing deadlock
pthread_mutex_t lock_a = PTHREAD_MUTEX_INITIALIZER;
pthread_mutex_t lock_b = PTHREAD_MUTEX_INITIALIZER;

void* thread1_vulnerable(void* arg) {
    pthread_mutex_lock(&lock_a);
    sleep(1);  // Increases chance of deadlock
    pthread_mutex_lock(&lock_b);  // Waits for lock_b

    do_work();

    pthread_mutex_unlock(&lock_b);
    pthread_mutex_unlock(&lock_a);
    return NULL;
}

void* thread2_vulnerable(void* arg) {
    pthread_mutex_lock(&lock_b);  // Gets lock_b
    sleep(1);
    pthread_mutex_lock(&lock_a);  // DEADLOCK: Waits for lock_a

    do_work();

    pthread_mutex_unlock(&lock_a);
    pthread_mutex_unlock(&lock_b);
    return NULL;
}

// VULNERABLE: Lock held during blocking operation
void blocking_with_lock_vulnerable() {
    pthread_mutex_lock(&lock);
    read(socket_fd, buffer, size);  // Blocks while holding lock!
    pthread_mutex_unlock(&lock);
}

# VULNERABLE: Python improper locking
import threading

counter = 0
lock = threading.Lock()

# VULNERABLE: No locking
def increment_vulnerable():
    global counter
    # VULNERABLE: Race condition
    counter += 1

# VULNERABLE: Lock not released on exception
def process_vulnerable(data):
    lock.acquire()

    if not data:
        # VULNERABLE: Lock not released
        raise ValueError("No data")

    result = process(data)
    lock.release()
    return result

# VULNERABLE: Double acquire (non-reentrant)
def outer_vulnerable():
    lock.acquire()
    inner_vulnerable()  # VULNERABLE: Deadlock
    lock.release()

def inner_vulnerable():
    lock.acquire()  # Deadlock - same lock already held
    do_work()
    lock.release()

# VULNERABLE: Lock ordering
lock_a = threading.Lock()
lock_b = threading.Lock()

def thread1_vulnerable():
    with lock_a:
        time.sleep(0.1)
        with lock_b:  # Potential deadlock
            work()

def thread2_vulnerable():
    with lock_b:  # Different order!
        time.sleep(0.1)
        with lock_a:  # DEADLOCK
            work()

# VULNERABLE: Incorrect lock scope
class VulnerableCounter:
    def __init__(self):
        self.value = 0
        self.lock = threading.Lock()

    def increment_and_get(self):
        with self.lock:
            self.value += 1

        # VULNERABLE: Read outside lock
        return self.value  # Race condition

// VULNERABLE: Java improper locking
public class VulnerableLocking {

    private int counter = 0;
    private final Object lock = new Object();

    // VULNERABLE: No synchronization
    public void incrementVulnerable() {
        counter++;  // Race condition
    }

    // VULNERABLE: Lock not released on exception
    public void processVulnerable(Object data) {
        synchronized(lock) {
            if (data == null) {
                throw new IllegalArgumentException();
                // Lock released by synchronized block, BUT...
            }
            process(data);
        }
    }

    // VULNERABLE: Double lock (explicit locks)
    private final ReentrantLock reentrantLock = new ReentrantLock();

    // Note: ReentrantLock actually handles this, but conceptually:
    // Non-reentrant lock would deadlock here

    // VULNERABLE: Lock ordering violation
    private final Object lockA = new Object();
    private final Object lockB = new Object();

    public void method1() {
        synchronized(lockA) {
            try { Thread.sleep(100); } catch (Exception e) {}
            synchronized(lockB) {  // Waits for lockB
                work();
            }
        }
    }

    public void method2() {
        synchronized(lockB) {  // Different order
            try { Thread.sleep(100); } catch (Exception e) {}
            synchronized(lockA) {  // DEADLOCK
                work();
            }
        }
    }

    // VULNERABLE: Coarse-grained locking
    private List<User> users = new ArrayList<>();
    private List<Order> orders = new ArrayList<>();

    public synchronized void updateUser(User u) {
        // Holds lock even for unrelated data
        users.add(u);
    }

    public synchronized void updateOrder(Order o) {
        // Same lock, but different data
        orders.add(o);  // Unnecessary contention
    }
}

// VULNERABLE: JavaScript improper locking (async context)
class VulnerableLocking {
    constructor() {
        this.data = {};
        this.processing = false;
    }

    // VULNERABLE: No proper locking for async operations
    async updateData(key, value) {
        // VULNERABLE: Check-then-act race condition
        if (!this.processing) {
            this.processing = true;

            // Async gap - another call could enter here
            await someAsyncOperation();

            this.data[key] = value;
            this.processing = false;
        }
    }

    // VULNERABLE: Lock not released on error
    async processWithLock() {
        this.locked = true;

        const result = await fetchData();  // May throw
        // VULNERABLE: If fetchData throws, locked stays true

        this.locked = false;
        return result;
    }

    // VULNERABLE: No waiting for lock
    async accessShared() {
        if (this.locked) {
            return null;  // Just fails instead of waiting
        }

        this.locked = true;
        // ... work ...
        this.locked = false;
    }
}

Fixed Code

// SAFE: Proper locking in C
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>

pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
int shared_counter = 0;

// SAFE: Proper locking
void increment_safe() {
    pthread_mutex_lock(&lock);
    shared_counter++;
    pthread_mutex_unlock(&lock);
}

// SAFE: Lock released on all paths
int process_data_safe(int* data) {
    int result = 0;

    pthread_mutex_lock(&lock);

    if (data == NULL) {
        result = -1;
        goto cleanup;
    }

    *data = shared_counter;

cleanup:
    pthread_mutex_unlock(&lock);
    return result;
}

// SAFE: Using recursive mutex for reentrancy
pthread_mutex_t recursive_lock;

void init_recursive_lock() {
    pthread_mutexattr_t attr;
    pthread_mutexattr_init(&attr);
    pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
    pthread_mutex_init(&recursive_lock, &attr);
    pthread_mutexattr_destroy(&attr);
}

void outer_function_safe() {
    pthread_mutex_lock(&recursive_lock);
    inner_function_safe();  // Safe - recursive mutex
    pthread_mutex_unlock(&recursive_lock);
}

void inner_function_safe() {
    pthread_mutex_lock(&recursive_lock);  // OK with recursive
    do_work();
    pthread_mutex_unlock(&recursive_lock);
}

// SAFE: Consistent lock ordering
pthread_mutex_t lock_a = PTHREAD_MUTEX_INITIALIZER;
pthread_mutex_t lock_b = PTHREAD_MUTEX_INITIALIZER;

// Always acquire in order: lock_a, then lock_b
void* thread1_safe(void* arg) {
    pthread_mutex_lock(&lock_a);  // First
    pthread_mutex_lock(&lock_b);  // Second

    do_work();

    pthread_mutex_unlock(&lock_b);
    pthread_mutex_unlock(&lock_a);
    return NULL;
}

void* thread2_safe(void* arg) {
    pthread_mutex_lock(&lock_a);  // Same order!
    pthread_mutex_lock(&lock_b);

    do_work();

    pthread_mutex_unlock(&lock_b);
    pthread_mutex_unlock(&lock_a);
    return NULL;
}

// SAFE: Minimal lock scope
void process_with_minimal_lock() {
    char buffer[1024];

    // Read without lock (if socket is thread-local)
    read(socket_fd, buffer, sizeof(buffer));

    // Lock only for shared state access
    pthread_mutex_lock(&lock);
    update_shared_state(buffer);
    pthread_mutex_unlock(&lock);
}

# SAFE: Python proper locking
import threading
from contextlib import contextmanager

counter = 0
lock = threading.Lock()

# SAFE: Using with statement
def increment_safe():
    global counter
    with lock:
        counter += 1

# SAFE: Lock released on exception (context manager)
def process_safe(data):
    with lock:
        if not data:
            raise ValueError("No data")  # Lock released automatically
        return process(data)

# SAFE: Using RLock for reentrant locking
rlock = threading.RLock()

def outer_safe():
    with rlock:
        inner_safe()  # Safe with RLock

def inner_safe():
    with rlock:  # RLock allows reentrant acquisition
        do_work()

# SAFE: Consistent lock ordering
lock_a = threading.Lock()
lock_b = threading.Lock()

def acquire_both():
    """Always acquire in alphabetical order"""
    with lock_a:
        with lock_b:
            return work()

def thread1_safe():
    acquire_both()

def thread2_safe():
    acquire_both()  # Same order

# SAFE: Lock ordering with context manager
@contextmanager
def ordered_locks(*locks):
    """Acquire multiple locks in consistent order"""
    sorted_locks = sorted(locks, key=id)
    try:
        for lock in sorted_locks:
            lock.acquire()
        yield
    finally:
        for lock in reversed(sorted_locks):
            lock.release()

def safe_multi_lock():
    with ordered_locks(lock_a, lock_b):
        # Order determined by id, always consistent
        work()

# SAFE: Correct lock scope
class SafeCounter:
    def __init__(self):
        self.value = 0
        self.lock = threading.Lock()

    def increment_and_get(self):
        with self.lock:
            self.value += 1
            return self.value  # Read inside lock

// SAFE: Java proper locking
public class SafeLocking {

    private int counter = 0;
    private final Object lock = new Object();

    // SAFE: Synchronized method
    public synchronized void incrementSafe() {
        counter++;
    }

    // SAFE: Lock with try-finally
    private final ReentrantLock reentrantLock = new ReentrantLock();

    public void processWithLock(Object data) {
        reentrantLock.lock();
        try {
            if (data == null) {
                throw new IllegalArgumentException();
            }
            process(data);
        } finally {
            // SAFE: Always released
            reentrantLock.unlock();
        }
    }

    // SAFE: Consistent lock ordering
    private final Object lockA = new Object();
    private final Object lockB = new Object();

    private void acquireBothLocks() {
        // Always same order based on identity hash
        Object first = System.identityHashCode(lockA) < System.identityHashCode(lockB)
            ? lockA : lockB;
        Object second = first == lockA ? lockB : lockA;

        synchronized(first) {
            synchronized(second) {
                work();
            }
        }
    }

    // SAFE: Fine-grained locking
    private final List<User> users = new ArrayList<>();
    private final List<Order> orders = new ArrayList<>();
    private final Object usersLock = new Object();
    private final Object ordersLock = new Object();

    public void updateUser(User u) {
        synchronized(usersLock) {
            users.add(u);
        }
    }

    public void updateOrder(Order o) {
        synchronized(ordersLock) {
            // Different lock - no contention
            orders.add(o);
        }
    }

    // SAFE: Using concurrent collections
    private final ConcurrentHashMap<String, User> userMap = new ConcurrentHashMap<>();

    public void addUser(String id, User user) {
        // Thread-safe without explicit locking
        userMap.put(id, user);
    }

    // SAFE: Read-write lock for read-heavy workloads
    private final ReadWriteLock rwLock = new ReentrantReadWriteLock();
    private List<String> cache = new ArrayList<>();

    public List<String> readCache() {
        rwLock.readLock().lock();
        try {
            return new ArrayList<>(cache);
        } finally {
            rwLock.readLock().unlock();
        }
    }

    public void updateCache(List<String> newData) {
        rwLock.writeLock().lock();
        try {
            cache = new ArrayList<>(newData);
        } finally {
            rwLock.writeLock().unlock();
        }
    }
}

// SAFE: JavaScript proper async locking
const { Mutex, withTimeout } = require('async-mutex');

class SafeLocking {
    constructor() {
        this.data = {};
        this.mutex = new Mutex();
    }

    // SAFE: Using mutex for async operations
    async updateData(key, value) {
        const release = await this.mutex.acquire();
        try {
            await someAsyncOperation();
            this.data[key] = value;
        } finally {
            // SAFE: Always released
            release();
        }
    }

    // SAFE: Using runExclusive helper
    async processWithLock() {
        return await this.mutex.runExclusive(async () => {
            const result = await fetchData();
            // Lock automatically released even on error
            return result;
        });
    }

    // SAFE: Timeout to prevent deadlock
    async accessWithTimeout() {
        try {
            return await withTimeout(this.mutex, 5000).runExclusive(async () => {
                return await slowOperation();
            });
        } catch (e) {
            if (e.message === 'timeout') {
                console.log('Lock acquisition timed out');
                return null;
            }
            throw e;
        }
    }
}

// SAFE: Semaphore for limited concurrency
const { Semaphore } = require('async-mutex');

class ConnectionPool {
    constructor(maxConnections) {
        this.semaphore = new Semaphore(maxConnections);
        this.connections = [];
    }

    async withConnection(fn) {
        const [value, release] = await this.semaphore.acquire();
        try {
            const conn = await this.getConnection();
            return await fn(conn);
        } finally {
            release();
        }
    }
}

---

Exploited in the Wild

Deadlock Attacks

Triggering lock ordering violations to cause DoS.

Race Condition Exploits

Exploiting missing locks for privilege escalation.

Priority Inversion

Real-time system failures from improper locking.

---

Tools to test/exploit

• Thread sanitizers (TSan, Helgrind).

• Deadlock detectors.

• Lock analysis tools.

---

CVE Examples

• CVE-2016-9576: Kernel lock ordering deadlock.

• CVE-2019-12379: Race condition from missing lock.

---

References

1. MITRE. "CWE-667: Improper Locking." https://cwe.mitre.org/data/definitions/667.html

2. "The Art of Multiprocessor Programming" - lock ordering.