fix(internal): release WrappingContext storage when a context raises - #20016
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 5 commits intoSep 3, 2026
Conversation
…[APPSEC-69960]
A wrapping context that raises is skipped by the machinery that would have
popped its per-call ContextVar storage, so the storage chains one dict per
call through _STORAGE_PREV and is never released. On a long-lived worker
thread it grows without bound, and on two of the three paths the universal
storage is among the retained dicts, so it keeps hold of __frame__.
Three paths, measured on 3.13:
__enter__ concrete leaks, chain 1 -> 8 over 8 calls; universal is fine,
because the injected exception handler still reaches _exit()
__return__ both leak, chain 1 -> 5 over 5 calls, universal keeps __frame__
__exit__ same as __return__
On __enter__ the cause is that `entered` is appended to only after a
successful call, so a context that raises is absent from the list __exit__
later iterates. On __return__ the universal context sets _SKIP_EXIT_KEY and
re-raises without reaching super().__return__, and the resulting __exit__ is
suppressed on purpose, so nothing pops anything.
Release the storage on all three paths, comparing storage identity against a
snapshot taken before the call. That identity check is what makes it safe:
popping unconditionally would discard an outer re-entrant call's storage.
Existing semantics are unchanged. An Exception from __enter__ is still
swallowed and logged so a broken context cannot break the call, and a
BaseException still propagates to the caller.
Latent today, since no shipped context raises by design; it needs a bug in a
context's __enter__/__return__/__exit__ to trigger, which an Exception-derived
failure does even though it is swallowed. It becomes reachable by design in
APPSEC-69877, where BlockingException is raised from __enter__ to stop a
blocked outgoing request.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codeowners resolved asResolved from the full PR diff against No remaining files require a CODEOWNERS review. |
Dependency direction analysis
|
Circular import analysis
|
Contributor
🎉 All green!🧪 All tests passed 🔗 Commit SHA: 404121a | Docs | View more details | Give us feedback! |
BenchmarksBenchmark execution time: 2026-09-03 16:49:09 Comparing candidate commit 404121a in PR branch Found 0 performance improvements and 6 performance regressions! Performance is the same for 575 metrics, 10 unstable metrics, 2 known flaky benchmarks, 16 flaky benchmarks without significant changes.
|
Below 3.11 the wrapped function enters through a real `with` statement, and Python does not call __exit__ when __enter__ raises. So on those versions the universal context's own storage, and that of the contexts that did enter, are also left behind when a context raises out of __enter__ - not just the storage of the context that raised. From 3.11 the injected exception handler reaches _exit(), which already released them, which is why this only showed up on py3.9 in CI: assert _storage_chain_length(universal._storage.get()) == 0 AssertionError: assert 5 == 0 Release both from the raising path when the version needs it. Gating on the version keeps 3.11+ untouched, where popping here would be wrong: _exit() still runs, and a double pop would discard an outer re-entrant call's storage. tests/internal/test_wrapping.py now passes on 3.9 through 3.14, 59 tests each. Setting the gate to False reproduces the CI failure on 3.9, so the existing test does cover this path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
avara1986
marked this pull request as ready for review
September 2, 2026 12:37
avara1986
changed the title
![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: 6557190ecb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…EC-69960]
Codex review caught that releasing the storage inside __return__ is wrong, and
it is: _exit (bytecode path) and on_py_unwind (monitoring path) read
_SKIP_EXIT_KEY back off that same storage. Popping it first makes them read the
enclosing call's storage instead, so with a re-entrant wrapped call they exit
the outer call's contexts and pop its storage while its body is still running.
Non-re-entrant it is harmless, since __exit__ bails on a missing __contexts__,
but the re-entrant case is real corruption.
Revert __return__ and __exit__ to their original shape and keep only the
__enter__ fix, which is what APPSEC-69877 needs and what is verifiable here.
The remaining leaks are documented in place and tracked in APPSEC-69961:
- __return__ / __exit__ raising still leak, and fixing that means releasing
where the flag is consumed rather than before, with different handling below
3.11 where the with statement's __exit__ already cleans up.
- The 3.15 monitoring path has the same shape as the pre-3.11 with path, since
on_py_unwind returns early on the flag. Not fixed here because 3.15 is
outside requires-python and absent from CI, so it cannot be verified.
The two test parameters for those paths are now xfail(strict=True) against that
ticket, so they flip to a failure as soon as the leak is fixed.
This also removes the only cost the PR added to the happy path: __return__ and
__exit__ no longer build a {id: storage} snapshot on every call.
Matrix: tests/internal/test_wrapping.py 57 passed, 2 xfailed on 3.9 through
3.14. tests/wrapping/ 276 passed on 3.9 and 322 on 3.14.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
emmettbutler
approved these changes
Sep 2, 2026
emmettbutler
left a comment
emmettbutler
left a comment
Collaborator
There was a problem hiding this comment.
This is a crazy find, I wonder if it will solve a lot of strange memory leaks we've observed.
P403n1x87
approved these changes
Sep 2, 2026
gh-worker-dd-mergequeue-cf854d
Bot
merged commit Sep 3, 2026
6eff716
into
main
1535 of 1537 checks passed
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
alberto.vara/APPSEC-69960-wrapping-context-storage-leak
branch
September 3, 2026 19:54
avara1986
added a commit
that referenced
this pull request
Sep 4, 2026
…EC-69877] Review feedback. try_wrap_context's module hook returned early whenever (module_name, name) was already in the registry, treating the key as proof that the current target is wrapped. It is not: when http.client or urllib.request is reloaded, or dropped from sys.modules and imported again, ModuleWatchdog fires the hook for freshly defined functions while the registry still holds a context bound to the previous ones. The new HTTPConnection and OpenerDirector methods then ran without SSRF and API10 instrumentation until a full unpatch/patch cycle. This was a regression against the wrapt path it replaced, which handled the case for free: wrap_object runs on every hook fire, and apply_patch wraps whatever the attribute holds at that point. Compare the installed context's __wrapped__ against the freshly resolved target instead, and rebind when they differ, releasing the stale context first. A failure to release is logged and does not stop the rebind. Also adds the regression test for the blocking-path storage leak that review raised against this PR. The framework fix landed separately in #20016 (APPSEC-69960), but this is the code that raises BlockingException from __enter__, so the assertion belongs here: three blocked requests must leave both the concrete and the universal ContextVar unset, or each block chains another storage dict onto the worker thread. Both are mutation-checked: removing the rebind, or removing the storage release in _UniversalWrappingContext.__enter__, fails the corresponding test. appsec::appsec on py3.13: 708 passed, 1 xfailed. 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
3 participants
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.
Description
A wrapping context that raises is skipped by the machinery that would have popped its per-call
ContextVar storage. The storage chains one dict per call through
_STORAGE_PREVand is neverreleased, so on a long-lived worker thread it grows without bound, and on two of the three paths
the universal storage is among the retained dicts — so it keeps hold of
__frame__.Measured on Python 3.13, before the fix:
Scope: only the
__enter__path is fixed here.__return__/__exit__and the 3.15monitoring path leak the same way, but releasing the storage there is not a matter of adding a pop —
_exitandon_py_unwindread_SKIP_EXIT_KEYback off that storage, so popping first makes themact on the enclosing call's storage. Documented in place and tracked in APPSEC-69961, with the two test parameters
covering them marked
xfail(strict=True).Two distinct causes, both on
__enter__:__enter__—_UniversalWrappingContext.__enter__appends toenteredonly after asuccessful call, so a context that raises is absent from the list
__exit__later iterates. Its__enter__has usually already pushed storage viasuper().__enter__(). From 3.11 the universalstorage is fine here, because the injected exception handler still reaches
_exit().__enter__below 3.11 — those versions enter through a realwithstatement, and Python doesnot call
__exit__when__enter__raises. So the universal storage, which holds__frame__,and the storage of contexts that did enter are left behind as well. Gated on the version,
because popping here on 3.11+ would double-pop and discard an outer re-entrant call's storage.
The fix releases storage on all three paths, comparing storage identity against a snapshot taken
before the call. That identity check is the load-bearing part: popping unconditionally would
discard an outer re-entrant call's storage, since a context that already popped now holds a
different object.
Existing semantics are deliberately unchanged: an
Exceptionfrom__enter__is still swallowedand logged so a broken context cannot break the call, and a
BaseExceptionstill propagates.Impact
Latent today. No shipped
WrappingContextsubclass raises by design, so triggering it needs a bugin a context's
__enter__/__return__/__exit__— which anException-derived failure does,even though it is swallowed and logged. A debugger probe or code-origin context hitting an
unexpected error would leak.
It becomes reachable by design in #19989, where
BlockingExceptionis raised from__enter__tostop a blocked outgoing request, i.e. one leaked dict per blocked request.
Jira: https://datadoghq.atlassian.net/browse/APPSEC-69960
Blocks: #19989
Risks
Medium — this is shared instrumentation used by the debugger, code origin, selenium and the lazy
module loader.
_release_storagepops only when thecontext still holds the exact object it held before the call, so an already-popped context is
left alone.
except Exceptionin__enter__widened toexcept BaseExceptionin order to run cleanup,then re-raises non-
Exceptionunchanged. Behaviour forExceptionis identical to before.Additional Notes
Split out of #19989 at review request: the change is in shared wrapping infrastructure and wants
its own review rather than riding along in an AppSec PR.
🤖 Generated with Claude Code