node:vm: keep lineOffset/columnOffset from overflowing JSC parser positions - #38228
Conversation
|
Updated 12:18 PM PT - Aug 17th, 2026
✅ @robobun, your commit e9cbc739d4b04d09b230c08ace2eed1882d7c9ac passed in 🧪 To try this PR locally: bunx bun-pr 38228That installs a local version of the PR into your bun-38228 --bun |
|
Status: ready to merge; rebased onto main again after #38660 landed (the only conflict was both branches appending tests to the end of
Reproduced on a debug build of main ( Test: |
|
Warning Review limit reached
Next review available in: 8 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it reorders control flow in constructAnonymousFunction (building the wrapper before the standalone body parse so the clamp can use the wrapper length) and intentionally leaves the native vmModuleRunInNewContext/vmModuleRunInThisContext bindings unclamped on the basis that vm.ts routes those through Script, a human look would still be worthwhile.
What was reviewed:
clampOffsetForSourcearithmetic — computed inint64_twithstd::max(..., 0), soINT_MAX - 1 - sourceLengthcannot wrap for large sources and negative offsets pass through unchanged.- The
constructAnonymousFunctionreorder —stringifyAnonymousFunctiononly callstoWTFStringon args already verified as primitive strings invmModuleCompileFunction, so moving it before the standalone parse is side-effect-free; the addedRETURN_IF_EXCEPTIONreplaces the old continue-with-null-string on OOM. NodeVMSourceTextModule::create—sourceText.length()is read beforeWTF::move(sourceText), andSourceCodenow receives the clamped zero-based values (unchanged for normal offsets).- Confirmed
src/js/node/vm.tsbuildsrunInThisContext/runInNewContextonScript, so the un-clamped native host functions are not on the user-reachable path.
Extended reasoning...
Overview
This PR fixes an int overflow / debug-build abort in node:vm when lineOffset/columnOffset approach INT32_MAX. It adds a shared clampOffsetForSource(offset, sourceLength) helper in NodeVM.{h,cpp} and applies it at the three sites that build a SourceCode from user-supplied offsets: constructScript (vm.Script), constructAnonymousFunction (vm.compileFunction), and NodeVMSourceTextModule::create. The compileFunction path is reordered so the wrapper program is built first and the clamp uses its length (the longest text parsed). Eleven subprocess tests are added covering construction of all three APIs plus reported line/column of runtime and compile-time errors at the int32 edge.
Security risks
None identified. The change is defensive — it narrows the range of an int that is only used as a source-position offset, turning UB / an assertion abort into a saturated-but-valid position. No new user input surface, no allocation sizing derived from the offset.
Level of scrutiny
Medium-high. This is native C++ in the JSC bindings, and constructAnonymousFunction mixes user-input handling with JSC parser/code-cache calls. The clamp itself is small and its arithmetic is done in int64_t, but the reorder in constructAnonymousFunction is a real control-flow change that deserves a maintainer's eye — I traced that the args reaching stringifyAnonymousFunction are all primitive JSStrings (validated in vmModuleCompileFunction), so calling it earlier does not run user code, and the newly added RETURN_IF_EXCEPTION after it is a strict improvement over the old EXCEPTION_ASSERT-only path.
Other factors
- The PR intentionally does not clamp the native
vmModuleRunInNewContext/vmModuleRunInThisContexthost functions inNodeVM.cpp, on the basis thatsrc/js/node/vm.tsimplements those APIs vianew Script(...). I confirmed that invm.ts, but a maintainer may still want those binding functions clamped (or removed) rather than left as reachable-but-unused overflow paths. NodeVMSourceTextModule::createalready had a pre-existing signedness/off-by-one issue with itsuint32_toffsets that the PR notes as a separate issue (#33564); the change here does not make it worse and preserves the zero-based values passed toSourceCodefor normal inputs.- Test coverage is thorough: each case runs in its own subprocess (the failure mode is an abort), uses
test.concurrent.each, and asserts both that construction succeeds and that reported positions stay in(INT32_MAX-100, INT32_MAX]rather than wrapping negative or being dropped.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Re the review note about the native |
|
Heads up: #38235 changes the same lines in NodeVMSourceTextModule.cpp (it builds the module's SourceCode through makeSource so the offsets are converted to one-based positions and the identifier becomes the source URL). Whichever lands second only needs to route the clamped TextPosition into that makeSource call. One knock-on effect for this PR's module test case: once the module path goes through makeSource it has the same overflow profile as vm.Script, so the two-line SourceTextModule case needs lineOffset INT32_MAX - 1 (like the Script case) to abort without the clamp; with exactly INT32_MAX the one-based conversion wraps and SourceCode clamps it, so that input no longer fails before the fix. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reorders the compile path in constructAnonymousFunction and removes two native host functions plus a BaseVMOptions constructor, a human look would still be worthwhile.
What was reviewed:
clampOffsetForSourcearithmetic: widens to int64 before subtraction, clamps to[0, INT32_MAX-1], negative offsets pass through unchanged.vm.tsconfirmed to routerunInNewContext/runInThisContextthroughScript; no remaining references to the removedvmModuleRunIn*symbols or the deletedBaseVMOptions(filename, line, col)constructor.constructAnonymousFunctionreordering:stringifyAnonymousFunctionnow runs first, but its args are already-validated JSStrings so no user-JS re-entry ordering changes; the addedRETURN_IF_EXCEPTIONreplaces continuing with a null program string.NodeVMSourceTextModule::create:sourceText.length()is read beforeWTF::move(sourceText).
Extended reasoning...
Overview
The PR fixes an int32 overflow in node:vm's lineOffset/columnOffset handling that aborts assertion builds and produces UB in release. It adds clampOffsetForSource() in NodeVM.{h,cpp} and applies it at the three sites that build a SourceCode from user-supplied offsets: constructScript (NodeVMScript.cpp), constructAnonymousFunction (NodeVM.cpp), and NodeVMSourceTextModule::create. It also removes the native vmModuleRunInNewContext/vmModuleRunInThisContext host functions (~120 lines) and the BaseVMOptions(filename, lineOffset, columnOffset) constructor, all of which were unreachable since vm.ts builds those APIs on Script. Tests add 11 subprocess cases exercising INT32_MAX offsets across all three APIs.
Security risks
None identified. The clamp only lowers absurdly large offsets; it cannot widen the accepted input space. The removed host functions were dead code (verified: vm.ts routes through Script, and no references to vmModuleRunIn* remain in src/).
Level of scrutiny
Medium-high. This is C++ JSC bindings code with a non-trivial reordering in constructAnonymousFunction (the wrapper is now built before the standalone body parse so the clamp can use the wrapper's length), plus dead-code removal. The clamp itself is small and its arithmetic checks out (int64 widening before subtraction, std::max(..., 0) for huge sources, negative offsets return unchanged since int <= int64_t sign-extends). The reordering is safe because stringifyAnonymousFunction's args are already JSStrings from vmModuleCompileFunction, so no user-JS side-effect ordering changes; the new early RETURN_IF_EXCEPTION after it is an improvement over the old code, which would have passed a null string to StringSourceProvider::create on OOM.
Other factors
The comment-cop bot flagged long comments; the author addressed all three (threads resolved) by shortening to one line each. Test coverage is thorough: subprocess isolation for the abort case, both construction and error-position cases, all three APIs, and asserts positions stay near the requested offset rather than only "doesn't crash". The PR description documents that 9/11 tests fail without the fix and the full vm.test.ts + 95 Node parallel test-vm-*.js files pass with it. Still, C++ bindings changes with code reordering and function removal are outside the "simple/mechanical" bar for auto-approval.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it reorders control flow in constructAnonymousFunction, removes two native binding entry points, and has an acknowledged merge interaction with #38235 on the same NodeVMSourceTextModule.cpp lines, a human look would still be worthwhile.
What was reviewed:
clampOffsetForSourcearithmetic — computed in int64, clamped to 0, negative offsets pass through unchanged; applied at all threeSourceCodeconstruction sites.constructAnonymousFunctionreorder — wrapper is now built before the standalone body parse; args are pre-validated JSStrings sostringifyAnonymousFunctionruns no user code, and the addedRETURN_IF_EXCEPTIONcloses a pre-existing continue-with-null-string gap.NodeVMSourceTextModule::create—sourceText.length()is read beforeWTF::move(sourceText);SourceCodenow receives the clamped position.- Removed
vmModuleRunInNewContext/vmModuleRunInThisContextand the 3-argBaseVMOptionsctor — greppedsrc/andsrc/js/;vm.tsroutes both throughScript, no other references.
Extended reasoning...
Overview
Fixes an int32 overflow in node:vm's lineOffset/columnOffset handling that aborts assertion builds (JSTextPosition::checkConsistency) and produces wrapped negative line numbers on release. Adds a shared clampOffsetForSource(offset, sourceLength) helper capping the zero-based offset at INT_MAX - 1 - sourceLength, applies it in constructScript, constructAnonymousFunction, and NodeVMSourceTextModule::create, and removes the dead native runInNewContext/runInThisContext host functions (plus the BaseVMOptions ctor only they used). 11 new subprocess tests in vm.test.ts cover construction and reported error positions across all three APIs.
Security risks
None identified. The clamp only lowers a user-supplied int32 before it reaches JSC; no new user-controlled data flows are introduced. Removing the dead bindings shrinks surface area.
Level of scrutiny
Moderate-to-high: this is C++ in the JSC bindings layer where exception-check discipline and use-after-move matter. I verified the int64 arithmetic in clampOffsetForSource cannot itself overflow (worst case INT_MAX - 1 - UINT_MAX fits int64, then clamped to 0), that sourceText.length() is read before its move in the module path, and that the reorder in constructAnonymousFunction is side-effect-neutral because every arg reaching it from vmModuleCompileFunction is already a validated JSString (so toWTFString cannot re-enter JS). The added RETURN_IF_EXCEPTION after stringifyAnonymousFunction is a strict improvement over the prior EXCEPTION_ASSERT-only path. Grep across src/ confirms the removed vmModuleRunIn*Context symbols and the 3-arg BaseVMOptions constructor have no remaining references; src/js/node/vm.ts routes both public APIs through Script.
Other factors
The author notes #38235 touches the same NodeVMSourceTextModule.cpp lines and has documented the merge resolution (and adjusted the module test case to fail-without-fix under either landing order). The comment-cop bot's three long-comment flags were addressed in follow-up commits and all threads are resolved. Tests follow harness conventions (subprocess per case since the failure mode is an abort, test.concurrent.each, concurrent pipe drains, stderr-then-exitCode ordering). Deferring rather than approving because a maintainer should confirm the #38235 coordination plan and sign off on removing the two native binding entries.
… overflow on assertion builds
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reorders the wrapper-build in constructAnonymousFunction and removes two native host functions from the node:vm binding, a human look would still be worthwhile.
What was reviewed:
clampOffsetForSourcearithmetic: widened to int64 before subtraction, floored at 0, negative offsets pass through unchanged.- Reorder in
constructAnonymousFunction:stringifyAnonymousFunctiononly touches already-validated JSStrings, so hoisting it above the body-onlycheckSyntaxdoesn't change observable order; the newRETURN_IF_EXCEPTIONcloses an OOM fall-through. - Confirmed
src/js/node/vm.tsbuildsrunInNewContext/runInThisContextonScriptand nothing else references the removed native host functions or the three-argBaseVMOptionsconstructor. NodeVMSourceTextModule:sourceText.length()is read before theWTF::move, and the zero-basedSourceCodeargs are preserved as before.
Extended reasoning...
Overview
Adds clampOffsetForSource(offset, sourceLength) and applies it at the three node:vm sites that hand a user-supplied lineOffset/columnOffset to JSC's SourceCode: constructScript (vm.Script), constructAnonymousFunction (vm.compileFunction), and NodeVMSourceTextModule::create. In constructAnonymousFunction the wrapper program is now built before the offsets are clamped so the clamp bounds against the longest text actually parsed, with a new RETURN_IF_EXCEPTION after the build. Separately removes the native vmModuleRunInNewContext/vmModuleRunInThisContext host functions (and the BaseVMOptions(String, OrdinalNumber, OrdinalNumber) constructor only they used), which have been unreachable since vm.ts moved both APIs onto Script. Adds an 11-case subprocess test block.
Security risks
The input is a validated int32 that previously overflowed inside JSC's lexer position arithmetic — a debug-build abort / release wrap, not a memory-safety issue. The clamp only lowers the offset; it doesn't change what gets parsed or evaluated. No new attack surface.
Level of scrutiny
Medium-high. The clamp helper is small and its arithmetic checks out (int64 widening, floor at 0, negative pass-through). But the PR also (a) reorders code inside constructAnonymousFunction — a function that builds and parses user-supplied source under a ThrowScope — and (b) deletes ~130 lines of native binding code plus its createNodeVMBinding wiring. Both look correct: the hoisted stringifyAnonymousFunction only calls toWTFString on values already known to be JSStrings (the caller validates params and wraps the body in jsString), so no user-observable reordering, and I confirmed src/js/node/vm.ts routes both removed APIs through Script with no remaining references to the deleted symbols. Still, native JSC-binding refactors in this repo warrant a human pass.
Other factors
The comment-cop feedback was addressed (comments trimmed to one line each). Tests are subprocess-based since the failure mode is an abort, use test.concurrent.each, and assert positions land in (INT32_MAX - 100, INT32_MAX] rather than an exact value — reasonable given the clamp is length-dependent. The PR description flags an overlap with #38235 on the same NodeVMSourceTextModule.cpp lines; whichever lands second needs a small merge adjustment there.
202b7f0 to
78595bc
Compare
There was a problem hiding this comment.
I reviewed this and found no bugs. The clamp arithmetic, the constructAnonymousFunction reorder, and the dead-binding removal all check out, but since it touches JSC binding control flow and deletes ~120 lines of native code, a human look would still be worthwhile.
What was reviewed:
clampOffsetForSource: int64 widening before the subtraction,std::max(..., 0)guards a >2GiB source, negative offsets pass through unchanged, and the finalintcast is bounded.constructAnonymousFunctionreorder:stringifyAnonymousFunctionnow runs first — its args are pre-validated JSStrings so no user-observable coercion moved; the addedRETURN_IF_EXCEPTIONalso fixes the pre-existing continue-with-null-string on OOM.- Confirmed
src/js/node/vm.tsbuildsrunInNewContext/runInThisContextonScriptand no remaining references to the removedvmModuleRunIn*symbols exist. NodeVMSourceTextModule:sourceText.length()is read before theWTF::move; the pre-existinguint32_t → intnarrowing on the module path is unchanged.
Extended reasoning...
Overview
The PR fixes an integer-overflow abort in node:vm when lineOffset/columnOffset are near INT32_MAX. It adds clampOffsetForSource() in NodeVM.{h,cpp} and applies it at all three SourceCode construction sites (vm.Script, vm.compileFunction, SourceTextModule). It also removes the unreachable native vmModuleRunInNewContext/vmModuleRunInThisContext host functions plus the BaseVMOptions(String, OrdinalNumber, OrdinalNumber) constructor only they used, and adds 11 subprocess tests.
Security risks
None identified. The change narrows the accepted range of an already-int32-validated option to avoid signed overflow inside JSC's lexer. No new user input surfaces; the removed host functions were unreachable from JS (vm.ts re-implements both on top of Script).
Level of scrutiny
Moderate. The clamp itself is a 6-line arithmetic helper, and the two-line insertions in NodeVMScript.cpp/NodeVMSourceTextModule.cpp are mechanical. The two parts that deserve a closer look are (a) the control-flow reorder in constructAnonymousFunction — moving stringifyAnonymousFunction above the standalone body parse and adding RETURN_IF_EXCEPTION — and (b) the ~120 lines of native binding deletion. I verified (a) is safe because the ArgList passed in from vmModuleCompileFunction contains only pre-validated JSString values (no user toString coercion moved), and the new RETURN_IF_EXCEPTION closes a pre-existing hole where OOM would continue with a null program string. For (b), I grepped src/ and confirmed vm.ts routes both APIs through new Script(...).runIn*Context() and nothing else references the removed symbols.
Other factors
- The mechgate evidence shows 9/11 new tests fail on main (7 abort with the JSC assertion) and all pass with the fix, on both ASAN-debug and release.
- All 95
test/js/node/test/parallel/test-vm-*.jsfiles and the rest ofvm.test.tsstill pass, covering the removed-binding paths. - The comment-cop bot's concerns were addressed (comments trimmed to one line each; threads resolved).
- The PR notes an intentional overlap with #38235 on the
SourceTextModulelines and the test was written to be order-independent — whichever lands second needs a small merge. - Given this is C++ JSC binding code with a control-flow reorder and non-trivial dead-code deletion, deferring for a human sign-off rather than auto-approving.
…ositions Node's validator accepts any int32 for these options, but JSC stores positions as ints, converts the start position to one-based and counts the source's own lines on top of it. An offset near INT32_MAX therefore overflowed in the lexer (ASSERTION FAILED: line >= 0 in JSTextPosition::checkConsistency on assertion builds, wrapped negative line numbers on release builds). Clamp both offsets in vm.Script, vm.compileFunction and SourceTextModule so that offset + 1 + source length still fits in an int; a source cannot contain more line terminators or first-line columns than it has code units, so this is a no-op for every realistic value. compileFunction builds its wrapper program first and clamps against that, since the wrapper is the longest text it parses.
…ndings vm.ts has implemented both on top of Script since the node:vm rewrite, so these host functions (and the BaseVMOptions constructor only they used) were unreachable. They also built their SourceCode from the raw offsets, so dropping them leaves no unclamped path.
… line counting starts Use lineOffset INT32_MAX - 1 with one more line of source for the SourceTextModule and multi-line compileFunction cases, so the counter still steps past INT32_MAX without the clamp whether the first line is numbered lineOffset or lineOffset + 1.
78595bc to
e9cbc73
Compare
There was a problem hiding this comment.
LGTM — focused overflow fix with the whole class covered.
What was reviewed:
clampOffsetForSourcearithmetic: widens to int64 before subtracting the unsigned length and floors at 0, so huge sources can't underflow; negative offsets pass through unchanged.constructAnonymousFunctionreorder: wrapper is built first so both the body-only parse and the wrapped parse use clamped positions; the addedRETURN_IF_EXCEPTIONreplaces continuing with a null program string. Allargsare already validated JSStrings, so the reorderedtoWTFStringcalls are unobservable.- Dead-code removal:
vm.tsroutesrunInNewContext/runInThisContextthroughScript; no remaining references to the removed host functions or the 3-argBaseVMOptionsconstructor. NodeVMSourceTextModule:sourceText.length()is read before theWTF::move, and theSourceCodeline/column now come from the same clampedstartPositionas the provider.
Extended reasoning...
Overview
Adds clampOffsetForSource(offset, sourceLength) and applies it at all three node:vm entry points that build a JSC SourceCode from user-supplied lineOffset/columnOffset (vm.Script, vm.compileFunction, vm.SourceTextModule), preventing the int overflow in JSC's parser positions that aborted assertion builds and produced wrapped-negative line numbers on release builds. Also removes the unreachable native vmModuleRunInNewContext/vmModuleRunInThisContext host functions (and the BaseVMOptions constructor only they used), which were the only remaining unclamped SourceCode builders. 11 new subprocess test cases exercise construction and error-position reporting at the int32 edge.
Security risks
None. The input (lineOffset/columnOffset) was already validated as int32; the clamp only narrows the range further. No new user-reachable surface. The removed bindings were already unreachable from JS (vm.ts builds both APIs on Script).
Level of scrutiny
Medium. This is C++ in the JSC bindings, so integer arithmetic and exception-scope discipline matter, but the change is small and mechanical: one 6-line pure helper, three call sites, a hoist of an existing block plus a RETURN_IF_EXCEPTION, and dead-code deletion. The clamp math was checked by hand (int64 widening before the unsigned subtraction, std::max(..., 0) floor, negative offsets unchanged). The constructAnonymousFunction reorder was traced for observable side effects — none, since every args element is already a validated JSString by the time it reaches this function.
Other factors
- CI is fully green (Buildkite 179/179) and the evidence block confirms the new tests fail on main and pass with the fix on both ASAN-debug and release.
- All comment-cop threads (paragraph-length comments) were addressed and resolved; the surviving comments are one-liners.
- Verified via grep that
vm.tsroutes the module-levelrunInNewContext/runInThisContextthroughScript, and that nothing else references the removed symbols or the 3-argBaseVMOptionsconstructor. - The PR description flags the overlap with #38235 on the same
NodeVMSourceTextModule.cpplines and the test was adjusted (three-line source,INT32_MAX - 1) to remain valid whichever lands first.
2 participants
Problem
new vm.Script('1', { lineOffset: 2147483647 })andvm.compileFunction('return 1', [], { lineOffset: 2147483647 })abort on assertion-enabled builds withASSERTION FAILED: line >= 0inJSC::JSTextPosition::checkConsistency()(ParserTokens.h:231). Release builds survive and print wrapped line numbers (big.js:-2147483648).lineOffset: 2147483646plus a two-line source, andnew vm.SourceTextModule('1;\n2;', { lineOffset: 2147483647 }), abort the same way.lineOffset/columnOffset, and the bindings hand the value to JSC unchanged (NodeVMScript.cppmakeSource(...),NodeVM.cppconstructAnonymousFunction,NodeVMSourceTextModule.cppcreate). JSC stores positions asint:OrdinalNumber::oneBasedInt()isvalue + 1, and the lexer does++m_lineNumberper line terminator starting from that (Lexer.cppsetCode/shiftLineTerminator), so any offset within (source line count + 1) ofINT32_MAXoverflows. The+ 1overflow is undefined behaviour, which is why a debug build and the optimized asan build fail on different inputs.Fix
clampOffsetForSource(offset, sourceLength)(NodeVM.h/NodeVM.cpp) and applies it to both offsets invm.Script,vm.compileFunctionandSourceTextModulebefore theSourceCodeis built: the offset is capped atINT32_MAX - 1 - sourceLength.+1for one-based, per-line increments, first-line column additions, anddecorateParseErrorStack's ownline + offset) stays withinint. For any realistic offset the clamp is a no-op; an absurd one now reports positions just belowINT32_MAXinstead of undefined behaviour (e.g.big.js:2147483627for the 20-character repro).compileFunctionnow builds its wrapper program before the standalone parse of the body and clamps against the wrapper's length, since that is the longest text it parses (the body is re-parsed as line 2 of it). The earlyRETURN_IF_EXCEPTIONafter building the wrapper replaces continuing with a null string.decorateParseErrorStackand the provider's start position see the same value as the parser.SourceTextModulestill passes its (zero-based) values toSourceCodeexactly as before; the off-by-one there is fixed separately in node:vm: apply SourceTextModule lineOffset/columnOffset like Node and name frames after the identifier #38235, which touches the same lines. Whichever of the two lands second only has to wrap that PR'sTextPositionoperands inclampOffsetForSource; the module test case here usesINT32_MAX - 1with a three-line source so it fails without the clamp under either line-numbering.runInNewContext/runInThisContexthost functions fromNodeVM.cpp(second commit), plus theBaseVMOptionsconstructor only they used.vm.tshas built both APIs onScriptsince node:vm compatibility #19703, so they were unreachable, and they were the only remaining places that built aSourceCodefrom unclamped offsets.test/js/node/vm/vm.test.ts(describe("node:vm lineOffset/columnOffset at the edge of int32")): 11 subprocess cases covering construction of all three APIs and the reported line/column of runtime and compile-time errors. Without thesrc/change 9 of the 11 fail on the debug build (7 abort with the assertion above; the line/column cases otherwise report1/16, i.e. the offset silently lost); of the remaining 2, the one-lineScriptcase (the reported repro) only fails on optimized assertion builds, and thecolumnOffsetconstruction case never aborted (the column value itself is checked separately and did fail). With the change all 11 pass.vm.test.ts(225 pass),vm-sourceUrl.test.ts, and all 95test/js/node/test/parallel/test-vm-*.jsfiles (coversrunInNewContext/runInThisContextafter the removal); normal offsets (lineOffset: 5, negative offsets,columnOffset: 10) report the same lines and columns as before.Background
lineOffset/columnOffsettellnode:vmwhere a snippet sits inside a larger file so error positions line up with that file; they are zero-based and may be negative. Node validates them as int32 and otherwise passes them through, so2147483647is a legal input.OrdinalNumber(WTF) wraps anintthat can be read as zero- or one-based;TextPositionis a line/column pair of them. A JSCSourceCodecarries the one-based first line and column to start counting from, and the lexer counts lines upward from there in a plainint.JSTextPosition::checkConsistency()is a debug-only assertion that every token position the lexer produces has a non-negative line; it is what turns the overflow into an abort on assertion builds.vm.compileFunctionis implemented by wrapping the body in(function (params) {\n<body>\n})and compiling that as a program; the user's body is therefore line 2 of what JSC actually parses.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/vm/vm.test.ts