CodeQL documentation
CodeQL resources

Futile synchronization on field

ID: cs/unsafe-sync-on-field
Kind: problem
Security severity: 
Severity: error
Precision: high
Tags:
   - reliability
   - correctness
   - concurrency
   - external/cwe/cwe-662
   - external/cwe/cwe-366
Query suites:
   - csharp-security-and-quality.qls

Click to see the query in the CodeQL repository

A lock statement that locks a field which is modified is unlikely to provide thread safety. This is because different threads only lock the value of the field, not the field itself. Since the value of the field changes, different threads are locking different objects, and so are not mutually exclusive.

Recommendation

Instead of locking the field itself, use a dedicated object for locking. The object should be private and readonly to ensure that it cannot be modified or locked elsewhere.

Example

In the following example, the method AddItem can be called concurrently on different threads. AddItem attempts to protect the field total using a lock statement. However concurrent use of AddItem results in the value of total being incorrect.

class Adder
{
    dynamic total = 0;

    public void AddItem(int item)
    {
        lock (total)     // Wrong
        {
            total = total + item;
        }
    }
}

The following code resolves this problem by using a dedicated object mutex for the lock.

using System;

class Adder
{
    dynamic total = 0;

    private readonly Object mutex = new Object();

    public void AddItem(int item)
    {
        lock (mutex)    // Fixed
        {
            total = total + item;
        }
    }
}

References

  • © GitHub, Inc.
  • Terms
  • Privacy