Wrong Lock Usage

ID

csharp.wrong_lock_usage

Severity

high

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) that are not paired with a Monitor.Exit(x) call inside a finally block. Without that pairing, if the protected body throws, the lock is never released and the next thread that tries to acquire it deadlocks.

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); }. When code calls Monitor.Enter directly it must reproduce that pattern, otherwise an exception in the critical section leaks the lock for the lifetime of the process. This rule reports any Monitor.Enter/TryEnter whose lock target is not released by a matching Monitor.Exit inside the same scope’s finally clause.

public void Bad(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. If Monitor.Enter must be called directly (for example to use TryEnter with a timeout), wrap the critical section in try { …​ } finally { Monitor.Exit(sync); } so the lock is released on every code path, exceptional or not.

References