Inconsistently synchronized property¶
ID: cs/unsynchronized-getter
Kind: problem
Security severity:
Severity: error
Precision: medium
Tags:
- correctness
- concurrency
- external/cwe/cwe-662
Query suites:
- csharp-security-and-quality.qls
Click to see the query in the CodeQL repository
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¶
MSDN, C# Reference: lock Statement.
Wikipedia: Memory barrier.
Common Weakness Enumeration: CWE-662.