[AppSec] Dispose the WAF context when a non-web local root span closes - #9057
Open
dromanol wants to merge 4 commits into
Open
[AppSec] Dispose the WAF context when a non-web local root span closes#9057dromanol wants to merge 4 commits into
dromanol wants to merge 4 commits into
Conversation
A WAF context can be created on any local root span (the user events SDKs and the ASP.NET Core Identity integrations don't check the span type), but it was only disposed when the local root span was of type web, so those contexts kept their native memory until the finalizer ran, if ever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8efb27cc5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Disposing at local root span close cut off any child span still open on a non-web trace. Dispose when the last span of the segment closes instead, and serialize creation and disposal so a context created concurrently with the disposal can't be orphaned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BenchmarksBenchmark execution time: 2026-08-14 15:11:40 Comparing candidate commit c3c64ca in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 65 known flaky benchmarks, 61 flaky benchmarks without significant changes.
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9057) and master. ✅ No regressions detected |
Contributor
Author
|
@codex review |
Removed commented-out code regarding WAF context disposal.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f650d6462
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // this is the point where nothing can use it anymore. Disposing twice is a no-op. | ||
| // This has to happen while holding the lock, or AddSpan could reopen the segment | ||
| // in between and we would dispose the context of a span that is still running. | ||
| _appSecRequestContext?.DisposeAdditiveContext(); |
There was a problem hiding this comment.
When Task.Run captures a non-web root scope but starts its child only after the root closes, _openSpans reaches zero here and permanently disposes the additive context. The continuation's later StartActive still reuses the captured parent's TraceContext (Tracer.CreateSpanContext), and AddSpan permits the zero count to reopen, but GetOrCreateAdditiveContext now returns null, silently skipping user-event or Identity WAF checks. Fresh evidence beyond the prior thread is that AddSpan has no terminal-state check; holding the lock merely orders this disposal before the late AddSpan and therefore does not protect a captured continuation.
Useful? React with 👍 / 👎.
| // this is the point where nothing can use it anymore. Disposing twice is a no-op. | ||
| // This has to happen while holding the lock, or AddSpan could reopen the segment | ||
| // in between and we would dispose the context of a span that is still running. | ||
| _appSecRequestContext?.DisposeAdditiveContext(); |
There was a problem hiding this comment.
When the first WAF check races with the last span closing before _appSecRequestContext has been initialized, this null-conditional call can observe null and do nothing; immediately afterward the AppSecRequestContext getter can install a new object and GetOrCreateAdditiveContext can allocate its native context. With no spans left, there is no subsequent disposal point, so the leak remains. Fresh evidence beyond the resolved creation/disposal thread is that the lazy Interlocked.CompareExchange in the TraceContext.AppSecRequestContext getter occurs outside the _rootSpan lock, so the new per-context _sync cannot serialize this race because the object does not exist yet.
Useful? React with 👍 / 👎.
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.
Summary of changes
The WAF (additive) context is now disposed when the last span of the trace segment closes, not only when a local root span of type
webcloses. Creation and disposal of that context are also serialized so they can't race.Reason for change
Creation and release of the WAF context are asymmetric:
SecurityCoordinator.TryGet, which only requiresCoreHttpContextStore.Instance.Get()to be non-null — it does not check the local root span type. Call sites that reach it without a web root span:EventTrackingSdk.RunSecurityChecksAndReport,EventTrackingSdkV2.RunSecurityChecksAndReport(both carry the comment "Biggest part of this method will only work in a web context" but don't enforce it) andUserManagerCreateIntegrationon the ASP.NET Core Identity signup path.span.IsRootSpan && span.Type == SpanTypes.Web.So a
TrackUserLoginSuccess/SetUser/Identity-signup call made under a non-web local root span (background worker, queue consumer,Task.Runcontinuation…) created a WAF context that nothing ever disposed. Each libddwaf context owns amonotonic_buffer_resource(src/context.hpp) that never reuses freed memory and releases it all only on context destruction, plus the managedContext._encodeResultslist which is only drained inDispose. That memory is native and invisible to the GC, so it showed up as RSS growth with a flat managed heap, recovered only if and when~Context()happened to run.Implementation details
TraceContext.CloseSpandisposes the context in the_openSpans == 0branch, i.e. when the trace segment is complete. Waiting for the last span rather than for the local root span matters because a child span or a captured continuation can outlive the root on a non-web trace, and it would otherwise lose WAF coverage.Context.Disposeis idempotent, so the second call is a no-op.AppSecRequestContext.GetOrCreateAdditiveContextandDisposeAdditiveContextnow run under the existing_synclock: without it, a context being created concurrently with the disposal could be assigned just after the only disposal point and stay orphaned until finalization — exactly the leak this PR is about. No new lock ordering,_syncis released before the WAF is run and nothing underContext.Dispose/Context.Runcalls back intoAppSecRequestContext.Test coverage
Two new tests in
AppSecContextTests, both verified to fail without the corresponding change:GivenAWafContext_WhenTheLocalRootSpanCloses_ThenItIsDisposed— theory over span typesnull,webandcustom: creates a real WAF additive context, closes the span, asserts the context is gone. Thenull/customcases fail onmaster.GivenAWafContextOnANonWebSpan_WhenTheRootClosesBeforeItsChildren_ThenItSurvivesUntilTheyClose— the context stays usable while a child span is still open and is disposed when it closes.Also ran, on net8.0: the full
Datadog.Trace.Security.Unit.Testssuite (1047 passed) and theTraceContext/AppSec/PartialFlush/Tracertests ofDatadog.Trace.Tests(786 passed).Other details
Found while investigating native memory growth with AppSec enabled (SCRS-2370). #8989 removed one route into this (stale
HttpContextsurviving in a capturedExecutionContext), but not the asymmetry itself, which is still reachable onmasterthrough the call sites listed above.A complementary hardening would be to also refuse to create the context outside a web root span; that changes detection behaviour, so it is deliberately left out of this PR.