Skip to content

util.isError: propagate a throwing getPrototypeOf trap instead of crashing #37202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

util.isError: propagate a throwing getPrototypeOf trap instead of crashing #37202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions src/jsc/modules/NodeUtilTypesModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,26 +895,30 @@
auto scope = DECLARE_THROW_SCOPE(vm);
JSObject* object = value.toObject(globalObject);

// node util.isError relies on toString
// https://github.com/nodejs/node/blob/cf8c6994e0f764af02da4fa70bc5962142181bf3/doc/api/util.md#L2923
// util.isError is deprecated and removed in node 23
PropertySlot slot(object, PropertySlot::InternalMethodType::VMInquiry, &vm);
bool has = object->getPropertySlot(globalObject, vm.propertyNames->toStringTagSymbol, slot);
scope.assertNoException();
if (has) {
if (slot.isValue()) {
JSValue value = slot.getValue(globalObject, vm.propertyNames->toStringTagSymbol);
if (value.isString()) {
String tag = asString(value)->value(globalObject);
CLEAR_IF_EXCEPTION(scope);
if (tag == "Error"_s)
return JSValue::encode(jsBoolean(true));
{
PropertySlot slot(object, PropertySlot::InternalMethodType::VMInquiry, &vm);
bool has = object->getPropertySlot(globalObject, vm.propertyNames->toStringTagSymbol, slot);
scope.assertNoException();
if (has) {
if (slot.isValue()) {
JSValue value = slot.getValue(globalObject, vm.propertyNames->toStringTagSymbol);
if (value.isString()) {
String tag = asString(value)->value(globalObject);
CLEAR_IF_EXCEPTION(scope);
if (tag == "Error"_s)
return JSValue::encode(jsBoolean(true));
}
}
}
// The VMInquiry slot disallows VM entry while alive; the Proxy trap below needs it dead.
}

JSValue proto = object->getPrototype(globalObject);
RETURN_IF_EXCEPTION(scope, {});
if (proto.isCell() && (proto.inherits<JSC::ErrorInstance>() || proto.asCell()->type() == ErrorInstanceType || proto.inherits<JSC::ErrorPrototype>()))

Check failure on line 921 in src/jsc/modules/NodeUtilTypesModule.cpp

View check run for this annotation

Claude / Claude Code Review

Rebase/squash dropped 7 of 9 files: PR's titular fix and most regression tests are absent 

The rebase/squash dropped 7 of the 9 files this PR describes: the head commit (09f65a0b) touches only `NodeUtilTypesModule.cpp` + `util.test.js`, but the title, description, evidence block ("9 files touched", four passing console-log tests at lines 154–240), and every resolved review thread describe the `bindings.cpp` / `ZigGlobalObject.cpp` / `napi.cpp` / `ProcessObjectInternals.ts` / `console-log.test.ts` changes that are absent from the diff and not on main. Merging as-is closes fuzzer finger
Comment on lines 898 to 921

@claude claude Bot Aug 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The rebase/squash dropped 7 of the 9 files this PR describes: the head commit (09f65a0) touches only NodeUtilTypesModule.cpp + util.test.js, but the title, description, evidence block ("9 files touched", four passing console-log tests at lines 154–240), and every resolved review thread describe the bindings.cpp / ZigGlobalObject.cpp / napi.cpp / ProcessObjectInternals.ts / console-log.test.ts changes that are absent from the diff and not on main. Merging as-is closes fuzzer fingerprint 79443a4a1351567b without fixing it (globalThis.Symbol = NaN; console.log(Bun) still aborts at bindings.cpp:5646–5649), and CI won't catch it because the console-log regression tests were dropped with the fix. Please either restore the 7 dropped files or retitle/redescribe to the util.isError-only scope.

Extended reasoning...

What the finding is

The PR title is "Fix stale pending exceptions in console.log property iteration" and the description opens with "Fixes a fuzzer-found abort (fingerprint 79443a4a1351567b)", listing a 9-file diff-hotspot with 224 insertions. The evidence block shows four console-log.test.ts tests passing at lines 154–240 and cites a Rebase note claiming "Everything else applied cleanly and the suites above were rerun on the new base."

But git show --stat 09f65a0b (the squashed head, and the entirety of what get_pr_diff returns) is:

util.isError: propagate a throwing getPrototypeOf trap instead of crashing
 src/jsc/modules/NodeUtilTypesModule.cpp | 26 ++++++++++++++----------
 test/js/node/util/util.test.js          | 36 ++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 12 deletions(-)

The head commit's own message describes only the util.isError fix. Seven of the nine described files are absent, including the titular fix and its regression tests.

Step-by-step proof: each described change is absent from the diff and not on main

Verified against HEAD (the PR checkout) and git log -- <file> on main:

  1. bindings.cpp — the PR's titular fix. Lines 5646–5649 still read:

    if (!object->getPropertySlot(globalObject, property, slot))
    continue;
// Ignore exceptions from "Get" proxy traps.
CLEAR_IF_EXCEPTION(scope);

    The not-found branch continues past the CLEAR_IF_EXCEPTION — exactly the bug the title says is fixed. The reproducer globalThis.Symbol = NaN; console.log(Bun); still aborts debug builds.

  2. test/js/web/console/console-log.test.ts. wc -l reports 152 lines; the file ends at the SharedArrayBuffer test. The four spawned regression tests the evidence block shows as (pass) at lines 154–240 ("console.log(Bun) survives lazy properties whose initializer throws", "stops the prototype walk when a getPrototypeOf trap throws", the two stylize tests) do not exist. The evidence block is showing pass output for tests that aren't in the tree.

  3. ZigGlobalObject.cpp. Lines 2310/2313/2337/2345/2350 still RETURN_IF_EXCEPTION(scope, ) without init.set, and line 2315 still uncheckedDowncast<JSFunction>(prop). The identity-fallback / dynamicDowncast / assertNoExceptionExceptTermination hardening — the subject of three resolved review threads on this PR — is absent.

  4. napi.cpp. Lines 2079–2088 still read while (!owner->getOwnPropertyDescriptor(...)) { JSObject* proto = owner->getPrototype(globalObject).getObject(); ... } else { owner->getOwnPropertyDescriptor(...); } with no NAPI_RETURN_IF_EXCEPTION after either getOwnPropertyDescriptor call or after getPrototype — the subject of two resolved review threads.

  5. ProcessObjectInternals.ts:453. Still reads Bun.inspect.custom inside windowsEnv. The Rebase note says the only conflict was here and was resolved by "appending the inspect symbol argument after" resetForDelete — but no inspectCustom parameter exists. The very conflict the note claims to have resolved lost the fix.

  6. BunObject.cpp / JSEnvironmentVariableMap.cpp. git log on both shows only unrelated commits; the described removals/additions are not present.

Why this is not covered by anything else

  • These fixes did not land separately on main: git log on each of the 7 dropped files shows only unrelated commits (Drop the DOMWrapperWorld wrapper HashMap #39334, bake: name both flags at every insert_stale_extra call #39171).
  • CI will not catch the missing titular fix because the console-log.test.ts regression tests that would have caught it were dropped in the same squash.
  • The description's Rebase note explicitly asserts the opposite of what happened ("Everything else applied cleanly and the suites above were rerun on the new base"), so a reviewer approving on the description would close fingerprint 79443a4a1351567b while the crash remains.

Impact

Merging as-is:

  • Closes a fuzzer fingerprint for a debug abort / release output-truncation bug that is not fixed.
  • Ships a PR whose title, description, evidence block, and every resolved review thread describe changes that are not in the diff — CLAUDE.md: "Be humble & honest — NEVER overstate what you got done"; REVIEW.md: "Include every reproduction from the linked issue" and "Every behavioral change ships an automated test in the same PR."
  • Leaves the LazyProperty contract violation (return without init.set), the napi prototype-climb null-cell dereference, and the Windows env mid-reification abort — each raised and marked resolved in review — unfixed with their threads closed.

What is in the diff (the util.isError VMInquiry-scope fix + RETURN_IF_EXCEPTION after getPrototype, with a spawned regression test) is coherent and correct on its own.

How to fix

Two valid paths:

  • Restore the dropped 7 files from the pre-squash branch (bindings.cpp, ZigGlobalObject.cpp, napi.cpp, BunObject.cpp, JSEnvironmentVariableMap.cpp, ProcessObjectInternals.ts, console-log.test.ts) and re-run the two suites the evidence block references so it reflects the actual head; or
  • Retitle to "util.isError: propagate a throwing getPrototypeOf trap instead of crashing" (matching the head commit), replace the description with the util.isError-only scope, drop the fuzzer-fingerprint "Fixes" claim and the stale evidence block, and open a follow-up for the console.log / LazyProperty / napi / Windows-env fixes.

Why normal, not nit

The default "PR-description mismatch is nit" rule covers stale/imprecise descriptions where the code is what was intended. Here 7 of 9 files — including the fix the PR is named after — went missing during a rebase the description explicitly claims preserved them, the evidence block shows pass output for tests that don't exist, and merging spuriously closes a tracked crash with no CI guard. That is a concrete failure the author must resolve before merge, not a wording tweak.

return JSValue::encode(jsBoolean(true));
}

Expand Down
36 changes: 35 additions & 1 deletion test/js/node/util/util.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

import assert from "assert";
import { describe, expect, it } from "bun:test";
import "harness";
import { bunEnv, bunExe } from "harness";
import util from "util";
// const context = require('vm').runInNewContext; // TODO: Use a vm polyfill

Expand Down Expand Up @@ -152,6 +152,40 @@ describe("util", () => {
let err8 = new Error3();
strictEqual(util.isError(err8), true);
});

// Spawned: these inputs crashed the whole process before the fix (the
// VMInquiry slot outlived the toStringTag check, so any getPrototypeOf
// trap aborted), and a regression must not take the file down with it.
it.concurrent("handles Proxy getPrototypeOf traps", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`const util = require("node:util");
const expected = new Error("nope");
let caught;
try {
util.isError(new Proxy({}, { getPrototypeOf() { throw expected; } }));
} catch (error) {
caught = error;
}
console.log("identity:" + (caught === expected));
console.log("proto-error:" + util.isError(new Proxy({}, { getPrototypeOf: () => Error.prototype })));
console.log("proto-null:" + util.isError(new Proxy({}, { getPrototypeOf: () => null })));`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect({ stdout, stderr: stderr.trim(), exitCode }).toEqual({
stdout: "identity:true\nproto-error:true\nproto-null:false\n",
stderr: "",
exitCode: 0,
});
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});

describe("isObject", () => {
Expand Down
Loading