Conversation
|
Review requested:
|
nodejs-github-bot
added
build
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
There was a problem hiding this comment.
We run the V8 test suite, so do we gain anything from adding this here now that the test exists in V8?
There was a problem hiding this comment.
The V8 cctest verifies the upstream fix, but it only runs in node-test-commit-v8-linux, which maintaining-V8.md describes as a job to run in addition to the regular Node.js CI. The test/parallel test runs in every normal CI job.
The coverage is also not identical: this is a backport to V8 14.6, where kCount is 6, so the overflow boundary differs from current V8. The Node test covers the relevant boundary values and the original --trace-uncaught reproduction against Node's embedded V8.
The main reason I'd keep it is #65161: the V8 15.2 update replaces deps/v8 wholesale and does not contain this fix, so the V8 cctest would disappear along with the floating patch. The Node test provides a regression guard at the Node level against that.
That said, if you'd rather avoid duplicating the coverage, I'm happy to remove it.
There was a problem hiding this comment.
Is that you talking or an LLM? The V8 updates re-float the patches on top of the update, so the patch would stay (can you imagine if we had to start from scratch at every V8 version bump?) and test with it. Unless there's a possibility of a regression coming from something other than V8, the V8 test should suffice, and running it on each PR would be a waste of our resources
There was a problem hiding this comment.
That's me โ I use a coding agent, hence the Assisted-by trailer on the commits. Makes sense though, I hadn't considered the refloat process. I'll drop the Node test.
45532fc to
5f69d81
Compare
| # Reset this number to 0 on major V8 upgrades. | ||
| # Increment by one for each non-official patch applied to deps/v8. | ||
| 'v8_embedder_string': '-node.34', | ||
| 'v8_embedder_string': '-node.35', |
There was a problem hiding this comment.
| 'v8_embedder_string': '-node.35', | |
| 'v8_embedder_string': '-node.36', |
There was a problem hiding this comment.
Applied, thanks โ .35 had been taken on main since I opened this.
6ef464a to
a1fb72f
Compare
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: nodejs#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: nodejs#66074
Assisted-by: a closed-source coding agent
Signed-off-by: Eliau Elkouby <145869377+eliau2005@users.noreply.github.com>
Signed-off-by: Eliau Elkouby <145869377+eliau2005@users.noreply.github.com>
a1fb72f to
4e2b6bd
Compare
4 participants
Fixes: #66074
This floats the upstream V8 fix 786c1c2d88 (CL 8426465, "[stack-traces] Fix overflow in Error.stackTraceLimit trimming"). I wrote that CL. As requested in the issue, this targets
mainfirst.The bug
The regression comes from #65764 (
deps: V8: backport ebd15783b7ba). When stack traces are captured for uncaught exceptions (--trace-uncaught, or an attached inspector),Isolate::CaptureAndSetErrorStacktrims the raw call-site data withstack_trace_limit * CallSiteInfo::Fields::kCount. In our V8 14.6 treekCountis 6 and the arithmetic is signedint, so large limits overflow.Infinityis clamped toINT_MAX, andINT_MAX * 6wraps to-6. That failsCHECK_GT(new_capacity, 0)inRightTrim:It reproduces on
v27.0.0-nightly202609233d85c94cfa(linux-x64). It does not reproduce onv27.0.0-nightly2026090729667e046b, which was built before #65764 landed.Measured with an Error about 59 frames deep under
--trace-uncaught, before the fix:Error.stackTraceLimit1e10andInfinityIn general, a limit crashes when
int32(limit * 6)is negative, and it silently truncates when the result falls in[0, raw length). The frame-loss windows are as wide as the captured stack. Without--trace-uncaught/ inspector, nothing goes wrong.The fix
Compare the limit against the number of frames (
ulength() / kCount) and multiply only after the limit is known to be smaller. The upstream commit didn't apply cleanly to 14.6 because V8 main'sulength()returns a wrapper (.value()), while 14.6 returns a plainuint32_t. The resolved hunk is identical to upstream except that.value()is dropped.v8_embedder_stringis bumped to-node.35.The CL's cctest (
CaptureStackTraceForUncaughtExceptionHugeStackTraceLimit) is included in the backport. WithkCount == 6, itsInfinitycase is the one that exercises the bug in this tree.Interaction with the V8 15.2 update (#65161)
main@{#110043}, during V8 15.6 development. The 15.2 through 15.5 branches were cut before it, and none of them has a merge-back. So deps: update V8 to 15.2ย #65161 (V8 15.2.124.34) does not contain the fix. Itsisolate.ccstill has the unfixed multiplication. There the symptom would change from a crash to silent frame loss, starting at 858993460.deps/v8in deps: update V8 to 15.2ย #65161. After 15.2 it can be a plaindeps: V8: cherry-pick 786c1c2d88d4, and it would be good to include that in deps: update V8 to 15.2ย #65161. If this PR lands first, the patch would be refloated on top of the 15.2 update, where it applies cleanly.v26.x-staging, which has the sameisolate.ccand embedder string-node.34.Testing
On linux-x64 with a local release build of this branch:
2**32,1e10andInfinity. Small limits still trim: 0 โ 0 frames, 1 โ 1, 10 โ 10.make -j8 test-only:make tooltest: 15/15 passed.parallel/test-fs-rmSync-special-char-additional-error(ENOENT โฆ chmod 'โฆ/้_dir'). It is not related to this change: it fails the same way on the unpatched nightliesv27.0.0-nightly2026090729667e046bandv27.0.0-nightly202609233d85c94cfaon the same machine, which runs tests as root.make test-v8.CI
Please start both a regular Node.js CI run and a V8 CI run (
node-test-commit-v8-linux). Only V8 CI builds and runs the added V8 cctest.AI assistance
Parts of this work were done with a closed-source coding agent, as disclosed by the
Assisted-bytrailer on the commit. I wrote the original V8 CL. I reviewed the full diff and the resolvedisolate.cchunk myself, and verified the reproduction, the boundary values and the test results on this machine./cc @silverwind (reporter) @richardlau (asked for the float) @nodejs/v8-update