Wrong Lock Usage

ID

csharp.wrong_lock_usage

Severity

critical

Remediation Complexity

trivial

Remediation Risk

low

Remediation Effort

low

Resource

Reliability

Language

CSharp

Tags

concurrency, reliability

Description

Reports calls to Monitor.Enter(x) or Monitor.TryEnter(x) whose monitor is not reliably given back. Two families are covered: the acquiring method never releases the monitor at all, and the method does release it but not from a finally clause, so an exception or an early return can skip the release.

Rationale

Monitor.Enter acquires a synchronization lock that must be released via Monitor.Exit. The lock statement does this automatically by emitting the equivalent of try { Monitor.Enter(x); …​ } finally { Monitor.Exit(x); }. Code that calls Monitor.Enter directly must reproduce that pattern.

Acquired but never released in the same method. A lock has to be released by the very method that took it. When a method acquires a monitor and then simply returns — or leaves the release to a separate Release() style method that callers may forget, may call in the wrong order, or may never reach because an exception unwound the stack — the monitor stays held for the lifetime of the process and every later acquirer deadlocks. Pairing acquire and release inside one method is what makes the pairing verifiable at all: a reader of the method can see both ends of the critical section.

Released, but not on every exit path. Even when the release lives in the same method, placing it after the critical section instead of inside a finally means any exception thrown in between — or any return, break or continue that jumps over it — leaks the lock exactly as if it were never written.

public void NeverReleased(object sync)
{
    Monitor.Enter(sync);                // FLAW — nothing in this method releases the monitor
    DoWork();
}

public void ReleasedElsewhere(object sync)
{
    Monitor.Enter(sync);                // FLAW — the release is another method's problem
}

public void Release(object sync)
{
    Monitor.Exit(sync);                 // callers may never reach this
}

public void SkippableRelease(object sync)
{
    Monitor.Enter(sync);                // FLAW — no try/finally guarding the release
    DoWork();
    Monitor.Exit(sync);                 // skipped if DoWork() throws
}

public void Good(object sync)
{
    Monitor.Enter(sync);                // OK — paired with Monitor.Exit in finally
    try
    {
        DoWork();
    }
    finally
    {
        Monitor.Exit(sync);
    }
}

public void Best(object sync)
{
    lock (sync)                         // OK — compiler emits the try/finally pair
    {
        DoWork();
    }
}

Remediation

Prefer the lock statement whenever possible — the compiler generates the correct try/finally automatically, and it makes acquire and release structurally inseparable.

If Monitor.Enter must be called directly (for example to use TryEnter with a timeout), release the monitor in the same method and do it from a finally: Monitor.Enter(sync); try { …​ } finally { Monitor.Exit(sync); }. Never split the acquire and the release across two methods — no caller can be relied on to invoke the releasing method on every path, exceptional or not.

References