CodeQL queries 1.23

Skip to end of metadata
Go to start of metadata

Name: Double-checked lock is not thread-safe

Description: A repeated check on a non-volatile field is not thread-safe on some platforms, and could result in unexpected behavior.

ID: cs/unsafe-double-checked-lock

Kind: problem

Severity: error

Precision: medium

Query: UnsafeLazyInitialization.ql
/**
 * @name Double-checked lock is not thread-safe
 * @description A repeated check on a non-volatile field is not thread-safe on some platforms,
 *              and could result in unexpected behavior.
 * @kind problem
 * @problem.severity error
 * @precision medium
 * @id cs/unsafe-double-checked-lock
 * @tags correctness
 *       concurrency
 *       external/cwe/cwe-609
 */

import csharp
import semmle.code.csharp.commons.StructuralComparison

class DoubleCheckedLock extends StructuralComparisonConfiguration {
  DoubleCheckedLock() { this = "double checked lock" }

  override predicate candidate(ControlFlowElement x, ControlFlowElement y) {
    exists(IfStmt unlockedIf, IfStmt lockedIf, LockStmt lock |
      x = unlockedIf.getCondition() and
      y = lockedIf.getCondition() and
      lock = unlockedIf.getThen().stripSingletonBlocks() and
      lockedIf.getParent*() = lock.getBlock()
    )
  }
}

predicate doubleCheckedLock(Field field, IfStmt ifs) {
  exists(DoubleCheckedLock config, LockStmt lock, Expr eq1, Expr eq2 | ifs.getCondition() = eq1 |
    lock = ifs.getThen().stripSingletonBlocks() and
    config.same(eq1, eq2) and
    field.getAnAccess() = eq1.getAChildExpr*()
  )
}

from Field field, IfStmt ifs
where
  doubleCheckedLock(field, ifs) and
  not field.isVolatile() and
  exists(VariableWrite write | write = ifs.getThen().getAChild+() and write.getTarget() = field) and
  field.getType() instanceof RefType
select ifs, "Field $@ should be 'volatile' for this double-checked lock.", field, field.getName()

Double-checked locking requires that the underlying field is volatile, otherwise the program can behave incorrectly when running in multiple threads, for example by computing the field twice.

Recommendation

There are several ways to make the code thread-safe:

  1. Avoid double-checked locking, and simply perform everything within the lock statement.
  2. Make the field volatile using the volatile keyword.
  3. Use the System.Lazy class, which is guaranteed to be thread-safe. This can often lead to more elegant code.
  4. Use System.Threading.LazyInitializer.
Example

The following code defines a property called Name, which calls the method LoadNameFromDatabase the first time that the property is read, and then caches the result. This code is efficient but will not work properly if the property is accessed from multiple threads, because LoadNameFromDatabase could be called several times.

string name;

public string Name
{
    get
    {
        // BAD: Not thread-safe
        if (name == null)
            name = LoadNameFromDatabase();
        return name;
    }
}

A common solution to this is double-checked locking, which checks whether the stored value is null before locking the mutex. This is efficient because it avoids a potentially expensive lock operation if a value has already been assigned to name.

string name;    // BAD: Not thread-safe

public string Name
{
    get
    {
        if (name == null)
        {
            lock (mutex)
            {
                if (name == null)
                    name = LoadNameFromDatabase();
            }
        }
        return name;
    }
}

However this code is incorrect because the field name isn't volatile, which could result in name being computed twice on some systems.

The first solution is to simply avoid double-checked locking (Recommendation 1):

string name;

public string Name
{
    get
    {
        lock (mutex)    // GOOD: Thread-safe
        {
            if (name == null)
                name = LoadNameFromDatabase();
            return name;
        }
    }
}

Another fix would be to make the field volatile (Recommendation 2):

volatile string name;    // GOOD: Thread-safe

public string Name
{
    get
    {
        if (name == null)
        {
            lock (mutex)
            {
                if (name == null)
                    name = LoadNameFromDatabase();
            }
        }
        return name;
    }
}

It may often be more elegant to use the class System.Lazy, which is automatically thread-safe (Recommendation 3):

Lazy<string> name;    // GOOD: Thread-safe

public Person()
{
    name = new Lazy<string>(LoadNameFromDatabase);
}

public string Name => name.Value;

References