Guard rethrowIfBlockingException call against unexpected failures - #12240
Guard rethrowIfBlockingException call against unexpected failures#12240dougqh wants to merge 4 commits into
Conversation
BlockingExceptionHandler.rethrowIfBlockingException was invoked outside the try/catch that guards the rest of the instrumentation exception handler, so any unexpected Throwable from that call itself (e.g. a NoClassDefFoundError from a classloader that can't see the appsec module) would escape uncaught, replacing the original swallowed exception and crashing the instrumented method's caller instead of being logged. Move the call inside the try/catch and only rethrow when the caught exception is actually a BlockingException. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🎯 Code Coverage (details) 🔗 Commit SHA: 2261d3f | Docs | View more details | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 209973c00a
ℹ️ 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".
There was a problem hiding this comment.
The new catch handler loads BlockingException after it catches a linkage failure. An isolated class loader can throw another NoClassDefFoundError from this check, so the instrumentation error can still reach application code.
🤖 Datadog Autotest · Commit 209973c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
…ception handler Documents the gap flagged by codex on PR #12240: the eatException catch block's own `instanceof BlockingException` check resolves the class via the instrumented class's own classloader, so a classloader that can't see the appsec module gets a NoClassDefFoundError instead of the original exception. Left failing intentionally; not pushed until a fix is decided. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the INSTANCEOF check with a class-name comparison so the catch handler no longer forces resolution of BlockingException via the instrumented class's own classloader - the same NoClassDefFoundError risk the try/catch exists to guard against. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2261d3f847
ℹ️ 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".
| false); | ||
| mv.visitMethodInsn( | ||
| Opcodes.INVOKEVIRTUAL, "java/lang/Class", "getName", "()Ljava/lang/String;", false); | ||
| mv.visitLdcInsn("datadog.appsec.api.blocking.BlockingException"); |
There was a problem hiding this comment.
Preserve blocking for BlockingException subclasses
When advice throws a subclass of the public, non-final BlockingException, BlockingExceptionHandler.rethrowIfBlockingException recognizes it via instanceof and throws the same object, but this exact runtime-name comparison returns false and the subsequent POP swallows it. The instrumented operation then continues instead of enforcing the AppSec block; preserve the helper's subtype semantics without resolving BlockingException through the instrumented classloader, for example by inspecting superclass names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The new exact class-name check swallows a BlockingException subclass. The existing helper rethrows this subclass.
🤖 Datadog Autotest · Commit 2261d3f · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| false); | ||
| mv.visitMethodInsn( | ||
| Opcodes.INVOKEVIRTUAL, "java/lang/Class", "getName", "()Ljava/lang/String;", false); | ||
| mv.visitLdcInsn("datadog.appsec.api.blocking.BlockingException"); |
There was a problem hiding this comment.
Preserve BlockingException subclass propagation
An AppSec or RASP block that uses a BlockingException subclass can fail, so the request can continue.
Assertion details
- Input: Enabled AppSec instrumentation advice throws a subclass of the public, non-final BlockingException class.
- Expected:
The handler must rethrow BlockingException and its subclasses, as rethrowIfBlockingException does with an instanceof check. - Actual:
The catch handler compares the thrown object's exact class name with BlockingException. It discards a subclass because its class name differs.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
What Does This Do
Moves the
BlockingExceptionHandler.rethrowIfBlockingException(t)call generated byExceptionHandlersinside the existing try/catch that guards the rest of the instrumentation exception handler, and has the catch handlerinstanceof-check the caughtThrowablebefore rethrowing — only a genuineBlockingExceptionpropagates; everything else is swallowed like any other instrumentation error.Motivation
rethrowIfBlockingException(added in #7516 soBlockingExceptioncould escape the swallow-and-log) was previously called outside the try/catch. That meant anyThrowablefrom that call — not just a genuineBlockingException— would escape uncaught, replacing the original swallowed exception and potentially crashing the instrumented method's caller (e.g. aNoClassDefFoundErrorfrom a classloader that can't see the appsec module).Found while investigating a top-volume error-tracking issue whose "Failed to handle exception in instrumentation for ..." log samples traced back to this handler.
Additional Notes
None.
Test plan
:dd-java-agent:agent-tooling:forkedTest—ExceptionHandlerForkedTest,ExceptionHandlerExitOnFailureForkedTest,AppSecDisabledExceptionHandlerForkedTest(covers the "blocking exception is rethrown" case) all pass./gradlew spotlessApply— no additional changes🤖 Generated with Claude Code