[AppSec] Free the WAF diagnostics of every config applied to the builder - #9056
Open
dromanol wants to merge 1 commit into
Open
[AppSec] Free the WAF diagnostics of every config applied to the builder#9056dromanol wants to merge 1 commit into
dromanol wants to merge 1 commit into
Conversation
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
BenchmarksBenchmark execution time: 2026-08-14 15:16:11 Comparing candidate commit 77e6fcc in PR branch Found 1 performance improvements and 1 performance regressions! Performance is the same for 70 metrics, 0 unstable metrics, 67 known flaky benchmarks, 59 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9056) and master.
|
|||||||||||||||||||||||||
| Metric | Master (Mean ± 95% CI) | Current (Mean ± 95% CI) | Change | Status |
|---|---|---|---|---|
| .NET Framework 4.8 - Baseline | ||||
| duration | 190.62 ± (190.48 - 191.31) ms | 207.66 ± (207.07 - 207.93) ms | +8.9% | ❌⬆️ |
| .NET Framework 4.8 - Bailout | ||||
| duration | 194.65 ± (194.57 - 195.15) ms | 211.50 ± (211.11 - 211.93) ms | +8.7% | ❌⬆️ |
ddwaf_builder_add_or_update_config allocates a fresh diagnostics object on every call and overwrites the one it is given without freeing it, so reusing a single object across the loop leaked all but the last one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Remove comments on WAF diagnostics object handling Removed comments about WAF diagnostics object allocation and reuse.
dromanol
force-pushed
the
dani/waf-diagnostics-leak-per-config
branch
from
August 14, 2026 14:22
656cebf to
77e6fcc
Compare
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
WafConfigurator.Updatenow uses a freshDdwafObjectStructfor eachddwaf_builder_add_or_update_configcall and releases the previous one, instead of passing the same object to every call in the loop.Reason for change
ddwaf_builder_add_or_update_configallocates a brand new diagnostics tree on every call and overwrites the object it is given without freeing it. libddwaf is explicit about it (src/interface.cpp):We passed a single
diagnosticsthrough the wholeconfigs.Updatesloop and the caller (Waf.Create/Waf.Update) destroyed it once, so every config but the last leaked its diagnostics tree. That is native memory invisible to the GC, leaked on each WAF init and on every RCM update cycle that carries more than one config.The same file already got this right for the obfuscator config (
ApplyObfuscatorConfig, with the comment "diagnostics are always allocated by the WAF with the default allocator"); this brings the ruleset loop in line.Implementation details
Per iteration: pass a local
configDiagnostics, thenObjectDestroythe accumulateddiagnosticsand take over the new one. Reported diagnostics semantics are unchanged — the last applied config still wins, which is what the WAF's overwrite already gave us — soUpdateResult/telemetry output is identical.Test coverage
Covered indirectly by the existing WAF init/update tests (
Datadog.Trace.Security.Unit.Tests), which assert the reported diagnostics after applying multiple configs; the behaviour they check is unchanged. The leak itself is native and not observable from a managed unit test.Other details
Found while investigating native memory growth with AppSec enabled (SCRS-2370). This is a real but small leak (tens of KB per update cycle) and is not the main suspect for that escalation — it is filed separately because it is an independent bug.