[IAST] Fix entry loss in DefaultTaintedMap under concurrency - #9041
Open
dromanol wants to merge 2 commits into
Open
[IAST] Fix entry loss in DefaultTaintedMap under concurrency#9041dromanol wants to merge 2 commits into
dromanol wants to merge 2 commits into
Conversation
Put published the chain head with a read-modify-write, and RemoveDeadKeys collected dead keys and removed them unconditionally in a second pass. Both drop entries when a request is served by more than one thread. Publish the head with a compare-and-swap, and make purging use compare-and-remove / compare-and-update so it never deletes an entry inserted concurrently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9041) and master. ✅ No regressions detected |
BenchmarksBenchmark execution time: 2026-08-14 09:57:27 Comparing candidate commit 7e36689 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 64 known flaky benchmarks, 62 flaky benchmarks without significant changes.
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://datadoghq.atlassian.net/browse/APPSEC-69539
Summary of changes
Make
DefaultTaintedMapinsertion and purging thread-safe. Both operations silently dropped tainted entries when a request was served by more than one thread.Reason for change
Two lost-update races, both reproducible:
Putpublished the chain head with a read-modify-write (TryGetValue+_map[index] = entry). The existing comment said "one of the ITaintedObjects could potentially be lost" — under contention the loss is 62% of entries, not an occasional one.RemoveDeadKeyscollected fully-dead keys into a list and removed them with an unconditionalTryRemovein a second pass, after walking all 16384 buckets. AnyPutlanding on one of those keys in that (very wide) window was deleted along with the dead chain.The effect is lost taint, i.e. IAST false negatives: vulnerabilities that should be reported aren't. Async continuations hopping threads within one request make this the common case, not a corner case.
Implementation details
Putpublishes the head with a compare-and-swap (TryUpdate/TryAdd) and retries on conflict. Flat mode keeps overwriting without chaining, as before.RemoveDeadKeysremoves inline instead of deferring to a second pass, using compare-and-remove for a fully-dead chain andTryUpdatefor replacing a dead head. If a concurrentPutwon the race, the purge leaves the key alone — the dead nodes are still chained and the next purge picks them up.ICollection<KeyValuePair<,>>.Remove, whichConcurrentDictionaryimplements as an atomic compare-and-remove.TryRemove(KeyValuePair<,>)only exists from netcoreapp2.0, and this assembly also targetsnet461/netstandard2.0._map[key]inRemoveDeadKeysbecameTryGetValue— it would throwKeyNotFoundExceptionagainst a concurrentClear().Reader-side note:
Getis left untouched on purpose. Unlinking only ever movesNextforward, so a concurrent chain walk can neither loop nor go backwards, and dead nodes have a nullValueso they never match. A new test covers that.Test coverage
New
DefaultTaintedMapConcurrencyTests, verified red before the fix and green after:Puton the same bucket (8 threads x 400)Purge()concurrent withPut()(2048 buckets)Get()while purging and insertingLiveness is controlled through a test
ITaintedObjectrather than the GC, so the tests are deterministic.Also run: 29/29
DefaultTaintedMaptests and 530/530 IAST unit tests onnet8.0, 29/29 onnet48, andDatadog.Tracebuilds clean onnet461,netstandard2.0,netcoreapp3.1andnet6.0.Other details
Found while investigating APPSEC-69539 (
ExecutionEngineExceptioninDefaultTaintedMap.Get). This does not fix that crash — everything reproduced here is data loss, not memory unsafety, and the reader-side test shows concurrent purging does not corrupt whatGetwalks. Filed separately because the taint loss is worth fixing on its own.🤖 Generated with Claude Code