Linked cache entry thread safety fix - #6
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: dotnet#131931 Source head: 7f28947
⛔ Shipwright · BlockedRecommendation: do not merge PR #6 · Tier
Findings (16)
Fireworks usage: 24,906 input · 2,051 output · 26,957 total tokens · $0.0068 · 23s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| _gate = gate; | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Shipwright · CRITICAL
'Snapshot' uses 'Volatile.Read(ref state._count)', but the builder path writes '_count' with plain assignments in 'SetCount', 'Insert', 'Clear', and 'RemoveAtBuilder'.
Impact: 'Snapshot' uses 'Volatile.Read(ref state._count)', but the builder path writes '_count' with plain assignments in 'SetCount', 'Insert', 'Clear', and 'RemoveAtBuilder'. After 'EnableConcurrentReads', a reader can observe a stale count while array contents have already changed, or observe a new count before the corresponding array writes are visible, yielding an inconsistent snapshot.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| public void CopyTo(IChangeToken[] array, int arrayIndex) | ||
| { | ||
| State state = _state; |
There was a problem hiding this comment.
Shipwright · CRITICAL
'EnableConcurrentReads' only sets the flag and does not publish the current '_state' with a volatile write or memory barrier.
Impact: 'EnableConcurrentReads' only sets the flag and does not publish the current '_state' with a volatile write or memory barrier. A reader that starts after the flag is set may still see a stale '_state' reference from before the transition, so the lock-free snapshot can miss tokens that were added before concurrent reads were enabled.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| { | ||
| State state = _state; | ||
| IChangeToken[] items = state._items; | ||
| int count = Volatile.Read(ref state._count); |
There was a problem hiding this comment.
Shipwright · CRITICAL
The '_concurrentReadsEnabled' flag is written under '_gate' in 'EnableConcurrentReads' but read without any synchronization or volatile semantics in 'EnsureCapacity', 'SetCount', '
Impact: The '_concurrentReadsEnabled' flag is written under '_gate' in 'EnableConcurrentReads' but read without any synchronization or volatile semantics in 'EnsureCapacity', 'SetCount', 'Add', 'AddRange', 'Insert', 'RemoveAt', 'Clear', 'Remove', and the indexer setter. A writer that has just enabled concurrent reads can continue observing the old flag value and mutate the shared array in place while readers walk it lock-fr…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| { | ||
| State state = _state; | ||
| IChangeToken[] items = state._items; | ||
| int count = Volatile.Read(ref state._count); |
There was a problem hiding this comment.
Shipwright · HIGH
'EnableConcurrentReads' sets '_concurrentReadsEnabled = true' under the lock, but the flag is read without synchronization in 'EnsureCapacity', 'SetCount', 'Add', 'AddRange', 'Inse
Impact: 'EnableConcurrentReads' sets '_concurrentReadsEnabled = true' under the lock, but the flag is read without synchronization in 'EnsureCapacity', 'SetCount', 'Add', 'AddRange', 'Insert', 'RemoveAt', 'Clear', 'Remove', and the indexer setter. Since the flag is not volatile, a writer thread that has just enabled concurrent reads may continue to see the old value and mutate the shared array in place, while readers now wa…
Suggested fix: Fix the review finding before release.
| _gate = gate; | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
Shipwright · HIGH
'Snapshot' reads 'state._count' with 'Volatile.Read', but '_count' is written with a plain assignment in the builder path ('state._count = count' in 'SetCount', 'Insert', 'Clear',
Impact: 'Snapshot' reads 'state._count' with 'Volatile.Read', but '_count' is written with a plain assignment in the builder path ('state._count = count' in 'SetCount', 'Insert', 'Clear', and 'RemoveAtBuilder'). After 'EnableConcurrentReads' is called, a reader can observe a stale count while the array contents have already been updated, or observe a new count before the corresponding array writes are visible, yielding an i…
Suggested fix: Fix the review finding before release.
|
|
||
| public void CopyTo(IChangeToken[] array, int arrayIndex) | ||
| { | ||
| State state = _state; |
There was a problem hiding this comment.
Shipwright · HIGH
'EnableConcurrentReads' only sets the flag and does not publish the current '_state' with a volatile write or memory barrier.
Impact: 'EnableConcurrentReads' only sets the flag and does not publish the current '_state' with a volatile write or memory barrier. A reader that starts after the flag is set may still see a stale '_state' reference from before the transition, so the lock-free snapshot can miss tokens that were added before concurrent reads were enabled.
Suggested fix: Fix the review finding before release.
| if (!_concurrentReadsEnabled) | ||
| { | ||
| RemoveAtBuilder(state, index); | ||
| } |
There was a problem hiding this comment.
Shipwright · HIGH
'GetEnumerator' captures 'items' and 'count' once, but 'items' is a mutable array that can be replaced or mutated by a concurrent writer.
Impact: 'GetEnumerator' captures 'items' and 'count' once, but 'items' is a mutable array that can be replaced or mutated by a concurrent writer. If a writer grows the array or clears it while the enumerator is active, the enumerator can throw 'IndexOutOfRangeException' or return inconsistent results because it reads 'items[i]' without synchronization.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| get | ||
| { | ||
| State state = _state; | ||
| return new ReadOnlySpan<IChangeToken>(state._items, 0, Volatile.Read(ref state._count)); |
There was a problem hiding this comment.
Shipwright · HIGH
'Count' reads 'state._count' with 'Volatile.Read', but the builder path writes '_count' with a plain assignment.
Impact: 'Count' reads 'state._count' with 'Volatile.Read', but the builder path writes '_count' with a plain assignment. After concurrent reads are enabled, a reader can observe a stale count that does not reflect the latest additions or removals, causing callers to iterate over an incorrect number of tokens.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| { | ||
| State state = _state; | ||
| int count = state._count; | ||
| int index = Array.IndexOf(state._items, item, 0, count); |
There was a problem hiding this comment.
Shipwright · HIGH
'CopyTo' reads 'state._count' with 'Volatile.Read' but then copies from 'state._items' without ensuring the array length matches the observed count.
Impact: 'CopyTo' reads 'state._count' with 'Volatile.Read' but then copies from 'state._items' without ensuring the array length matches the observed count. A concurrent 'Clear' or 'RemoveAtConcurrent' can replace '_state' with a smaller array after the count is read, causing 'Array.Copy' to throw 'ArgumentException' or copy stale data.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| _state = s_empty; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Shipwright · HIGH
'IndexOf' and 'Contains' read 'state._count' with 'Volatile.Read' but then call 'Array.IndexOf' on 'state._items' without synchronization.
Impact: 'IndexOf' and 'Contains' read 'state._count' with 'Volatile.Read' but then call 'Array.IndexOf' on 'state._items' without synchronization. A concurrent writer can replace the array or mutate its contents, causing the search to operate on a different array than the count was read from, leading to incorrect results or an 'IndexOutOfRangeException'.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| State state = _state; | ||
| return Volatile.Read(ref state._count); | ||
| } | ||
| } |
There was a problem hiding this comment.
Shipwright · HIGH
The indexer getter reads 'state._count' with 'Volatile.Read' and then accesses 'state._items[index]' without synchronization.
Impact: The indexer getter reads 'state._count' with 'Volatile.Read' and then accesses 'state._items[index]' without synchronization. A concurrent writer can replace '_state' with a smaller array after the count check, causing an 'IndexOutOfRangeException' or returning a token from a stale array.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| if (!_concurrentReadsEnabled) | ||
| { | ||
| RemoveAtBuilder(state, index); | ||
| } |
There was a problem hiding this comment.
Shipwright · MEDIUM
'GetEnumerator' captures 'items' and 'count' once, but 'items' is a mutable array that can be replaced or mutated by a concurrent writer.
Impact: 'GetEnumerator' captures 'items' and 'count' once, but 'items' is a mutable array that can be replaced or mutated by a concurrent writer. If a writer grows the array or clears it while the enumerator is active, the enumerator can throw 'IndexOutOfRangeException' or return inconsistent results because it reads 'items[i]' without synchronization.
Suggested fix: Fix the review finding before release.
| { | ||
| State state = _state; | ||
| int count = state._count; | ||
| int index = Array.IndexOf(state._items, item, 0, count); |
There was a problem hiding this comment.
Shipwright · MEDIUM
'CopyTo' reads 'state._count' with 'Volatile.Read' but then copies from 'state._items' without ensuring the array length matches the observed count.
Impact: 'CopyTo' reads 'state._count' with 'Volatile.Read' but then copies from 'state._items' without ensuring the array length matches the observed count. A concurrent 'Clear' or 'RemoveAtConcurrent' can replace '_state' with a smaller array after the count is read, causing 'Array.Copy' to throw 'ArgumentException' or copy stale data.
Suggested fix: Fix the review finding before release.
| _state = s_empty; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Shipwright · MEDIUM
'IndexOf' and 'Contains' read 'state._count' with 'Volatile.Read' but then call 'Array.IndexOf' on 'state._items' without synchronization.
Impact: 'IndexOf' and 'Contains' read 'state._count' with 'Volatile.Read' but then call 'Array.IndexOf' on 'state._items' without synchronization. A concurrent writer can replace the array or mutate its contents, causing the search to operate on a different array than the count was read from, leading to incorrect results or an 'IndexOutOfRangeException'.
Suggested fix: Fix the review finding before release.
| State state = _state; | ||
| return Volatile.Read(ref state._count); | ||
| } | ||
| } |
There was a problem hiding this comment.
Shipwright · MEDIUM
The indexer getter reads 'state._count' with 'Volatile.Read' and then accesses 'state._items[index]' without synchronization.
Impact: The indexer getter reads 'state._count' with 'Volatile.Read' and then accesses 'state._items[index]' without synchronization. A concurrent writer can replace '_state' with a smaller array after the count check, causing an 'IndexOutOfRangeException' or returning a token from a stale array.
Suggested fix: Fix the review finding before release.
| get | ||
| { | ||
| State state = _state; | ||
| return new ReadOnlySpan<IChangeToken>(state._items, 0, Volatile.Read(ref state._count)); |
There was a problem hiding this comment.
Shipwright · MEDIUM
'Count' reads 'state._count' with 'Volatile.Read', but the builder path writes '_count' with a plain assignment.
Impact: 'Count' reads 'state._count' with 'Volatile.Read', but the builder path writes '_count' with a plain assignment. After concurrent reads are enabled, a reader can observe a stale count that does not reflect the latest additions or removals, causing callers to iterate over an incorrect number of tokens.
Suggested fix: Fix the review finding before release.
1 participant
Fixes dotnet#46032
Source merge-base:
bdec678032fd579854e525c5c309eac1c1dd22c8Source head:
7f289470e1cefa681b9cdcb8c17314616120e9f2