Repository navigation
Conversation
…t string a second time parseArgs converts an args element to a string, classifies that string, and kept the element as the token's raw. For --name=value and for an option in a short option group, the raw name was cut out of a second conversion of the element, by the shape of the first one. When the two conversions differed, an unwrap() or a slice bound panicked and the process aborted. These three token shapes now keep the string that was classified. A lone option keeps the element, because its raw name is the whole element.
|
Updated 10:14 AM PT - Oct 21st, 2026
✅ @robobun, your commit 5033db45346579feb08b92ef746e7e6c4990ff0d passed in 🧪 To try this PR locally: bunx bun-pr 46324That installs a local version of the PR into your bun-46324 --bun |
|
Status: ready for review How it is reproduced, on Bun 1.4.0, 1.4.2 and on a release build of main: let n = 0;
const arg = { toString() { return n++ === 0 ? "--foo=bar" : "nothing"; } };
console.log(JSON.stringify(require("node:util").parseArgs({ args: [arg], strict: false, tokens: true })));
The self-review and the release measurements against main are in the description (Notes). |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
ChangesNon-string parseArgs option handling
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/node/util/parse_args.rs— Strict-mode callers passing a lone-for--fooelement that is not a string still get a second usertoStringrun, so the error they see names a different option or is replaced by whatever that second conversion throws. The two sibling producers at parse_args.rs:589 and :720 keepraw: arg_ref(the element), and check_option_usage at :326 and :270 plus check_option_like_value at :228 calltoken.raw.as_bun_string(global)?again. The three cut sites were switched toValueRef::Bunstr(&arg)but these siblings of the same class were not. Fix: every strict-mode reader must format the string classify_token read at :535; store the classified string for Lone* tokens too and keep the element only formake_raw_name_js_value(:158) sorawNameidentity is unchanged. [also at: src/runtime/node/util/parse_args.rs:806 - Callers that disable positionals and pass a non-string element get anUnexpected argumentmessage naming a string the parser never saw, or their element's secondtoStringexception instead of ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL.]Why this was flagged
Trigger:
parseArgs({ args: [obj], options: {} })(strict is the default) whereobj.toString()returns "--foo" the first time and "--bar" or throws the second time. tokenize_args converts once at parse_args.rs:533, classifies "--foo" as LoneLongOption, and stores the element at :720 (raw: arg_ref). handle_token at :787 calls check_option_usage, which at :326 callstoken.raw.as_bun_string(global)?on the JSValue, runningtoStringa second time. The user getsUnknown option '--bar'for an option that was parsed asfoo, or the user's own exception instead of ERR_PARSE_ARGS_UNKNOWN_OPTION. Same at :589 for-fand at :228/:270. The base branch behaves the same, but the PR fixes exactly this class at :635, :659 and :743 and leaves these two producers, contrary to the fix-the-whole-class rule. Remedy: storeValueRef::Bunstr(&arg)for Lone* tokens as well and have only make_raw_name_js_value return the element forrawName.Verification: pre-existing, acknowledged in diff: the doc comment at src/runtime/node/util/parse_args.rs:106 ("The element only for a lone option") records that lone options still hold the JSValue. The producers at :589 and :720 keep
raw: arg_ref; the diff changed only :635, :659, :743. :326, :270 and :228 calltoken.raw.as_bun_string(global)?, re-running user conversion, so strict mode throwsUnknown option '--bar'although "--foo" was classified. -
🟣
src/runtime/node/util/parse_args.rs— Callers whoseargsarray is shrunk by an element'stoStringgetvalues.fooset totrueand the following argument silently skipped, where Node parses the array as it was when called. The first conversion at parse_args.rs:533 runs user code;args.endwas fixed at :529 butargs.get(global, index + 1)at :618, :573 and :710 reads the live array, so a removed index yieldsundefinedas the consumed value andindex += 1at :641/:596/:727 skips it. Node copiesargswithArrayPrototypeSlicebefore tokenizing. Fix: snapshot every element (or at least its length and values) before any conversion so user code duringtoStringcannot change which element a string option consumes.Why this was flagged
Trigger:
parseArgs({ args, options: { foo: { type: "string" } }, strict: false })withargs = [obj, "x"]whereobj.toString()doesargs.pop()and returns "--foo". tokenize_args computesnum_args = 2at parse_args.rs:529 and converts obj at :533, running the pop. Sinceindex + 1 < num_argsstill holds, :710 callsargs.get(global, 1), which isget_indexon a now-shorter array and returnsundefined(parse_args.rs:27). The token at :714-724 storesvalue: undefinedwithinline_value: false; store_option writesvalues.foo = undefinedand :727 advances past the vanished element. Node v26 runsArrayPrototypeSlice(args)first and consumes "x". The base branch has the same lines, so this is not a regression, but the PR is about elements whose conversion runs user code and this is the remaining way such code changes the parse. Population: any caller forwarding caller-controlled arrays with non-string entries. Remedy: read all elements into a MarkedArgumentBuffer-style snapshot before the loop, or at minimum re-checkget_lengthafter each conversion.Verification: pre-existing. Trigger: an
argselement whosetoStringmutates the caller'sargsarray while returning an option-like string.num_argsis frozen at src/runtime/node/util/parse_args.rs:529; the conversion at :533 runs user code; :573, :618 and :710 then read the live array gated only by the staleindex + 1 < num_args. The base branch has the identical code, so merging makes nothing worse.
…it code A fixture process that aborts again now shows its panic message in the failure, not only exit code 134.
|
Thanks for the review. One change is pushed (5033db4). Two points stay as they are.
Lone options and positionals are converted a second time in strict mode. Left as is on purpose. The description has it under "Left as is, on purpose".
|
There was a problem hiding this comment.
LGTM — the three raw sites now borrow the string that classify_token read, and the latest commit asserts the fixture's stderr alongside its exit code, which covers the one nit from the previous pass.
What was reviewed:
- Traced every consumer of
OptionToken.raw(make_raw_name_js_value,RawNameFormatter, and the three strict-mode error sites): for the short-group and--name=valueshapes all go throughas_bun_string, which now returns the borrowed classified string, so the=index and thesubstring_with_lenoffsets are computed on the same string; the lone-option branches still return the original element viaas_js_value. - Checked the
&argborrow lifetime: tokens are consumed synchronously insidehandle_tokenwithin the loop iteration, matching the existingBunstr(&raw)pattern in theShortOptionAndValuearm. - Checked the test for vacuity: the fixture is spawned once with both pipes drained concurrently, each
test.eachcase compares the five element kinds against the plain-string result withtoEqual(includingconversions: 1), and an abort on the unfixed build leavesfromElementsempty so every case fails rather than passing. - A separate file instead of extending
parse-args.test.mjsmatches the directory's existing precedent of per-topic files.
Extended reasoning...
The change is three one-line edits in src/runtime/node/util/parse_args.rs replacing raw: arg_ref with raw: ValueRef::Bunstr(&arg) for the short-option-group (both branches) and --name=value token shapes, plus a spawned fixture and test file under test/js/node/util/parse_args/. It touches no security-sensitive surface; it removes a user-reachable panic (unwrap on None / slice out of range) triggered by an args element whose string conversion changes between calls. The fix is small, self-contained, and its correctness follows directly from reading the raw-name consumers, which all slice the string via as_bun_string. No CODEOWNERS entry covers the changed paths, no CHANGES_REQUESTED review is outstanding, and the only prior inline nit (unasserted fixture stderr) was addressed in commit 5033db4.
Problem
util.parseArgsaborts the process when two string conversions of oneargselement differ (a changingtoString()):panic: called `Option::unwrap()` on a `None` valueorrange end index 3 out of range for slice of length 2, exit 134.tokenize_argsclassifies the first conversion, but kept the element asraw.RawNameFormatterandmake_raw_name_js_value(src/runtime/node/util/parse_args.rs:115,:142) converted it again and cut the new string by the shape of the first.Fix
raw: ValueRef::Bunstr(&arg)at:635,:659(short option group) and:743(--name=value).classify_tokenread. A lone option keeps the element, its whole raw name, as in Node.test/js/node/util/parse_args/parse-args-non-string-args.test.ts(16 tests, 15 fail before). Also theparse_argssuites and Node'stest-parse-args.mjs.Background
rawNameis the option as written, without an inline value:--foofor--foo=bar.REVIEW.md:47and Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021 count such a panic as a crash.rawNameandnamethen come from different strings. A raw name built by the tokenizer for every shape changes two strict-mode messages and measured slower (Notes).Downsides
args: 0 differing results in 120,000 fuzz calls, default-path instructions 25,323 to 25,330 (spread 24), wall-clock inside A/A noise, same binary size.rawNameis from the first conversion. Node differs (Notes).Notes
Repro. Each file prints a result under Node v26.3.0 and aborts under Bun 1.4.0 to 1.4.4-canary.
With this change:
Node v26.3.0. Node documents
argsasstring[]. It does not check or convert the elements. It readsarg.lengthfrom the raw element and converts the element again in everyString.prototypecall. So it never aborts, and it never parses such an element like its first string (0 of the 70 cells of the test fixture). For the three repros it returns an empty result ({"values":{},"positionals":[],"tokens":[]}for ts1 and ts2). A plain object gives an empty result in 11 of the 14 fixture cases. AStringsubclass gives options from the later string (-o -t -h -i -nfor"nothing"). An array with one item gives a positional. Bun classifies a non-string element by its string conversion, before and after this change.History. The readers that convert again date from #7310. Bun 1.0.16, 1.2.23, 1.3.13 and 1.3.14 exit 0 on the repros, with a
rawNameread past the end of the second string ("",-and a NUL character,"-A"). Bun 1.4.0, 1.4.2 and 1.4.4-canary exit 134.Other triggers. A
toString()is not needed. ABufferover aSharedArrayBufferthat a Worker rewrites aborts main in 3 of 3 runs. With this change it ran about 4 million calls per run with no abort.Left as is, on purpose.
Unknown option 'x', Node:'-x'). util.parseArgs: include leading '-' in unknown-option error for grouped short options #36037 is open for it.Not in this PR. Branch
robobun/41a3d441/parse-args-raw-namebuilds the raw name in the tokenizer for every shape, so no reader holds the element. It also removes the second conversion for lone options and positionals, which changes two strict-mode messages for these elements. On release builds it ran fewer instructions on the default path (25,323 to 25,056) but more time on plain strings withtokensoff: 64 positionals 2,586 to 3,516 ns per call, 64 lone long options 6,272 to 7,348 ns.Measurements. Release builds of main and of main plus this change, same tree and flags. The fuzz scripts are my own.
toString()calls for one element (main -> this PR, Node in brackets):--foo=barwith tokens 2 -> 1 [4].-abcwith tokens 4 -> 1 [3]. Strict unknown--foo=bar2 -> 1 [5]. Lone option and positional errors 2 -> 2.BunString__fromJScalls perparseArgscall, string args (gdb breakpoint hits):tokens: truewith['--foo=bar','-abc']13 -> 9. The same without tokens 9 -> 9. Strict unknown--nope=19 -> 8.stepi, 11th call,['--foo=bar','-abc','-fX','pos'], 8 runs each): main 25,311 to 25,335, median 25,323. This PR 25,330 to 25,352, median 25,330.--k=v: 9,806 / 9,860 / 9,842.-abcgroups: 12,495 / 12,468 / 12,451. Withtokens: true,--k=v: 45,122 / 45,410 / 43,907.-abcgroups: 106,317 / 106,890 / 100,635.mi_*andWTF::fastMalloc): 14.23 -> 14.27 without tokens, 45.32 -> 45.33 with tokens (run-to-run noise about 0.05). Live GC cells per call: 5 -> 5 and 21 -> 21.parseArgstext 23,114 -> 22,897 bytes (nm -S).sizetext -256 bytes. The stripped binary is 107,271,904 bytes in both builds.valgrind,perf,bloatyorstrace: they are not installed in the build container.Crash reports. Not checked. A query for someone with access: frames in
parse_args.rs, releases from 1.4.0.Tracking issues. None filed. This PR records the abort, and #36037 records the missing dash.
Tests. One child process runs 14 cases, each for 5 kinds of element (object with
toString,Stringsubclass, array,Symbol.toPrimitive,valueOf). The test compares each result with the result for the plain first string and checks that the element was converted once. The file is new becauseparse-args.test.mjshas a stress test (100 mixed several times) that exceeds the 5 s default under a debug build on my machine, with and without this change.Suites on the debug build. New file: 16 pass.
parse-args-null-config10,parse-args-token-key-order1,default-args11, Node'stest-parse-args.mjs113.parse-args.test.mjs: 109 pass and the stress test above.Merge check.
git merge-fileof this change with the heads of #36037, #43905, #44521 and #40404 gives 0 conflict regions inparse_args.rs.