Repository navigation
bindgen: delete b.any and .variadic - #46789
dylan-conway wants to merge 65 commits into
Conversation
|
Updated 6:05 AM PT - Oct 29th, 2026
❌ @dylan-conway, your commit b2011a2 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 46789That installs a local version of the PR into your bun-46789 --bun |
| } | ||
| let error_code = code.to_int32(); | ||
| }; | ||
| let error_code = code as i32; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: node:http2 users calling session.goaway with an error code of 2**31 or more now send 0x7FFFFFFF on the wire instead of the code they passed. src/runtime/api/bun/h2_frame_parser.rs:4684 does code as i32, which saturates, and then ErrorCode(error_code as u32) at :4706 and :4714. The base used to_int32() (modular ToInt32), so as u32 restored the exact uint32. Fix: convert the f64 straight to u32 (code as u32), as rst_stream at :4943 and emit_error_to_all_streams at :6268 already do, so every code in the 0..=4294967295 range that http2.ts validates reaches the GOAWAY frame unchanged.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Http2Session.prototype.goaway(code) in src/js/node/http2.ts:4691 validates code with validateInteger(code, "code", 0, kMaxUint32) and passes it to the parser; the client-side goaway at src/js/node/http2.ts:5628 passes it through too. In src/runtime/api/bun/h2_frame_parser.rs:4684 the new body does let error_code = code as i32;, which saturates any f64 >= 2147483648 to 2147483647. ErrorCode(error_code as u32) at :4706 and :4714 therefore puts 0x7FFFFFFF into the GOAWAY frame. On the base branch the same line was code.to_int32() (ECMAScript ToInt32, modular), so 4294967295 became -1 and as u32 gave back 4294967295, the exact code the caller passed. The peer now receives a different error code than the application sent for any code in 2147483648..=4294967295, which the JS validation explicitly allows. The sibling conversions code as u32 at :4943 (rst_stream) and :6268 (emit_error_to_all_streams) do not have this problem.
Verification: pre-existing — triggered when session.goaway(code) is called with code >= 2**31. src/runtime/api/bun/h2_frame_parser.rs:4684 let error_code = code as i32; saturates to i32::MAX, and :4706/:4714 put 0x7FFFFFFF into the GOAWAY frame. The base did NOT preserve the value: base JSValue::to_int32 (179664e:src/jsc/JSValue.rs:694-707) also saturates, so base sent the same wrong code.
…as before, and the arguments a body tested by class have their types
…into claude/bindgen-no-any # Conflicts: # src/runtime/api/JSSourceMap.rs # src/runtime/api/YAMLObject.rs
… takes `undefined` and that an array argument is only read
…o `b.object` takes `object` and gives `any`
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 2 findings from earlier reviews are still open above.
Still open from earlier reviews (2):
- 🔴
test/integration/bun-types/bun-types.test.ts:321—The bun-types integration test fails on every release CI run after merging, because the new schema file is missing from… - Also unresolved: 1 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
| }, | ||
| { fromJS: true }, | ||
| ); | ||
| const id = b.i32.validateIntegerRange(-2147483648, 2147483647).nanAs(0).nullable; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing, security-relevant: a root process calling Bun.spawn({ uid: NaN }) or { gid: NaN } spawns the child as uid/gid 0 instead of throwing. The schema at src/runtime/api/bun/js_bun_spawn_bindings.bind.ts:27 maps NaN to 0 with .nanAs(0), and js_bun_spawn_bindings.rs:611-617 passes that 0 to the OS. A NaN here is almost always a bug in the caller (parseInt of an unset env var), so the privilege drop silently does not happen. Fix: reject NaN for uid and gid (drop .nanAs(0) so validateIntegerRange reports it as ERR_INVALID_ARG_TYPE, or throw ERR_OUT_OF_RANGE as Node's validateInteger does), keeping null/undefined as "not set".
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A process running as root calls Bun.spawn({ cmd, uid: Number(process.env.RUN_AS) }) with the variable unset, so uid is NaN. The member type at src/runtime/api/bun/js_bun_spawn_bindings.bind.ts:27 is b.i32.validateIntegerRange(-2147483648, 2147483647).nanAs(0).nullable, used for uid and gid at :57-58. In C++, src/jsc/bindings/Bindgen/IDLConvertIntegerRange.cpp:24-25 returns the nan value 0 before the integer and range checks. src/runtime/api/bun/js_bun_spawn_bindings.rs:611-617 then sets uid = Some(0) and the child is spawned as root, with no error. Node's validateInteger throws ERR_OUT_OF_RANGE ("an integer") for NaN, and Bun's own node:child_process guards its path with isInt32 at src/js/node/child_process.ts:989, so only direct Bun.spawn callers are affected. The base branch behaves the same: src/jsc/JSGlobalObject.rs:1193-1196 on 179664e returned the default 0 for NaN, so this is pre-existing, but the diff re-encodes it explicitly in the schema. A non-root caller gets EPERM from setuid(0) instead, which hides the real cause.
Verification: pre-existing (security-relevant, does not block). Trigger: a process calls Bun.spawn directly with uid or gid equal to NaN. src/runtime/api/bun/js_bun_spawn_bindings.bind.ts:27 uses .nanAs(0), so the OS receives uid/gid 0 and a root parent spawns a child that still runs as root instead of throwing. Base branch fails by the same route: src/jsc/JSGlobalObject.rs:1193-1196 returns Ok(default) on NaN.
…`, which nothing calls
…functions, and the commands of `RedisClient` and the paths of `node:fs` are `.deferred`
…f the promise settled in the tick that raised it
… and `expected` keeps the message
…says share one function
… so no argument is `.deferred` to save code
… a shared one is named after its type
…what they received
|
A question about order, from #48124 (stacked on #47993). Both change
If this PR lands first, I redo both on it. The shared Which order do you prefer? |
|
A question about order, from #43563. #43563 makes If this PR lands first, I redo #43563 on it. The path rows are then one statement in each parser: convert the options before the path. Two lines of the schema decide the rest.
If #43563 lands first, this PR gets conflicts in the seven parser heads of Which order do you prefer? |
What does this PR do?
Deletes
b.anyand.variadicfrom bindgen. A schema now says one of two things about every argument, member and result: what the generated code converts it to, or that it is handed over as it is.b.any.variadicb.opaqueb.rest(..)b.union(..)b.dictionary(..)deferredThe schemas of the generator's own tests are not counted.
What each
b.anybecameb.anythat the body converted by hand: a string to text or bytes, a number to an integer, an options object to fields, an instance to its native pointerb.anythat the body stores, hands on, returns, or tests itself (is_number(),as_array_buffer())b.opaque, and the body keeps main's textb.anywhose conversion JavaScript can observe, so it has to run at a point the body picksT.deferred, orb.dictionary(members, { deferred: true })for an options objectb.any.variadicb.rest(b.opaque)The rules the schemas follow:
b.opaque.b.opaquewith main's text. No dictionary is built below the entry from a stored value.src/jscand the helpers keep theirJSValuesignatures. A parser that turns the typed argument into the tree's own type (Arguments::from_js(options),Value::from_js(BodyInit)) takes the generated type where that is shorter than main's text.b.undefinedismissing,b.nullisnone, and a lastb.ignoredorb.opaqueisother.What bindgen gained
docs/project/bindgen.mdxdescribes each.b.opaque,b.ignored,b.cell(..)as the type of an argument,b.wrapped(..)with.shared,b.promise,b.dataView,b.abortSignal,b.stringObject,b.stringLike,b.bigInt,b.jsNumber,b.i64..deferred,.validateIntegerRange(),.nanAs(),b.i64.asIntN.deferred: trueon a dictionary,fromJSon a union,reportedAson a member,own,.ifPassedandprivateSymbolon a member of adeferreddictionary, a union as an alternative of a union.convert_*first tests the bits of the value or the type byte of its cell:undefinedfor a type with a default, a boolean, a number, a safe integer in range, a string, a BigInt, an array, a function that is not a proxy. It calls C++ for the rest.One change outside the bindings:
wait_for_promisereturnsStoppedwhen a termination is pending, even if the promise settled in the tick that raised it. A conversion asserts if it runs with a termination pending.Behaviour
Only what differs from main is listed.
Wrong on main, right here (3). Each test fails on main.
Bun.build({ naming: "[name].js" })namingtwicebun-build-api.test.ts: "a string naming is read once"fs.promises.write(fd, new String("a"))toStringof the object, then refuses itfs.test.ts: "promises.write refuses a String object without converting it"init(.., new Proxy(fn, {}))of a native zlib, brotli or zstd handlezlib-handle-bounds-check.test.ts: "init takes a callable Proxy as the write callback"Right on main, right here, reads differently. The class of the error and its
codeare equal in every case. 86 wordings differ; these are the groups, each with one example.node:fsfunction,nameofBun.TerminalThe "bigint" property must be of type boolean, got stringThe "options.bigint" property must be of type boolean. Received type string ('a'), Node's formpositionoffs.readandfs.writemust be of type number or bigintmust be of type bigint or integer, Node's wordingsignalofnode:fs,Bun.spawn, an HTTP/2 requestmust be of type AbortSignalmust be an instance of AbortSignal, Node's wordingrmdir,SocketAddressThe "options.silent" argument ..The "options.silent" property .., Node's wordingBun.buildThe "target" property must be of type string, got numbertarget must be a stringExpected "error" callback to be a functionhandlers.error must be a functiononExit,onDisconnect,timeout,cgroupofBun.spawnonExit must be a function or undefinedonExit must be a functionserver.requestIP(1),server.subscriberCount(1)Expected Request objectrequest must be an instance of Requestchown,futimes,appendFile,fs.promises.readandwriteuid is requiredThe "uid" argument must be of type number. Received undefined, Node's wordingbufferoffs.promises.write,dictionaryof a brotli handlemust be of type string or TypedArraymust be of type string or an instance of Buffer, TypedArray, or DataView, Node's wordingaltsvc(origin, value)of the native HTTP/2 parsermust be of type originmust be of type stringflagsandfdofBun.udpSocket,modeofchmodThe "flags" property must be of type number, got stringThe "flags" argument must be of type number. Received type string ('a')sourcemapof the build options ofBun.serve({ app })The "sourcemap" property must be of type "inline" | "external" | "linked", got numbersourcemap must be a stringfs.readvSync(fd, 1)Expected an ArrayBufferView[]Expected ArrayBufferView[]socket.setKeepAlive(true, -1)It must be >= 0 and <= 9007199254740991It must be >= 0 and <= 2147483647, the bound both enforcemodeorflagsabove 2**32Received 8589934592Received 8_589_934_592: Node's wording formodeandflags, and Node prints an fd without the separatorsA refused object has its
constructorread once, forReceived an instance of ..in the message. AProxysees that read.Right on main, wrong here: 0. Wrong on main, wrong in another way here: 0.
Cost of a call
Counted by reading the generated code and the bodies of main and of this PR: calls from Rust into C++, property reads, allocations and copies on a call that succeeds.
readSync;statSync,lstatSyncandmkdirSyncwith options (2 to 1);existsSync,unlinkSyncandrenameSyncwith aURL;writeSyncof a string in an encoding other than UTF-8;closeSync,fsyncSync;Bun.writeto aBlob;new Blob(parts, { type })(2 to 1); a body made fromFormData,URLSearchParamsor aBlob;ws.send,sendText,ping,pongandpublish*with a string (2 to 1);server.publish;fetch(string); an HTTP/2request;setKeepAlive,setNoDelay; the options ofBun.udpSocket.args::Writeloses a field and so doesPayload::Bytes.Binary size
The stripped release
bunfor Linux x64 is 138,390,072 bytes, 106,496 more than main (138,283,576). The growth is the conversions themselves: main converted these arguments with shared helpers that take aJSValue, and the generated code has one conversion for each typed argument and member, with its name and its error text..deferred, the members of adeferreddictionary)node:fs--aottableBun.build--aot621 members have an entry that compiled code calls directly; main has 815. A union has no direct form, so a member with a union argument is called through its host function. The cost is not measured.
How did you verify your code works?
test/internal/bindgen.test.tscovers each new type, modifier and refusal. It also converts a list of values with and without.deferredand compares the results.toString,valueOfand Proxy traps. Release build against release build: 314,731 lines, of which 8,451 differ in the message only and 36 in theconstructorread. A script sorts every differing line into "message only", "constructorread on a refused value" and "other", and every "other" line was read: clock, port, working directory, temporary names, and races of calls that are not awaited. The tables above are that output.--aottable of built-ins, the source lints, andbun-types.test.ts.