fix(asm): stop reporting pass-through instrumentation frames as Datadog crashes - #20072
Draft
avara1986 wants to merge 1 commit into
Draft
fix(asm): stop reporting pass-through instrumentation frames as Datadog crashes#20072avara1986 wants to merge 1 commit into
avara1986 wants to merge 1 commit into
Conversation
…og crashes [APPSEC-69877]
Every unhandled exception is shipped to crash tracking with its full traceback,
and the intake tags the report crash_datadog:true as soon as a ddtrace path
appears among the frames. The RASP hooks wrap builtins.open, os.system,
pathlib.Path.open, stripe and the IAST sinks with wrapt, so our wrapper frame
sits in the traceback of ordinary application errors and we get blamed for the
customer's bug. TelemetryWriter._format_stack_trace leaks the same frames into
telemetry error logs.
Filter them where they are reported. What the application itself sees is
unchanged; only our own reports lose the frame.
Registered, not inferred. A frame is dropped only when its code object was
registered as a wrapper that exists to forward a call, so an exception that
genuinely originates in the library keeps its attribution. Registration is
automatic: try_wrap_function_wrapper is the single funnel for every appsec
wrapt wrapper, so one call there covers filesystem, stripe and IAST.
The positional rule that suggests itself - "if the deepest frame is ours, we
raised it" - does not work, and the case it fails on is the most common one:
builtins.open on a missing file -> FileNotFoundError
DDTRACE patch.py:43 wrapped_builtin_open | return original(*args, **kwargs)
deepest frame is ddtrace: True
builtins.open is a C callable and owns no frame, so the wrapper is the deepest
frame even though it only forwarded. Dropping every ddtrace frame instead would
discard genuine faults, including background-thread crashes, which is precisely
what crash tracking is for.
So a registered wrapper that is the deepest frame is resolved by looking at the
instruction it stopped on. A Python callee would own the deepest frame itself,
so a frame that is both deepest and stopped on a CALL was forwarding to a C
callable. Measured identical on 3.9, 3.11, 3.12 and 3.14:
pass-through to a C callable out_of_call=True dropped
our own explicit raise out_of_call=False kept
our own implicit raise (KeyError) out_of_call=False kept
our own bad call to a C callable out_of_call=True dropped
The last row is the known gap: a bug in a wrapper that raises through a C call
is misattributed. It is narrower than the alternatives, which lose rows 2 and 3
as well.
Coverage is appsec-only for now. Contrib integrations still leave frames;
registering at the trace_utils and internal wrapping funnels is a follow-up,
since it changes behaviour for every integration.
internal 833 passed; telemetry, appsec::appsec and crashtracker green on
py3.13. No-oping the registration fails the appsec tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codeowners resolved asResolved from the full PR diff against |
Circular import analysis
|
Dependency direction analysis
|
Contributor
|
BenchmarksBenchmark execution time: 2026-09-04 11:57:10 Comparing candidate commit 2ac9213 in PR branch Found 0 performance improvements and 9 performance regressions! Performance is the same for 572 metrics, 10 unstable metrics, 3 known flaky benchmarks, 15 flaky benchmarks without significant changes.
|
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
Track A of APPSEC-69877. Mmigrating the pure-Python RASP hooks to
WrappingContext, APPSEC-69878 is #19989 and is not a substitute:builtins.open,os.system,os.fork,builtins.eval, stripe and the IAST hashlib sinks are C callables with no__code__, so they cannot be bytecode-wrapped and keep leaving frames.Every unhandled exception is shipped to crash tracking with its full traceback, and the intake tags the report
crash_datadog:trueas soon as a ddtrace path appears among the frames. Because the RASP hooks wrap those targets with wrapt, our wrapper frame sits in the traceback of ordinary application errors and we get blamed for the customer's bug (dashboard).TelemetryWriter._format_stack_traceleaks the same frames into telemetry error logs.This filters them at both reporting boundaries. The traceback the application itself sees is unchanged — only our own reports lose the frame.
Design
Registered, not inferred. A frame is dropped only when its code object was registered as a wrapper that exists to forward a call, so an exception that genuinely originates in the library keeps its attribution. Registration is automatic:
try_wrap_function_wrapperis the single funnel for every appsec wrapt wrapper, so one call there covers filesystem, stripe and IAST.Why not the positional rule
The rule that suggests itself — "if the deepest frame is ours, we raised it; otherwise drop our frames" — fails on the most common case:
builtins.openis a C callable and owns no frame, so the wrapper is the deepest frame even though it only forwarded. Dropping every ddtrace frame instead would discard genuine faults, including background-thread crashes — precisely what crash tracking exists for.Resolving the deepest-frame case
A registered wrapper that is the deepest frame is resolved by the instruction it stopped on. A Python callee would own the deepest frame itself, so a frame that is both deepest and stopped on a
CALLwas forwarding to a C callable. Measured identical on 3.9, 3.11, 3.12 and 3.14:out_of_callraiseKeyError)The last row is the known gap: a bug in a wrapper that raises through a C call is misattributed. It is narrower than the alternatives, which lose rows 2 and 3 as well.
Risks
trace_utilsandddtrace.internal.wrappingfunnels is a one-line follow-up each, left out here because it changes behaviour for every integration and wants its own review.ddtrace/internal/telemetry/writer.pyandddtrace/internal/core/crashtracking.pyare not appsec-owned.sys.tracebacklimitthat truncatesextract_tbbut not the raw walk desynchronises the two, so the traceback is reported unfiltered rather than mismatched. Covered by a test.Testing
tests/internal/test_instrumentation_frames.py(8 tests) covers: unregistered wrapper kept; registered wrapper dropped for a Python callee and for a C callee; registered wrapper kept when it raised explicitly or implicitly; empty registry is a no-op; an all-passthrough traceback is reported rather than emptied; a truncated traceback is reported unfiltered.tests/appsec/appsec/test_filesystem.pyadds the end-to-end assertion forbuiltins.openandpathlib.Path.open: the application's traceback still contains the wrapper, the reported frames do not, and the customer's own frame survives.Mutation-checked — no-oping the registration fails the appsec tests.
internal833 passed;telemetry,appsec::appsecandcrashtrackergreen on py3.13.scripts/lint fmt,typing,spellingandsuitespec-checkclean.Checklist
Reviewer Checklist
🤖 Generated with Claude Code