CodeQL queries 1.23

Skip to end of metadata
Go to start of metadata

Name: Inconsistently synchronized property

Description: If a property has a lock in its setter, but not in its getter, then the value returned by the getter can be inconsistent.

ID: cs/unsynchronized-getter

Kind: problem

Severity: error

Precision: medium

Query: SynchSetUnsynchGet.ql
/**
 * @name Inconsistently synchronized property
 * @description If a property has a lock in its setter, but not in its getter,
 *              then the value returned by the getter can be inconsistent.
 * @kind problem
 * @problem.severity error
 * @precision medium
 * @id cs/unsynchronized-getter
 * @tags correctness
 *       concurrency
 *       external/cwe/cwe-662
 */

import csharp

from Property p, Field f
where
  f.getDeclaringType() = p.getDeclaringType() and
  exists(Setter setter, LockStmt writelock, FieldWrite writeaccess |
    p.getSetter() = setter and
    writeaccess = f.getAnAccess() and
    writelock.getEnclosingCallable() = setter and
    writelock.getAChildStmt+().getAChildExpr+() = writeaccess
  ) and
  exists(Getter getter, FieldRead readaccess |
    getter = p.getGetter() and
    readaccess = f.getAnAccess() and
    readaccess.getEnclosingCallable() = getter and
    not exists(LockStmt readlock | readlock.getAChildStmt+().getAChildExpr+() = readaccess)
  )
select p, "Field '$@' is guarded by a lock in the setter but not in the getter.", f, f.getName()

Data which is accessed concurrently from multiple threads is vulnerable to race conditions and other errors. lock statements are often needed to make concurrent code correct and ensure that the data is consistent. However lock statements must be used consistently on all methods which are potentially called concurrently.

When there is a lock statement on a property setter, it implies that the property could be assigned to concurrently, so the property could also be read concurrently. Therefore a lock statement is necessary on the property getter. Even simple getters require a lock statement to ensure that there is a memory barrier when reading a field.

Recommendation

Examine the logic of the program to check whether the property could be read concurrently. Add a lock statement in the property getter if necessary.

Alternatively, remove the lock statement from the property setter if it is no longer needed.

Example

This example shows a property setter which uses a lock statement, but there is no corresponding lock statement in the getter. This means that count is not synchronized with GenerateDiagnostics(), and there is a read barrier missing from the property getter, which could cause bugs on some CPU architectures.

public int ErrorCount
{
    get
    {
        return count;
    }

    set
    {
        lock (mutex)
        {
            count = value;
            if (count > 0) GenerateDiagnostics();
        }
    }
}

The solution is to add a lock statement to the property getter, as follows:

public int ErrorCount
{
    get
    {
        lock (mutex)
        {
            return count;
        }
    }

    set
    {
        lock (mutex)
        {
            count = value;
            if (count > 0) GenerateDiagnostics();
        }
    }
}

References