CWE-362: Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition') - C
Overview
Race conditions in .NET usually come from shared mutable state on a singleton service, a static field, or an in-memory cache, combined with ASP.NET Core's default of invoking singleton-scoped services concurrently from every in-flight request. The lock statement (backed by Monitor) protects a synchronous critical section, but the compiler rejects await inside a lock block (CS1996), so async code needs SemaphoreSlim instead. Single-variable counters and flags are better served by System.Threading.Interlocked, which avoids locking entirely. For state backed by a relational database - the more common case in web applications, where the "shared resource" is a row that any server instance can touch - prefer EF Core's [Timestamp]/RowVersion optimistic concurrency or an explicit SELECT ... FOR UPDATE-equivalent transaction over an application-level lock, since a lock only protects the process that holds it.
Common Vulnerable Patterns
Unsynchronized Singleton Counter
// VULNERABLE - a singleton service's field is mutated with no synchronization
public class RequestCounterService
{
private int _count;
public void Increment()
{
// read-modify-write: two concurrent calls can both read the same
// value before either writes it back, losing an increment
_count = _count + 1;
}
public int Count => _count;
}
// Attack: fire 1,000 concurrent requests, each calling Increment() once.
// Result: Count often ends up less than 1,000 - some increments are lost.
Why this is vulnerable: ASP.NET Core invokes a singleton service concurrently for every request that resolves it. _count = _count + 1 is a read, an add, and a write as three separate steps; if two threads interleave between the read and the write, both compute the same new value from the same stale read and one increment is silently lost.
Check-Then-Act on a Shared In-Memory Balance
// VULNERABLE - check and update are two separate, unsynchronized steps
public class WalletService
{
private readonly Dictionary<string, decimal> _balances = new();
public void Withdraw(string accountId, decimal amount)
{
if (_balances[accountId] < amount)
throw new InvalidOperationException("insufficient funds");
// RACE WINDOW: another concurrent call can withdraw here before this line runs
_balances[accountId] -= amount;
}
}
// Attack: two concurrent Withdraw(accountId, 100) calls when the balance is 100.
// Both calls read balance = 100 and both pass the check before either writes.
// Result: the balance goes to -100 instead of the second call being rejected.
Why this is vulnerable: The balance check and the deduction are not atomic with respect to each other. A Dictionary<TKey, TValue> is not thread-safe for concurrent reads and writes either, which can additionally corrupt the dictionary's internal state under load, not just the balance value.
EF Core Update Without a Concurrency Token
// VULNERABLE - no concurrency token, the second SaveChangesAsync silently
// overwrites the first
public async Task WithdrawAsync(int accountId, decimal amount)
{
var account = await _db.Accounts.FindAsync(accountId);
if (account is null || account.Balance < amount)
throw new InvalidOperationException("insufficient funds");
account.Balance -= amount;
await _db.SaveChangesAsync();
}
// Attack: two concurrent WithdrawAsync(accountId, 100) calls when the balance is 100.
// Both load balance = 100, both pass the check, both save balance = 0.
// One withdrawal is completely lost - the balance should be -100 or rejected.
Why this is vulnerable: Without a [Timestamp]/[ConcurrencyCheck] column, EF Core's SaveChangesAsync() issues a plain UPDATE ... WHERE Id = @id with no check that the row is unchanged since it was read. The second transaction's write silently replaces the first's, a classic lost update.
Secure Patterns
lock for a Synchronous Critical Section
using System;
using System.Collections.Generic;
// SECURE - lock protects the full read-modify-write sequence
public class WalletService
{
private readonly object _lock = new();
private readonly Dictionary<string, decimal> _balances = new();
public void Withdraw(string accountId, decimal amount)
{
lock (_lock)
{
if (!_balances.TryGetValue(accountId, out var balance) || balance < amount)
throw new InvalidOperationException("insufficient funds");
_balances[accountId] = balance - amount;
}
}
}
Why this works: lock (_lock) guarantees only one thread executes the block at a time, so the check and the write happen as one atomic unit from every caller's perspective. The lock object is a dedicated, private readonly object - never this, a boxed value type, or a string literal, all of which can be inadvertently shared with unrelated code and cause deadlocks or ineffective locking.
On .NET 9 and later, declare the field as System.Threading.Lock rather than object:
The lock statement recognises the type and calls Lock.EnterScope() instead of Monitor.Enter/Monitor.Exit, which is faster and, more usefully here, makes the field's purpose part of its type - Monitor.Enter(_lock) on some other object is no longer an easy mistake to make in the same class. Verified on .NET 10 that both forms compile and serialise correctly: 100,000 Parallel.For increments under a Lock, under an object, and under an explicit using (_lock.EnterScope()) each produced exactly 100,000. Note the trap in the upgrade: changing a field's type from object to Lock changes the meaning of every lock on it, so any code that also passed that field to Monitor.Enter directly, or awaited while holding it, needs re-reading rather than a find-and-replace.
SemaphoreSlim for an Async Critical Section
using System;
using System.Threading;
using System.Threading.Tasks;
// SECURE - SemaphoreSlim guards an async critical section (lock cannot wrap an await)
public class AsyncWalletService
{
private readonly SemaphoreSlim _semaphore = new(1, 1);
private decimal _balance;
public AsyncWalletService(decimal initialBalance) => _balance = initialBalance;
public decimal Balance => _balance;
public async Task WithdrawAsync(decimal amount)
{
await _semaphore.WaitAsync();
try
{
if (_balance < amount)
throw new InvalidOperationException("insufficient funds");
_balance -= amount;
}
finally
{
_semaphore.Release();
}
}
}
Why this works: SemaphoreSlim initialized with a count of 1 behaves as an async-aware mutex: WaitAsync() suspends the caller without blocking a thread until the semaphore is free, and only one caller can be inside the try block at a time. The finally block guarantees Release() runs even if the critical section throws, so a failed withdrawal cannot leave the semaphore permanently held.
Interlocked for a Simple Counter
using System.Threading;
// SECURE - Interlocked.Increment avoids the need for a lock on a single counter
public class RequestCounterService
{
private int _count;
public void Increment() => Interlocked.Increment(ref _count);
public int Count => Interlocked.CompareExchange(ref _count, 0, 0);
}
Why this works: Interlocked.Increment is a single hardware-level atomic operation - there is no window in which two threads can both read the same value before either writes it back. Reading _count through Interlocked.CompareExchange(ref _count, 0, 0) (a no-op compare-and-swap) ensures the read observes the latest value with the correct memory ordering, rather than a potentially stale, cached value.
ConcurrentDictionary with an Explicit Compare-and-Swap Loop
using System.Collections.Concurrent;
// SECURE - TryUpdate is a compare-and-swap: it applies the decrement only if
// the value is still the one this call decided from, and the loop re-reads
public class InventoryService
{
private readonly ConcurrentDictionary<string, int> _stock = new();
public bool TryReserve(string sku, int quantity)
{
while (true)
{
if (!_stock.TryGetValue(sku, out var current))
return false; // unknown SKU
if (current < quantity)
return false; // not enough stock
if (_stock.TryUpdate(sku, current - quantity, current))
return true; // our decrement won
// Another thread changed the value between the read and the write.
// Discard the decision and make it again against the new value -
// re-reading here is the whole point of the loop.
}
}
}
Why this works: ConcurrentDictionary guarantees thread-safe individual operations, but a separate TryGetValue followed by an unconditional TryUpdate or an indexer assignment is still a check-then-act race even on a concurrent collection. TryUpdate(key, newValue, comparisonValue) closes it by carrying the value the decision was made from: the write lands only if the stored value is still comparisonValue, so a concurrent change cannot be overwritten - it makes this call's TryUpdate return false and go round again. Verified on .NET 10: 200 concurrent single-unit reservations against 100 units granted exactly 100 and left the stock at 0, over five runs; a single TryReserve(3) against 10 returned true and left 7; TryReserve(1) against 0 returned false; an unknown SKU returned false without creating an entry.
AddOrUpdate is the wrong tool here, and its delegates are not atomic. Microsoft documents that ConcurrentDictionary's factory delegates run outside the collection's internal locks - so that unknown user code never executes under one - and that they may therefore be invoked more than once for a single call when another thread updates the same key first. Measured on .NET 10 with 8 threads doing 20,000 AddOrUpdate calls each: 160,000 calls produced 277,600 updateValueFactory invocations, 73% more than one apiece. The stored value is still correct, because the retry re-reads and re-runs the delegate - which is exactly what hides the problem. What is not correct is anything the delegate did. Replacing the returned value with a side effect - incrementing an audit counter inside the delegate - produced 265,406 audit entries for 160,000 reservations in the same run: 105,406 records of reservations that never happened.
So AddOrUpdate and GetOrAdd are safe where the factory is a pure function of the current value ((_, v) => v + 1), because re-running it costs nothing but CPU. They are not safe for a factory that writes to a captured local, appends to a log, publishes an event, allocates a scarce resource, or increments a counter. Where the decision has to be recorded, use the explicit TryUpdate loop above so the retry is visible in your own code, or take a lock over the whole check-and-act.
EF Core Optimistic Concurrency with RowVersion
using System;
using Microsoft.EntityFrameworkCore;
public class Account
{
public int Id { get; set; }
public decimal Balance { get; set; }
[Timestamp] // SECURE - EF Core includes this column in the WHERE clause and
// regenerates it on every UPDATE
public byte[] RowVersion { get; set; } = Array.Empty<byte>();
}
// SECURE - the UPDATE fails if RowVersion changed since the row was read
public async Task<bool> TryWithdrawAsync(int accountId, decimal amount)
{
var account = await _db.Accounts.FindAsync(accountId);
if (account is null || account.Balance < amount)
return false;
account.Balance -= amount;
try
{
await _db.SaveChangesAsync();
return true;
}
catch (DbUpdateConcurrencyException)
{
// another transaction modified this row first; reload and let the
// caller retry rather than silently overwriting the other write
return false;
}
}
Why this works: [Timestamp] marks RowVersion as a concurrency token; EF Core generates an UPDATE ... WHERE Id = @id AND RowVersion = @originalRowVersion and the database driver automatically produces a new value for RowVersion on every write. If a concurrent transaction already updated the row, the WHERE clause matches zero rows, EF Core detects the mismatch, and throws DbUpdateConcurrencyException instead of silently applying a stale write.
Database Row Lock for Cross-Process State
using System.Data;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Storage;
// SECURE - an explicit transaction plus a conditional UPDATE serializes the
// check-and-deduct across every process and server instance touching the row
public async Task<bool> WithdrawAtomicAsync(int accountId, decimal amount)
{
await using IDbContextTransaction tx =
await _db.Database.BeginTransactionAsync(IsolationLevel.ReadCommitted);
var affected = await _db.Database.ExecuteSqlInterpolatedAsync(
$"""
UPDATE Accounts
SET Balance = Balance - {amount}
WHERE Id = {accountId} AND Balance >= {amount}
""");
await tx.CommitAsync();
return affected == 1;
}
Why this works: The precondition (Balance >= @amount) and the write happen inside a single UPDATE statement the database executes atomically, so there is no separate read for another transaction to interleave with. This does not depend on any in-process lock, so it correctly serializes concurrent requests even when they land on different application server instances behind a load balancer - the database is the single source of truth for the resource.
Framework-Specific Guidance
ASP.NET Core
// SECURE - register the wallet service as a singleton, but its internal state
// is protected by the lock/semaphore shown above, not by DI lifetime alone
builder.Services.AddSingleton<WalletService>();
Why this works: Registering a service as AddSingleton does not make its state race-free - it guarantees the opposite: every concurrent request shares the same instance. Do not assume a scoped or transient lifetime avoids races either, since scoped services can still read and write resources (a database row, a static cache) that are genuinely shared across the whole application; the synchronization has to protect the resource itself, not rely on DI lifetime to keep instances apart.
Entity Framework Core
// SECURE - retry on a concurrency conflict instead of surfacing it as a hard failure
public async Task WithdrawWithRetryAsync(int accountId, decimal amount, int maxAttempts = 3)
{
for (var attempt = 1; attempt <= maxAttempts; attempt++)
{
var account = await _db.Accounts.FindAsync(accountId);
if (account is null || account.Balance < amount)
throw new InvalidOperationException("insufficient funds");
account.Balance -= amount;
try
{
await _db.SaveChangesAsync();
return;
}
catch (DbUpdateConcurrencyException) when (attempt < maxAttempts)
{
_db.Entry(account).State = EntityState.Detached; // reload on next loop
}
}
throw new InvalidOperationException("could not complete withdrawal, please retry");
}
Why this works: Optimistic concurrency correctly rejects a conflicting write, but that rejection needs a handling strategy. Detaching the stale entity and reloading it on the next iteration re-reads the current balance and RowVersion before retrying, so a losing transaction gets a fair second attempt against fresh data instead of failing outright on the first conflict.
Considerations
A lock is only as wide as the thing holding it. An in-process lock serialises the threads inside one instance and does nothing about a second instance, so a fix that works on a developer machine can fail the moment the service is scaled out or restarted behind a load balancer. Decide first where the shared state actually lives. If it is a database row, the serialisation has to happen in the database - a row lock, a conditional update, or a version column. If it is genuinely in-process and stays that way, an in-process lock is correct and cheaper.
Not every race is worth fixing. Two requests overwriting a display preference, or a page-view counter losing an increment, is a race with no security consequence and often no user-visible one. The ones that matter change a decision: a balance check, a quota, a one-time token being redeemed, a permission being evaluated. Fixing a benign race costs throughput and adds a failure mode, so say which category the finding is in before reaching for a lock.
lock cannot span an await. The compiler will not let you, and the
workaround people reach for - releasing before the await and reacquiring after -
reopens the window the lock existed to close. Use SemaphoreSlim with
WaitAsync() for critical sections that contain asynchronous work, and keep the
section as small as the invariant allows.
Optimistic and pessimistic locking fail in opposite directions. A pessimistic lock makes every caller wait, so it is predictable but caps throughput and can deadlock if two paths take locks in different orders. Optimistic concurrency lets callers proceed and rejects the loser, which is faster when conflicts are rare and degenerates into wasted work and retries when they are common. Pick by how often the same row is genuinely contended, not by which is easier to write.
Retries need a bound and a backoff. A conflict-and-retry loop with neither turns a contended row into a livelock under load - every caller retrying immediately, none making progress. Cap the attempts, back off between them, and decide what the caller sees when the cap is reached. "Try again" is a legitimate answer; silently returning stale data is not.
Testing
A re-scan cannot confirm this fix. The analyzer sees a SemaphoreSlim where
there was none and reports the finding closed; it cannot tell whether the
critical section covers the whole decision, and it cannot tell a working lock
from one that rejects every legitimate caller. Both need assertions.
The harder problem is that the obvious concurrency test is a coin toss. Firing five tasks at the racy wallet on .NET 10, 200 times, the "correct" answer of 3 came up 69 times - so a third of runs report a pass against code that has the bug. Whether it reproduces is a property of the machine, and CI machines are usually the slowest and least parallel ones you own.
Pin the interleaving instead of hoping for it. Give the class under test a hook
between the check and the write, and make the hook a Barrier: every caller
that passed the check waits there until all of them have, so the stale-read
window is guaranteed rather than probable. The Barrier needs a timeout, so
that a correctly locked implementation - where only one caller is ever inside
the section - waits it out and completes instead of deadlocking.
// The class under test takes an optional hook. In production it is null;
// only the test passes one.
public class AsyncWalletService
{
private readonly SemaphoreSlim _semaphore = new(1, 1);
private readonly Action? _betweenCheckAndWrite;
private decimal _balance;
public AsyncWalletService(decimal initialBalance, Action? betweenCheckAndWrite = null)
{
_balance = initialBalance;
_betweenCheckAndWrite = betweenCheckAndWrite;
}
public decimal Balance => _balance;
public async Task WithdrawAsync(decimal amount)
{
await _semaphore.WaitAsync();
try
{
if (_balance < amount)
throw new InvalidOperationException("insufficient funds");
await Task.Yield(); // stands in for the awaited audit write
_betweenCheckAndWrite?.Invoke();
_balance -= amount;
}
finally
{
_semaphore.Release();
}
}
}
[Fact]
public async Task ConcurrentWithdrawals_NeverOverdraftTheAccount()
{
using var barrier = new Barrier(5);
var wallet = new AsyncWalletService(
initialBalance: 100m,
// Every caller that got past the balance check stops here until all
// five have. The timeout is what lets the locked version through:
// only one caller ever arrives, so it waits 200 ms and continues.
betweenCheckAndWrite: () => barrier.SignalAndWait(TimeSpan.FromMilliseconds(200)));
var tasks = Enumerable.Range(0, 5)
.Select(_ => Task.Run(async () =>
{
try { await wallet.WithdrawAsync(30m); return true; }
catch (InvalidOperationException) { return false; }
}));
var results = await Task.WhenAll(tasks);
// Exactly 3 of the 5 concurrent $30 withdrawals succeed against $100,
// and the balance lands on the arithmetic those 3 imply.
Assert.Equal(3, results.Count(succeeded => succeeded));
Assert.Equal(10m, wallet.Balance);
}
Measured on .NET 10 over 30 runs, this returns 5 successes every time against
the unsynchronized version and 3 every time against the SemaphoreSlim one.
The naive version returned 3, 4 and 5 in roughly equal proportions against the
same broken code.
Assert these, with the result each should produce:
- Accept, single call.
WithdrawAsync(30)against a balance of 100 completes and leavesBalanceat exactly 70. A lock that refuses every caller passes the concurrency test above and fails this one. - Accept, at the boundary.
WithdrawAsync(100)against 100 completes and leaves 0. Off-by-one guards (<=where<was meant) surface here and nowhere else. - Reject, past the boundary.
WithdrawAsync(1)against 0 throwsInvalidOperationExceptionand leavesBalanceat 0 - the balance is unchanged, not merely non-negative. - Concurrent. The barrier test above: 3 successes,
Balance == 10. Assert the balance as well as the count, because a lost update can produce the right number of successes and the wrong total. - No caller is left holding the semaphore. After a run in which some withdrawals threw,
WithdrawAsyncon the same instance still completes. ARelease()that only runs on the success path passes every test above and deadlocks the sixth caller.
Common Pitfalls
- Locking on a value that can be shared elsewhere:
lock (this),lock ("account-lock"), or locking a boxed value type all risk locking on an object another, unrelated piece of code can also lock on (or, for boxed values, a different object each time), which either deadlocks or fails to exclude concurrent access. Use a dedicatedprivate readonly objectper resource. - Using
ConcurrentDictionarybut writing back without comparing: The collection's individual operations are thread-safe, butTryGetValuefollowed bydict[key] = newValue(or byTryUpdatepassing a fresh read as the comparison value) is still a check-then-act race - another thread can modify the value in between and the write silently overwrites it. Pass the value the decision was made from asTryUpdate'scomparisonValue, and loop. - Assuming an
AddOrUpdate/GetOrAddfactory runs once, under the lock: it does neither. The delegates run outside the collection's locks and are re-invoked when a concurrent write beats them - 160,000 calls produced 277,600 invocations on .NET 10 - so a factory with any side effect performs it once per attempt, not once per call. Fine for a pure function of the current value; wrong for anything that records a decision. - Catching
DbUpdateConcurrencyExceptionand discarding it: Swallowing the exception without reloading and retrying (or surfacing the conflict to the caller) makes the concurrency token pointless - the conflict is detected but the losing write's data is dropped with no indication to the user or caller. - Forgetting
SemaphoreSlim.Release()on an exception path: Acquiring withWaitAsync()outside atry/finally, or releasing only on the success path, permanently reduces the semaphore's count on every failure and eventually deadlocks every caller waiting on it.
Dependencies and Installation
No additional package is required for lock, SemaphoreSlim, Interlocked, or System.Collections.Concurrent - all are part of the base class library. [Timestamp]/optimistic concurrency and ExecuteSqlInterpolatedAsync require Microsoft.EntityFrameworkCore; no extra package is needed beyond the EF Core provider already in use.