Fix uncaught StackOverflowError in StackTraces.getStackTrace - #12233
Fix uncaught StackOverflowError in StackTraces.getStackTrace#12233dougqh wants to merge 3 commits into
Conversation
printStackTrace() can itself throw a second StackOverflowError when formatting a throwable caught with little remaining stack margin. The existing fallback only caught Exception, so the second StackOverflowError (an Error) escaped uncaught. Widen the catches to Throwable so it falls through to the manual getStackTrace()-based reconstruction instead. APMLP-1767 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Widen safeGetMessage's catch to Throwable, and try t.getMessage() in the innermost fallback of StackTraces.getStackTrace before giving up on just the class name, so a bit more diagnostic context survives even when both printStackTrace() and getStackTrace() throw. APMLP-1767 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dd-octo-sts
Bot
added
the
tag: ai generated
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![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: 4cf9c0427c
ℹ️ 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".
| try { | ||
| return t.getMessage(); | ||
| } catch (Exception e) { | ||
| } catch (Throwable e) { |
There was a problem hiding this comment.
If ThreadDeath is delivered while safeGetMessage is executing—or an overridden getMessage() throws it—this broad catch converts the termination signal into a diagnostic string and allows the thread to continue. The new catch (Throwable) clauses in getStackTrace have the same problem for ThreadDeath and unrelated fatal VM errors. Since the intended recovery is specifically for stack exhaustion while retaining the existing handling of ordinary exceptions, catch Exception | StackOverflowError instead.
Useful? React with 👍 / 👎.
| trace = sw.toString(); | ||
| } catch (Exception ignored) { | ||
| // printStackTrace() failed (e.g. getMessage() throws inside toString()). | ||
| } catch (Throwable ignored) { |
There was a problem hiding this comment.
When the application catches the original StackOverflowError without first unwinding its recursive frames—the production scenario described by this change—this catch can recover from printStackTrace, but the method then calls truncate outside the fallback and its catch handles only Exception. With the stack still constrained, even trace.length() in truncate can raise another StackOverflowError; reproducing this with a recursive caller and a constrained -Xss causes getStackTrace to exit with that second error despite these new catches. Extend the specific StackOverflowError recovery through the truncation step and return the reconstructed trace if truncation overflows.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
A recovered long stack trace still enters a final truncation block that catches only Exception, allowing another StackOverflowError to escape under the same low-stack condition this change targets.
🤖 Datadog Autotest · Commit 4cf9c04 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| trace = sw.toString(); | ||
| } catch (Exception ignored) { | ||
| // printStackTrace() failed (e.g. getMessage() throws inside toString()). | ||
| } catch (Throwable ignored) { |
There was a problem hiding this comment.
Error-reporting instrumentation can still propagate StackOverflowError into application code for configured or CI Visibility stack-trace limits.
Assertion details
- Input: A throwable whose
printStackTraceoverflows, whose manual fallback produces a trace longer than the finitemaxCharslimit, and whose low remaining stack causestruncateto overflow. - Expected:
Stack-trace capture should return the recovered trace rather than propagate another StackOverflowError from final formatting. - Actual: After recovering from the initial StackOverflowError,
getStackTracecallstruncate; if truncation also throws StackOverflowError, itscatch (Exception)does not intercept it and the error escapes.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
🟢 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. |
catch (Throwable) also swallowed ThreadDeath and unrelated fatal Errors; narrow to the two cases this fix actually needs to recover from. Also extend the same recovery to the truncate() call so a second StackOverflowError while the stack is still constrained during truncation doesn't escape uncaught. Addresses codex review comments on #12233.
2 participants
What Does This Do
Widens exception handling in StackTraces.getStackTrace to catch Error
Motivation
Intended to improve the case where a StackOverflowError is being reported
In that situation, StackTraces.getStackTrace is likely to trigger a fresh StackOverflowError
Additional Notes
StackTraces.getStackTracecaughtprintStackTrace()failures withcatch (Exception ...), so a secondStackOverflowErrorthrown while formatting an already stack-constrained throwable escaped uncaught instead of falling back to the manualgetStackTrace()-based reconstruction.Throwableso anyError(not justException) is handled the same way.03773be6-9b34-11f1-930e-da7ad0900002(serviceci360-engage-optimize): aStackOverflowErrorin the customer's app was caught by our instrumentation with almost no stack margin left, and capturing its trace triggered a second overflow that propagated out of our error-handling code.Jira: APMLP-1767
Test plan
TestThrowables.throwingStackOverflowOnPrintStackTrace()and a unit test assertinggetStackTracefalls back gracefully instead of throwing./gradlew :dd-trace-core:test --tests "datadog.trace.core.util.StackTracesTest"passes./gradlew :dd-trace-core:spotlessApply🤖 Generated with Claude Code