Skip to content

Commit 5f69d81

Browse files
committed
deps: V8: backport 786c1c2d88d4
Original commit message: [stack-traces] Fix overflow in Error.stackTraceLimit trimming When stack traces are captured for uncaught exceptions (enabled via Isolate::SetCaptureStackTraceForUncaughtExceptions, e.g. by the inspector or by Node.js's --trace-uncaught), CaptureAndSetErrorStack reuses the simple stack trace and trims it to Error.stackTraceLimit. Error.stackTraceLimit counts frames, but the raw call site data stores CallSiteInfo::Fields::kCount slots per frame, so the trim multiplied the limit by kCount: once in the uint32_t comparison against the array length and once, as int, to compute the new length. GetStackTraceLimit clamps the limit to [0, INT_MAX], so for very large limits the uint32_t product can wrap to a value below the array length. The trim branch is then taken although the limit exceeds the number of captured frames, and the int multiplication of the new length overflows. On main (kCount == 5) the product first wraps at 858993460. That limit trimmed the raw data to 4 slots (no complete frame) and 858993461 to 9 slots (one frame), so error.stack silently lost frames. Infinity, the value from the Node.js report, is clamped to INT_MAX; its wrapped product (2147483643) is not below the array length, so on main it does not take the trim branch and does not reach the signed overflow. Fix this by comparing the limit with the number of frames in the raw data (length / kCount), and only multiplying once the limit is known to be smaller than the frame count. The resulting length is then bounded by the existing array length and cannot overflow. Behavior for limits that did not overflow is unchanged, since the raw data length is always a multiple of kCount. This regressed with https://crrev.com/c/7673818 (ebd15783b7b, "[objects]: Defer CallSiteInfo creation"), which switched from one CallSiteInfo per frame to kCount raw slots per frame. This is the underlying cause of Node.js issue 66074. The symptom there differs from main: Node's V8 14.6 backport of that change has kCount == 6 and uses int for the comparison and for RightTrim, so the product overflows for limits above 357913941. For many of those, including Infinity (INT_MAX * 6 wraps to -6), the result is negative and fails "Check failed: new_capacity > 0." in RightTrim. Comparing in frames avoids the overflow in both cases. The new cctest CaptureStackTraceForUncaughtExceptionHugeStackTraceLimit enables capture for uncaught exceptions and checks that limits of 858993460, 858993461 and Infinity yield the same error.stack as a limit of 10, and that a limit of 1 still trims to a single frame. 858993460 and 858993461 are the first limits whose product with kCount wraps around uint32_t; both fail without this change. The new test and the existing stack trace tests also pass in a UBSan build, with no diagnostics. Bug: 565047704 Refs: #66074 Change-Id: I3422ca1de6a7dd9448c7fd53fb9bc5e40e2a17c1 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8426465 Reviewed-by: Patrick Thier <pthier@chromium.org> Reviewed-by: Leszek Swirski <leszeks@chromium.org> Auto-Submit: eliau elkouby (‫אליהו אלקובי‬‎) <eliau.elkouby@gmail.com> Commit-Queue: Patrick Thier <pthier@chromium.org> Cr-Commit-Position: refs/heads/main@{#110043} Refs: v8/v8@786c1c2 Fixes: #66074 Assisted-by: a closed-source coding agent Signed-off-by: Eliau Elkouby <145869377+eliau2005@users.noreply.github.com>
1 parent 342bf6d commit 5f69d81

4 files changed

Lines changed: 37 additions & 5 deletions

File tree

‎common.gypi‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@
4343

4444
# Reset this number to 0 on major V8 upgrades.
4545
# Increment by one for each non-official patch applied to deps/v8.
46-
'v8_embedder_string': '-node.34',
46+
'v8_embedder_string': '-node.35',
4747

4848
##### V8 defaults for Node.js #####
4949

‎deps/v8/AUTHORS‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ Douglas Crosher <dtc-v8@scieneer.com>
124124
Dusan Milosavljevic <dusan.m.milosavljevic@gmail.com>
125125
Eden Wang <nedenwang@tencent.com>
126126
Edoardo Marangoni <edoardo@wasmer.io>
127+
Eliau Elkouby <eliau.elkouby@gmail.com>
127128
Elisha Hollander <just4now666666@gmail.com>
128129
Eric Rannaud <eric.rannaud@gmail.com>
129130
Erich Ocean <erich.ocean@me.com>

‎deps/v8/src/execution/isolate.cc‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1645,12 +1645,15 @@ MaybeDirectHandle<JSObject> Isolate::CaptureAndSetErrorStack(
16451645
static_cast<uint32_t>(
16461646
stack_trace_for_uncaught_exceptions_frame_limit_));
16471647
DCHECK_GE(stack_trace_limit, 0);
1648-
if (static_cast<int>(stack_trace_limit) *
1649-
CallSiteInfo::Fields::kCount <
1650-
raw_data_for_call_site_infos->length()) {
1648+
// Compare in frames rather than raw slots to avoid overflowing for
1649+
// large Error.stackTraceLimit values.
1650+
uint32_t frame_count = raw_data_for_call_site_infos->ulength() /
1651+
CallSiteInfo::Fields::kCount;
1652+
if (static_cast<uint32_t>(stack_trace_limit) < frame_count) {
16511653
call_site_infos_or_formatted_stack = FixedArray::RightTrimOrEmpty(
16521654
this, raw_data_for_call_site_infos,
1653-
stack_trace_limit * CallSiteInfo::Fields::kCount);
1655+
static_cast<uint32_t>(stack_trace_limit) *
1656+
CallSiteInfo::Fields::kCount);
16541657
}
16551658
// Notify the debugger.
16561659
OnStackTraceCaptured(stack_trace);

‎deps/v8/test/cctest/test-api-stack-traces.cc‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,34 @@ TEST(CaptureStackTraceForUncaughtException) {
438438
CHECK_EQ(1, report_count);
439439
}
440440

441+
TEST(CaptureStackTraceForUncaughtExceptionHugeStackTraceLimit) {
442+
LocalContext env;
443+
v8::Isolate* isolate = env.isolate();
444+
v8::HandleScope scope(isolate);
445+
isolate->SetCaptureStackTraceForUncaughtExceptions(true);
446+
447+
CompileRun(
448+
"function foo() { return new Error().stack; }\n"
449+
"function bar() { return foo(); }\n"
450+
"function stackWithLimit(limit) {\n"
451+
" Error.stackTraceLimit = limit;\n"
452+
" return bar();\n"
453+
"}\n");
454+
Local<Value> expected = CompileRun("stackWithLimit(10)");
455+
CHECK(expected->IsString());
456+
457+
// For these limits, limit * CallSiteInfo::Fields::kCount overflows.
458+
for (const char* limit : {"858993460", "858993461", "Infinity"}) {
459+
std::string source = std::string("stackWithLimit(") + limit + ")";
460+
CHECK(CompileRun(source.c_str())->StrictEquals(expected));
461+
}
462+
463+
// Small limits must still trim the stack trace.
464+
CHECK(CompileRun("stackWithLimit(1).split('\\n').length === 2")->IsTrue());
465+
466+
isolate->SetCaptureStackTraceForUncaughtExceptions(false);
467+
}
468+
441469
// Test uncaught exception in a setter
442470
const char uncaught_setter_exception_source[] =
443471
"var setters = ['column', 'lineNumber', 'scriptName',\n"

0 commit comments

Comments
 (0)