Fix non-object returns from native construct handlers#31955
Conversation
|
Updated 2:56 PM PT - Jun 16th, 2026
❌ @robobun, your commit 3d5152e has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31955That installs a local version of the PR into your bun-31955 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEnforce constructor/construct-call semantics: FFI/native functions become non-constructible; mock functions gain proper construct behavior and instance recording; Node buffer exports are made non-constructible; tests updated and added to validate these semantics. ChangesNon-constructible native and FFI functions
Mock function construction support
Node buffer module construction
Tests and misc
Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
CI status: the only red job is the Linux x64 ASAN shard, failing on test/regression/issue/30205.test.ts (the --isolate napi finalizer test). The spawned bun test --isolate run prints 8 pass, 0 fail, Ran 8 tests across 8 files, and then the child aborts with SIGABRT during process shutdown under BUN_JSC_collectContinuously=1. Why this is unrelated to the PR:
The abort-after-summary profile is the same GC timing family this test was added for in #30216, surfacing at exit rather than mid-run. It looks sensitive to the particular binary produced by each CI compile. Leaving this one for a maintainer to judge, since retriggering repeatedly will not converge. |
|
This also fixes #32103: Branch |
|
CI status update for build 61893: the earlier napi shutdown flake (30205) did not recur. The only failure is test/js/node/test/parallel/test-net-server-close-before-ipc-response.js in the Linux x64 ASAN shard, an upstream Node net/IPC timing test where the mustNotCall('listening') listener fired while the server was closing. It is unrelated to this PR (nothing here touches net, IPC, or event emission), it passed in every prior ASAN run of this branch with identical native code (builds 61263, 61267, 61270), and it passes 5 of 5 locally. Each red build on this branch has been a different unrelated timing flake in that shard; the PR's own tests are green across all platforms. Leaving retries to maintainers rather than pushing more empty commits. |
A native construct handler must return an object or throw: Interpreter::executeConstruct calls asObject() on the result, which is an assertion failure in debug builds and a type confusion in release builds when the handler returns undefined or another primitive. Three places registered a plain call handler as the construct handler: - JSMockFunction used jsMockFunctionCall for both call and construct, so Reflect.construct(jest.fn(), []) returned whatever the mock implementation returned, including undefined. Mock functions now implement ordinary-function construct semantics like Jest's JS mock functions: create this from newTarget.prototype, run the mock with that this, return the result if it is an object, otherwise the new instance. The constructed instance is recorded in mock.instances. - JSFFIFunction::create ignored its nativeConstructor parameter (which already defaults to callHostFunctionAsConstructor) and passed the FFI call handler instead; createForFFI did the same. FFI functions now throw when constructed. - node:buffer isAscii, isUtf8, and resolveObjectURL passed their call handlers as construct handlers. They now throw when constructed, like other native functions (the follow-up suggested in #22480). Found by fuzzing: constructing a jest spy tripped ASSERTION FAILED: cell->isObjectSlow() in JSC::asObject.
linkSymbols routes through createForFFI; runtime functions like the node:os natives route through JSFFIFunction::create. Assert that constructing those throws too.
Jest pushes this into mock.instances on every call, exactly like mock.contexts, so instances stays index-aligned with calls. Push thisValue unconditionally instead of only on the construct path, and drop the now-unused constructedObject parameter.
The test asserted the behavior of @angular/cli@latest from the live registry. Angular 22.0.0 now requires Node ^22.22.3 || ^24.15.0 || >=26.0.0 at runtime, so it prints a version error and exits 3 under bun x --bun, which reports Node v24.3.0. This fails on every branch, including the PR base commit. Pin to @angular/cli@21.0.0, whose engines are ^20.19.0 || ^22.12.0 || >=24.0.0, keeping the node 24 coverage the test exists for while making it independent of future publishes.
Constructible native functions are classified as classes by Bun.inspect, so the ignored nativeConstructor parameter in JSFFIFunction::create made console.log(os.hostname) print [class hostname]. The JSFFIFunction fix in this branch flips that to [Function: hostname]; add coverage for the node:os exports and the bun:ffi path, and let the userInfo test run where USER is unset, matching the existing SHELL fallback.
robobun
force-pushed
the
farm/6b0428f9/fix-mock-construct
branch
from
4bee6da to
3d5152e
Compare
|
Build 62900 (the rebased sha 3d5152e) failed at CI agent provisioning: |
1 participant
What does this PR do?
Fixes a fuzzer-found crash (debug assertion
cell->isObjectSlow()inJSC::asObject, a type confusion in release builds) caused by native construct handlers returning non-objects.JSC's
Interpreter::executeConstructends withreturn asObject(JSValue::decode(result));, so a native[[Construct]]implementation must return an object or throw. Returningundefinedor another primitive asserts in debug builds; in release builds the primitive is reinterpreted as aJSObject*, and C++ callers ofJSC::construct()then operate on a garbage pointer.Three places registered a plain call handler as the construct handler:
JSMockFunctionregisteredjsMockFunctionCallfor both call and construct, soReflect.construct(jest.fn(), [])returned whatever the mock implementation returned, includingundefined. This is what the fuzzer hit, via constructing aspyOnmock. It was also a visible compat bug:new (jest.fn())()evaluated toundefinedin release builds, while Jest returns a new instance.JSFFIFunction::createaccepted anativeConstructorparameter (defaulting tocallHostFunctionAsConstructor) but ignored it and passed the FFI call handler instead;createForFFIdid the same.Reflect.construct(dlopen(...).symbols.fn, [])hit the same assertion whenever the FFI return type was a primitive.node:buffer'sisAscii,isUtf8, andresolveObjectURLpassed their call handlers as construct handlers, so constructing them returned booleans/undefined. fix(buffer): use correct constructor forbuffer.isAscii#22480 already suggested removing these constructors; this does that.The fix
JSMockFunctiongets a realjsMockFunctionConstructimplementing ordinary-function construct semantics, which is what Jest mock functions (plain JS functions) get for free: createthisfromnewTarget.prototype(falling back to%Object.prototype%), run the mock-call logic with thatthis, return the implementation's result if it is an object, otherwise the new instance. Like Jest, every invocation records itsthisintomock.instancesandmock.contexts(so a construct records the new instance, and the arrays stay index-aligned withmock.calls). The shared bookkeeping moved intojsMockFunctionCallImpl, which takesthisValueexplicitly (for a native construct frame,callframe->thisValue()holdsnewTarget, not a usablethis- the old code was also recordingnewTargetintomock.contexts).JSFFIFunctionpassesnativeConstructorthrough (andcallHostFunctionAsConstructorincreateForFFI), so FFI functions throwTypeErrorwhen constructed, like every other Bun host function.node:buffertrio drops the explicit construct handler, using thecallHostFunctionAsConstructordefault.The mock
[[Construct]]line is the fix for the fuzzer crash; the other two are the same bug class found by auditing everyInternalFunction/getHostFunction/JSFunction::createregistration in the tree. All remaining construct handlers return an object or throw on every path. (One latent case left alone: the v8 shim'sObjectTemplateusesTemplate::DummyCallbackfor construct, but templates are never exposed as JS values.)Behavior changes
new (jest.fn())()now returns a new object instead ofundefined(matches Jest).new (require("node:buffer").isAscii)(buf)now throwsTypeErrorinstead of behaving like a call. Node returns a fresh object here (V8 makes all native functions constructible, which ECMA-262 disallows for builtins not identified as constructors); Bun already throws for nearly all native functions (path.join,os.cpus, ...), so this follows the existing convention. The test added in fix(buffer): use correct constructor forbuffer.isAscii#22480 asserting the old call-through behavior is updated.bun:ffifunction now throwsTypeErrorinstead of being undefined behavior.Bun.inspectstops classifying them as classes:console.log(os.hostname)now prints[Function: hostname]instead of[class hostname]. Fixes OS Module os["class"] gives wrong output #32103.console.log(os.hostname)now prints[Function: hostname]instead of[class hostname]: Bun's inspect classifies constructible native functions as classes, and the ignorednativeConstructorparameter made every node:os binding export constructible. Fixes OS Module os["class"] gives wrong output #32103.Unrelated CI fix included
Fixes #31797.
test/cli/install/bunx.test.ts("should handle package that requires node 24") ranbun x --bun @angular/cli@latestagainst the live registry. Angular CLI 22.0.0 now requires Node^22.22.3 || ^24.15.0 || >=26.0.0at runtime and exits 3 under Bun (which reports Node v24.3.0), so the test fails on every branch, including this PR's base commit. The test is now pinned to@angular/cli@21.0.0(engines^20.19.0 || ^22.12.0 || >=24.0.0), which keeps the node 24 coverage it exists for and stops it from tracking future publishes.How did you verify your code works?
Reflect.constructon a spy) aborts with the assertion on an unfixed debug build and exits cleanly on the fixed one.test/js/bun/test/mock-fn.test.js(constructing a mockdescribe block),test/js/node/buffer.test.js,test/js/node/buffer-resolveObjectURL.test.ts, andtest/js/bun/ffi/ffi.test.js. They fail withUSE_SYSTEM_BUN=1and pass with the fixed build.mock.instances/mock.contexts/mock.resultsrecording for plain calls, constructs, and mixed sequences. One pre-existing difference is unchanged: mocks have no default own.prototypeproperty (they are InternalFunctions), so without assigningfn.prototypethe instance gets%Object.prototype%andinstanceof fnthrows, as it already did before this PR,newTarget.prototypehandling viaReflect.construct, object-return-wins, and primitive-return-falls-back-to-instance.buffer.test.js(506 pass),mock-fn.test.js(81 pass),mock-modulesuites,spyMatchers.test.ts, andffi.test.jspass. Exception checks validated withBUN_JSC_validateExceptionChecks=1, plus a GC stress loop over the new construct path.