Fix use-after-free in Worker::terminate() and assertion in Reflect.construct(mock())#29200
Fix use-after-free in Worker::terminate() and assertion in Reflect.construct(mock())#29200
Conversation
…nstruct(mock()) Worker: the Zig WebWorker struct is freed on the worker thread when it exits, but the C++ Worker kept a raw impl_ pointer to it. A subsequent worker.terminate()/ref()/unref() from JS would dereference freed memory. Guard impl_ with a lock and clear it before the Zig side frees the struct. mock(): jsMockFunctionCall was registered as both the call and construct handler and can return undefined. Interpreter::executeConstruct calls asObject() on the result of a native constructor, which asserts isCell(). Add a dedicated construct wrapper that falls back to an empty object when the call path returns a non-object.
|
Updated 8:45 PM PT - Apr 11th, 2026
❌ @robobun, your commit 6c6de4f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29200That installs a local version of the PR into your bun-29200 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdded a dedicated construct handler for JSMockFunction to distinguish constructor invocations from calls. Made Worker impl_ access thread-safe by adding a lock and a clearZigImpl() method, and updated related methods. Added tests for mock constructor behavior and worker termination-after-exit handling. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/JSMockFunction.cpp`:
- Around line 985-995: The constructor implementation jsMockFunctionConstruct
must return the pre-created receiver instead of always allocating a fresh
object: if jsMockFunctionCall returns a non-object (primitive or undefined),
read the pre-created receiver via callframe->thisValue() and return/encode that,
and only fall back to JSC::constructEmptyObject(lexicalGlobalObject) defensively
if callframe->thisValue() is not an object; keep the existing exception handling
and continue returning encoded values from jsMockFunctionCall when it already
returns an object.
In `@src/bun.js/bindings/webcore/Worker.cpp`:
- Around line 141-145: updatePtr() currently reads impl_ without holding
m_implLock and Worker::create() assigns impl_ without locking, which can race
with the new protected writers (setKeepAlive(), clearZigImpl(), terminate());
fix by taking the same lock (m_implLock) when accessing impl_: add a Locker (or
equivalent lock guard) around the read in updatePtr() and around the initial
write to impl_ inside Worker::create() so all reads/writes of impl_ are
consistently synchronized with the existing
setKeepAlive()/clearZigImpl()/terminate() locking.
In `@test/js/web/workers/worker-terminate-after-exit.test.ts`:
- Around line 7-19: Listeners for "close" are being added after creating all
Worker instances, so some workers can emit "close" before the listeners are
registered; move the Promise.withResolvers and w.addEventListener("close", ...)
into the worker creation loop so each Worker registers its "close" listener
immediately upon instantiation (inside the loop that creates Worker instances),
collect the resulting promises (from Promise.withResolvers) into an array and
then await Promise.all on that promises array; reference symbols: workers,
Worker (constructor), Promise.withResolvers, w.addEventListener("close", ...),
and Promise.all.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1cda128c-e623-4482-b171-f611bea7317a
📒 Files selected for processing (5)
src/bun.js/bindings/JSMockFunction.cppsrc/bun.js/bindings/webcore/Worker.cppsrc/bun.js/bindings/webcore/Worker.htest/js/bun/test/mock-construct.test.tstest/js/web/workers/worker-terminate-after-exit.test.ts
| JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe)) | ||
| { | ||
| auto& vm = JSC::getVM(lexicalGlobalObject); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| JSC::EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| JSValue result = JSValue::decode(encodedResult); | ||
| if (!result.isObject()) | ||
| return JSValue::encode(JSC::constructEmptyObject(lexicalGlobalObject)); | ||
| return encodedResult; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ECMAScript, when Reflect.construct(target, args, newTarget) calls a constructor that returns a primitive, should the result be the original receiver created from newTarget.prototype or a fresh empty object? For JavaScriptCore native constructor host functions, is CallFrame::thisValue() that pre-created receiver?
💡 Result:
In ECMAScript, Reflect.construct(target, args, newTarget) invokes the [[Construct]] internal method of target with arguments args and newTarget. For ordinary ECMAScript function objects, [[Construct]] (10.2.2) creates thisArgument via OrdinaryCreateFromConstructor(newTarget, "%Object.prototype%"), which uses newTarget.prototype (or fallback) as [[Prototype]], then calls the function body with this bound to that object. After execution, if the constructor returns a primitive (including undefined), it ignores the return value and returns thisArgument—the pre-created object from newTarget.prototype—for base constructors. For derived constructors, it throws if non-object/non-undefined is returned. Thus, the result is always the original receiver created from newTarget.prototype. For JavaScriptCore native constructor host functions (built-in functions), CallFrame::thisValue holds the pre-created receiver. Per CallFrame.h comment: "Under the constructor implemented in C++, thisValue holds the newTarget instead of the automatically constructed value. The result of this function is only effective under the "construct" context. JSValue newTarget const { return thisValue; }". However, this indicates newTarget is accessible via thisValue in C++ host constructors, but the actual receiver (this binding) is the pre-created object from OrdinaryCreateFromConstructor or equivalent, passed as thisArgument to the host function per spec BuiltinCallOrConstruct (10.3.3). The comment clarifies special handling for C++ implemented constructors where thisValue slot serves dual purpose, but standard behavior provides the pre-created empty object as this.
Citations:
- 1: https://www.mgaudet.ca/technical/2020/7/24/investigating-the-return-behaviour-of-js-constructors
- 2: https://tc39.es/ecma262/multipage/reflection.html
- 3: https://github.com/WebKit/WebKit/blob/main/Source/JavaScriptCore/interpreter/CallFrame.h
- 4: https://tc39.es/ecma262/2022/multipage/ordinary-and-exotic-objects-behaviours.html
- 5: https://tc39.es/ecma262/2025/multipage/ordinary-and-exotic-objects-behaviours.html
- 6: https://read262.jedfox.com/ordinary-and-exotic-objects-behaviours/built-in-function-objects/
- 7: https://read262.netlify.app/ordinary-and-exotic-objects-behaviours/ecmascript-function-objects/
- 8: What's the reason for the fallback in OrdinaryCreateFromConstructor? tc39/ecma262#1264
🏁 Script executed:
# Search for tests related to mock functions and Reflect.construct
rg "Reflect\.construct" --type js --type ts -A 3 -B 1Repository: oven-sh/bun
Length of output: 23495
🏁 Script executed:
# Check for any existing JSC mock constructor implementations or patterns
rg "jsMockFunction" src/bun.js/bindings/ -A 2 -B 1Repository: oven-sh/bun
Length of output: 16827
🏁 Script executed:
# Look at the surrounding context of the jsMockFunctionConstruct function
sed -n '970,1005p' src/bun.js/bindings/JSMockFunction.cppRepository: oven-sh/bun
Length of output: 1546
Return the constructor receiver instead of allocating a fresh object.
The current implementation violates ECMAScript [[Construct]] semantics. When the mock implementation returns a primitive, constructEmptyObject() creates a fresh plain object, discarding the receiver that was pre-created from newTarget.prototype. Per the spec, [[Construct]] must return the pre-created receiver. The fix is to use callframe->thisValue() (the JSC equivalent of the spec's pre-created receiver) with a fallback to constructEmptyObject() for defensive safety.
💡 Proposed fix
JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe))
{
auto& vm = JSC::getVM(lexicalGlobalObject);
auto scope = DECLARE_THROW_SCOPE(vm);
JSC::EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe);
RETURN_IF_EXCEPTION(scope, {});
JSValue result = JSValue::decode(encodedResult);
- if (!result.isObject())
- return JSValue::encode(JSC::constructEmptyObject(lexicalGlobalObject));
+ if (!result.isObject()) {
+ JSValue thisValue = callframe->thisValue();
+ if (thisValue.isObject())
+ return JSValue::encode(thisValue);
+ return JSValue::encode(JSC::constructEmptyObject(lexicalGlobalObject));
+ }
return encodedResult;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe)) | |
| { | |
| auto& vm = JSC::getVM(lexicalGlobalObject); | |
| auto scope = DECLARE_THROW_SCOPE(vm); | |
| JSC::EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe); | |
| RETURN_IF_EXCEPTION(scope, {}); | |
| JSValue result = JSValue::decode(encodedResult); | |
| if (!result.isObject()) | |
| return JSValue::encode(JSC::constructEmptyObject(lexicalGlobalObject)); | |
| return encodedResult; | |
| } | |
| JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe)) | |
| { | |
| auto& vm = JSC::getVM(lexicalGlobalObject); | |
| auto scope = DECLARE_THROW_SCOPE(vm); | |
| JSC::EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe); | |
| RETURN_IF_EXCEPTION(scope, {}); | |
| JSValue result = JSValue::decode(encodedResult); | |
| if (!result.isObject()) { | |
| JSValue thisValue = callframe->thisValue(); | |
| if (thisValue.isObject()) | |
| return JSValue::encode(thisValue); | |
| return JSValue::encode(JSC::constructEmptyObject(lexicalGlobalObject)); | |
| } | |
| return encodedResult; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/JSMockFunction.cpp` around lines 985 - 995, The
constructor implementation jsMockFunctionConstruct must return the pre-created
receiver instead of always allocating a fresh object: if jsMockFunctionCall
returns a non-object (primitive or undefined), read the pre-created receiver via
callframe->thisValue() and return/encode that, and only fall back to
JSC::constructEmptyObject(lexicalGlobalObject) defensively if
callframe->thisValue() is not an object; keep the existing exception handling
and continue returning encoded values from jsMockFunctionCall when it already
returns an object.
There was a problem hiding this comment.
For InternalFunction native constructors in JSC, callframe->thisValue() is newTarget() (see the comment in CallFrame.h you quoted) — there is no pre-created receiver. Returning thisValue() would return the newTarget constructor itself, not an instance.
Updated in 98a869b to use InternalFunction::createSubclassStructure so the fallback object inherits from newTarget.prototype, matching other native constructors in the codebase. Added a test for instanceof.
| void Worker::clearZigImpl() | ||
| { | ||
| Locker locker { m_implLock }; | ||
| impl_ = nullptr; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Worker::updatePtr call sites =="
rg -nP '\bupdatePtr\s*\(' src --glob '*.{cpp,h,zig}'
echo
echo "== All impl_ accesses in Worker.* =="
rg -nP '\bimpl_\b' src/bun.js/bindings/webcore/Worker.cpp src/bun.js/bindings/webcore/Worker.h
echo
echo "== WebWorker__create declarations/implementations =="
rg -nP 'WebWorker__create\s*\(' src --glob '*.{cpp,h,zig}'
echo
echo "== WebWorker__dispatchExit declarations/implementations =="
rg -nP 'WebWorker__dispatchExit\s*\(' src --glob '*.{cpp,h,zig}'Repository: oven-sh/bun
Length of output: 1545
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Worker.cpp lines 145-155 (updatePtr function context) =="
sed -n '145,155p' src/bun.js/bindings/webcore/Worker.cpp
echo
echo "== Worker.cpp lines 195-235 (WebWorker__create and impl_ assignment) =="
sed -n '195,235p' src/bun.js/bindings/webcore/Worker.cpp
echo
echo "== Worker::create full signature =="
rg -B5 'void\* WebWorker__create' src/bun.js/bindings/webcore/Worker.cpp | head -20Repository: oven-sh/bun
Length of output: 2193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JSWorker.cpp lines 340-360 (updatePtr call context) =="
sed -n '340,360p' src/bun.js/bindings/webcore/JSWorker.cpp
echo
echo "== Worker.cpp lines 130-155 (full context of locking changes) =="
sed -n '130,155p' src/bun.js/bindings/webcore/Worker.cpp
echo
echo "== Worker.cpp lines 270-285 (terminate path) =="
sed -n '270,285p' src/bun.js/bindings/webcore/Worker.cppRepository: oven-sh/bun
Length of output: 2139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Worker constructor/create full flow =="
rg -B10 -A15 'static ExceptionOr<RefPtr<Worker>> create' src/bun.js/bindings/webcore/Worker.cpp | head -60
echo
echo "== Check if m_implLock is reentrant =="
rg -B5 -A5 'class Worker' src/bun.js/bindings/webcore/Worker.h | grep -A30 'm_implLock'
echo
echo "== Verify: does WebWorker__updatePtr check for null? =="
rg -A10 'WebWorker__updatePtr' src/bun.js/web_worker.zig | head -30Repository: oven-sh/bun
Length of output: 96
🏁 Script executed:
#!/bin/bash
set -euo pipeflag
echo "== Worker.h - class definition and members =="
sed -n '50,130p' src/bun.js/bindings/webcore/Worker.h
echo
echo "== Check m_implLock type =="
rg -B2 -A2 'm_implLock' src/bun.js/bindings/webcore/Worker.h
echo
echo "== Worker::create implementation start =="
sed -n '160,210p' src/bun.js/bindings/webcore/Worker.cppRepository: oven-sh/bun
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Worker.h - class definition and members =="
sed -n '50,130p' src/bun.js/bindings/webcore/Worker.h
echo
echo "== Check m_implLock type =="
rg -B2 -A2 'm_implLock' src/bun.js/bindings/webcore/Worker.h
echo
echo "== Worker::create implementation start =="
sed -n '160,210p' src/bun.js/bindings/webcore/Worker.cppRepository: oven-sh/bun
Length of output: 5782
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WebWorker__updatePtr in web_worker.zig =="
rg -B5 -A15 'fn WebWorker__updatePtr' src/bun.js/web_worker.zig
echo
echo "== Verify: is there a null check in WebWorker__updatePtr? =="
rg -A20 'export fn WebWorker__updatePtr' src/bun.js/web_worker.zigRepository: oven-sh/bun
Length of output: 1186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WebWorker__dispatchExit call from Zig worker thread =="
rg -B10 -A5 'WebWorker__dispatchExit' src/bun.js/web_worker.zig
echo
echo "== Full context: when does clearZigImpl happen? =="
sed -n '475,490p' src/bun.js/bindings/webcore/Worker.cpp
echo
echo "== Check: is Worker::create and updatePtr in same synchronous call? =="
sed -n '348,360p' src/bun.js/bindings/webcore/JSWorker.cppRepository: oven-sh/bun
Length of output: 2492
Lock impl_ reads in updatePtr() and writes during initialization.
The new lock protections in setKeepAlive(), clearZigImpl(), and terminate() are sound, but updatePtr() still reads impl_ at line 149 without holding m_implLock, and the initial write at line 228 (Worker::create()) is also unprotected. Although the current usage pattern calls updatePtr() once synchronously during construction (before the worker thread can execute), if this function were called again—or if initialization timing changes—a worker thread running WebWorker__dispatchExit() could race with an unlocked read. The complete fix requires protecting both the read in updatePtr() and the write in Worker::create().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/webcore/Worker.cpp` around lines 141 - 145, updatePtr()
currently reads impl_ without holding m_implLock and Worker::create() assigns
impl_ without locking, which can race with the new protected writers
(setKeepAlive(), clearZigImpl(), terminate()); fix by taking the same lock
(m_implLock) when accessing impl_: add a Locker (or equivalent lock guard)
around the read in updatePtr() and around the initial write to impl_ inside
Worker::create() so all reads/writes of impl_ are consistently synchronized with
the existing setKeepAlive()/clearZigImpl()/terminate() locking.
There was a problem hiding this comment.
impl_ is written in Worker::create() and read in updatePtr() strictly before the worker thread is spawned (the thread is created inside WebWorker__updatePtr). No other thread can observe impl_ at those points, so no synchronization is needed there. The lock exists solely to order parent-thread terminate()/setKeepAlive() against the worker-thread clearZigImpl() in WebWorker__dispatchExit.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This touches threading/concurrency code in Worker.cpp; a human should verify the locking is sufficient to eliminate all races (e.g., confirm no other impl_ accesses exist outside the lock, and that the lock ordering is safe).
Extended reasoning...
Overview
The PR fixes two fuzzer-discovered crashes: a use-after-free in Worker::terminate() and an assertion in Reflect.construct(mock()). It modifies Worker.cpp/Worker.h to guard impl_ with a Lock, and adds a dedicated jsMockFunctionConstruct handler in JSMockFunction.cpp.
Security Risks
The Worker fix directly addresses memory safety: a freed Zig WebWorker struct was accessible via a raw impl_ pointer from the parent thread. The lock-and-null approach is the correct mitigation pattern, but threading bugs are easy to get subtly wrong (missed call sites, lock-ordering issues, ABA-style races).
Level of Scrutiny
This warrants human review. The concurrency fix in Worker.cpp is simple in structure but operates across two threads with a lock acquired in WebWorker__dispatchExit (worker thread) and in terminate()/setKeepAlive() (parent thread). The unguarded impl_ write at line 228 (worker->impl_ = impl) and the unguarded read in updatePtr() are plausibly safe (both happen before the worker thread starts), but a reviewer should confirm that invariant.
Other Factors
A bug was flagged about the jsMockFunctionConstruct fallback ignoring newTarget.prototype, which is a low-severity spec deviation. The added tests verify crash-prevention but do not cover the instanceof behaviour. Overall this is a good fix but the threading code merits a second pair of eyes.
… close listener before it can fire
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
2 similar comments
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/web/workers/worker-terminate-after-exit.test.ts`:
- Line 31: Remove the tautological assertion expect(true).toBe(true) from the
test; locate the expect(true).toBe(true) call in
worker-terminate-after-exit.test and delete it (or replace it with a meaningful
assertion related to the worker termination behavior), leaving the test to rely
on its existing thrown-error/crash/hang detection logic instead of a no-op
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9aa3a06f-8642-40c5-b016-56829db84992
📒 Files selected for processing (3)
src/bun.js/bindings/JSMockFunction.cpptest/js/bun/test/mock-construct.test.tstest/js/web/workers/worker-terminate-after-exit.test.ts
There was a problem hiding this comment.
LGTM — my previous concern about has been addressed in the updated commit.
Extended reasoning...
Overview
This PR fixes two crash-level bugs: a use-after-free in and an assertion failure in . Files modified are , , , and two new test files.
Security risks
No security-sensitive code paths are involved. Both fixes are defensive null/type guards that prevent crashes rather than bypassing any security boundary.
Level of scrutiny
Both fixes are well-scoped and mechanical. The Worker fix adds a to guard and clears it in before the Zig struct is freed. The mock fix adds a proper handler using to honor . A previous review iteration flagged both the close-listener race in the test (resolved by registering listeners in-loop) and the spec deviation (resolved by using ). Both are now fixed in commit 98a869b.
Other factors
Tests cover the crash-triggering inputs and spec-compliance for with a custom . The path reads without holding , but this is called synchronously during construction before the worker thread can race, so it is not a live data race.
| return JSValue::encode(jsUndefined()); | ||
| } | ||
|
|
||
| JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe)) | ||
| { | ||
| auto& vm = JSC::getVM(lexicalGlobalObject); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| JSC::EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| JSValue result = JSValue::decode(encodedResult); | ||
| if (result.isObject()) | ||
| return encodedResult; | ||
| JSObject* newTarget = asObject(callframe->newTarget()); | ||
| Structure* structure = JSC::InternalFunction::createSubclassStructure(lexicalGlobalObject, newTarget, lexicalGlobalObject->objectStructureForObjectConstructor()); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| return JSValue::encode(JSC::constructEmptyObject(vm, structure)); | ||
| } | ||
|
|
||
| void JSMockFunctionPrototype::finishCreation(JSC::VM& vm, JSC::JSGlobalObject* globalObject) | ||
| { |
There was a problem hiding this comment.
🟡 When jsMockFunctionConstruct delegates to jsMockFunctionCall, the thisValue captured inside jsMockFunctionCall is callframe->thisValue(), which for JSC InternalFunction native constructors equals newTarget() (the constructor function itself). This causes two observable deviations from Jest semantics: (1) mock().mockReturnThis() used as a constructor via Reflect.construct returns the mock function itself as the result (since result.isObject() is true for functions); (2) mock.mock.contexts records the mock constructor function rather than the constructed instance for every construct invocation. Neither issue is covered by the new mock-construct.test.ts suite.
Extended reasoning...
What the bug is and how it manifests
When jsMockFunctionConstruct delegates to jsMockFunctionCall by passing the same callframe, the line 'JSValue thisValue = callframe->thisValue()' inside jsMockFunctionCall captures newTarget() rather than a pre-created instance. This is an inherent property of JSC::InternalFunction native constructors, confirmed explicitly in the PR timeline by the author: 'For InternalFunction native constructors in JSC, callframe->thisValue() is newTarget() — there is no pre-created receiver.' Two separate misbehaviors follow.
Bug 1 — mockReturnThis() in construct context returns the mock constructor itself
When mockReturnThis() is active, jsMockFunctionCall executes 'case ReturnThis: return JSValue::encode(thisValue)', returning the mock constructor function (which equals newTarget()). Back in jsMockFunctionConstruct, the check 'if (result.isObject()) return encodedResult' passes — because functions are objects — so the constructor returns the mock function itself as the constructed result. Concretely: const m = mock().mockReturnThis(); const r = Reflect.construct(m, []); r === m // true. In Jest, mockReturnThis() in a constructor would return the pre-created this (the new instance), not the constructor function.
Bug 2 — mock.mock.contexts records newTarget instead of the constructed instance
In jsMockFunctionCall, the contexts array is populated with thisValue (which equals the mock constructor function). The actual constructed instance is only synthesized later, inside jsMockFunctionConstruct, after jsMockFunctionCall has already pushed to contexts. So mock.mock.contexts[0] === m for construct calls, instead of the newly allocated instance. Jest's mock.mock.contexts[i] for a constructor call holds the newly created this-object.
Why existing code does not prevent it
Before this PR, Reflect.construct(mock(), []) crashed (assertion in executeConstruct due to the native handler returning jsUndefined()), so neither behavior was observable. The PR correctly introduces jsMockFunctionConstruct to fix the crash, and correctly uses InternalFunction::createSubclassStructure for the primitive-return fallback path. However, the two paths that explicitly return thisValue (the ReturnThis implementation case) and record thisValue (contexts) remain affected, since thisValue equals newTarget — not an instance — in the construct callframe.
Addressing the refutations
The refutations correctly note that thisValue == newTarget is a pre-existing JSC limitation inherent to InternalFunction, and that a full fix would require architectural changes. However, the observable misbehaviors are new: before this PR, any Reflect.construct call on a mock would crash, making these bugs moot. After this PR enables construct, they become observable for the first time. The bugs exist in newly reachable code paths introduced by this PR.
Step-by-step proof for Bug 1
- const m = mock().mockReturnThis() sets impl.kind = ReturnThis.
- Reflect.construct(m, []) calls jsMockFunctionConstruct.
- jsMockFunctionCall is invoked with the same callframe; thisValue = callframe->thisValue() equals newTarget = m (the mock function).
- ReturnThis branch returns JSValue::encode(thisValue) — i.e., encodes the mock function m.
- Back in jsMockFunctionConstruct: result.isObject() is true (functions are objects), so return encodedResult — returns m as the constructed result.
- r === m is true, violating expected constructor semantics. The new tests do not exercise mockReturnThis() in construct context and do not assert on mock.mock.contexts values for construct calls.
There was a problem hiding this comment.
Both behaviors are pre-existing on the new operator path (which never crashed) — verified on the unpatched binary:
const m = mock().mockReturnThis();
new m() === m; // true (pre-existing)
m.mock.contexts[0] === m; // true (pre-existing)This PR's scope is fixing the crash when the construct path is entered via Interpreter::executeConstruct (e.g. Reflect.construct). It doesn't change what thisValue is during construction — that's inherent to InternalFunction and, as noted, would require a larger refactor (pre-creating the instance and threading it through jsMockFunctionCall). That's a separate behavioral improvement, not part of this crash fix.
1 participant
Fixes a use-after-poison crash found by Fuzzilli (fingerprint
Address:use-after-poison:bun-debug+0xaf04c65), plus a second crash hit by the same fuzzer input.Worker::terminate() use-after-free
The Zig
WebWorkerstruct is freed on the worker thread inexitAndDeinit(). The C++WebCore::Workerholds a rawvoid* impl_pointer to it which is never cleared. After the worker thread frees the struct,worker.terminate()/worker.ref()/worker.unref()on the parent thread callWebWorker__notifyNeedTermination(impl_)/WebWorker__setRef(impl_, ...), dereferencing freed memory.The existing
TerminatedFlagis only set from a task posted to the parent event loop, so there is a window where the Zig struct is already freed but the flag is not yet set — andterminate()wasn't checking the flag anyway.Fix: guard
impl_with aLock.WebWorker__dispatchExit(called from the worker thread immediately before the Zig struct is destroyed) now takes the lock and nullsimpl_.terminate()andsetKeepAlive()take the same lock and skip the call whenimpl_is null. TheupdatePtr()failure path also clearsimpl_since it frees the Zig struct on thread-spawn failure.Reflect.construct(mock(), [])assertionJSMockFunctionregisteredjsMockFunctionCallas both its call and construct native function. When there is no implementation (or the implementation returns a primitive), it returnsjsUndefined().JSC::Interpreter::executeConstructcallsasObject()on the result of a native constructor without checking, which asserts in debug and reads garbage in release.Fix: add a separate
jsMockFunctionConstructthat wrapsjsMockFunctionCalland substitutes an empty object when the result is not an object, matching normal[[Construct]]semantics. Regular calls are unaffected.