Skip to content

Fix RELEASE_ASSERT in LazyProperty when node:util fails to load during Bun.inspect#29235

Open
robobun wants to merge 4 commits intomainfrom
farm/d130353f/fix-util-inspect-lazy-init-assert
Open

Fix RELEASE_ASSERT in LazyProperty when node:util fails to load during Bun.inspect#29235
robobun wants to merge 4 commits intomainfrom
farm/d130353f/fix-util-inspect-lazy-init-assert

Conversation

@robobun
Copy link
Copy Markdown
Collaborator

@robobun robobun commented Apr 12, 2026

Fuzzilli found a crash with fingerprint LazyPropertyInlines.h(104):

ASSERTION FAILED: !(initializer.property.m_pointer & lazyTag)
LazyPropertyInlines.h(104) : static ElementType *JSC::LazyProperty<JSC::JSGlobalObject, JSC::JSFunction>::callFunc(...)

Root cause

Bun.inspect() on an object with a [Symbol.for('nodejs.util.inspect.custom')] method triggers lazy initialization of m_utilInspectFunction, which loads node:util. If evaluating node:util (or one of its transitive dependencies) throws, the initializer returned early via RETURN_IF_EXCEPTION(scope, ) without calling init.set(). LazyProperty::callFunc requires the lazy tag to be cleared before it returns, so it hit the RELEASE_ASSERT.

The same issue existed in m_utilInspectStylizeColorFunction.

Repro

Object.defineProperty(Array.prototype, 'forEach', { get() { throw new Error('poisoned'); } });
Bun.inspect({ [Symbol.for('nodejs.util.inspect.custom')]() { return 'x'; } });

Before: aborts with RELEASE_ASSERT.
After: throws a catchable JS error.

Fix

On exception, call init.property.setMayBeNull(init.vm, init.owner, nullptr) so the tag bits are cleared and the exception propagates to the caller. This matches the pattern already used in JSX509Certificate. Callers of utilInspectFunction() / utilInspectStylizeColorFunction() are updated to handle nullptr on subsequent calls after a failed load (pass jsUndefined() to the custom inspect function, and fall back to a plain string for BroadcastChannel's custom inspect).

…g Bun.inspect

The m_utilInspectFunction and m_utilInspectStylizeColorFunction lazy
property initializers would return early via RETURN_IF_EXCEPTION without
calling init.set() when loading node:util threw an exception. LazyProperty
requires init.set() to be called to clear the lazyTag bit, otherwise
RELEASE_ASSERT(!(m_pointer & lazyTag)) fires in callFunc.

On exception, set the property to null via setMayBeNull so the tag bits
are cleared and the exception propagates to the caller. Callers of
utilInspectFunction() are updated to handle the null case gracefully.
@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented Apr 12, 2026

Updated 10:18 PM PT - Apr 12th, 2026

@robobun, your commit b857ee6 has some failures in Build #45398 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 29235

That installs a local version of the PR into your bun-29235 executable, so you can run:

bun-29235 --bun

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 12, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 726c88d4-c613-45d7-833a-7749fbce3c7e

📥 Commits

Reviewing files that changed from the base of the PR and between 5df36ba and b857ee6.

📒 Files selected for processing (1)
  • test/js/bun/util/inspect-custom-lazy-init-exception.test.ts

Walkthrough

Adjusts custom-inspect callsite to pass undefined when options or inspectFn are null, propagates exceptions from lazy initialization of util.inspect-related properties, avoids calling inspect when missing (including BroadcastChannel), and adds tests exercising Bun.inspect when node:util load paths are poisoned.

Changes

Cohort / File(s) Summary
Custom Inspect Callsite
src/bun.js/bindings/UtilInspect.cpp
Now appends JSValue(options)/JSValue(inspectFn) only when non-null; otherwise appends jsUndefined() to avoid passing raw null pointers into the custom-inspect call.
Global Object Lazy Init
src/bun.js/bindings/ZigGlobalObject.cpp
Lazy initializer lambdas for util.inspect-related properties now propagate exceptions via init.property.setMayBeNull(...), cache/early-return when the inspect function is absent, and avoid calling args.append on a null function.
BroadcastChannel inspect()
src/bun.js/bindings/webcore/JSBroadcastChannel.cpp
Retrieves utilInspectFunction() earlier, returns a "BroadcastChannel" string when utilInspect is missing, and prevents building/calling arguments on a null inspect function.
REPL format fallback
src/bun.js/bindings/bindings.cpp
Bun__REPL__formatValue now checks for exception after bunGlobal->utilInspectFunction() and returns early (jsUndefined()) if an exception occurred.
Tests (new)
test/js/bun/util/inspect-custom-lazy-init-exception.test.ts
Adds tests that spawn bun -e scripts which poison Array.prototype.forEach to simulate node:util load failures; exercise repeated Bun.inspect calls and BroadcastChannel inspection, asserting exact stdout and exit status.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main issue being fixed: a RELEASE_ASSERT error in LazyProperty that occurs when node:util fails to load during Bun.inspect operations.
Description check ✅ Passed The PR description comprehensively covers the required template sections: it explains what the PR does (fixing a RELEASE_ASSERT in LazyProperty), provides root cause analysis, includes a minimal reproduction case, and demonstrates verification (before/after behavior).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Fix lazy property assertion failure in utilInspect initializers #28336 - Fixes the same LazyProperty assertion failure in the same two utilInspect initLater lambdas in ZigGlobalObject.cpp (uses DECLARE_TOP_EXCEPTION_SCOPE approach instead of setMayBeNull)

🤖 Generated with Claude Code

@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented Apr 12, 2026

Re: duplicate detection of #28336 — that PR replaces RETURN_IF_EXCEPTION with assertNoExceptionExceptTermination(), assuming only termination exceptions can occur inside the lazy initializer (which are deferred by DeferTerminationForAWhile).

That assumption doesn't hold: requireId(NodeUtil) evaluates module top-level JS, which can throw a regular (non-termination) exception. The repro in this PR demonstrates this — poisoning Array.prototype.forEach causes internal/util/inspect top-level eval to throw. Under #28336's approach that hits assertNoExceptionExceptTermination() in debug, or RELEASE_ASSERT(nodeUtilValue.isObject()) in release.

This PR instead sets the property to nullptr via setMayBeNull on exception (clearing the lazy tag) and lets the exception propagate to the caller, then hardens the callers against the null case.

- m_utilInspectStylizeColorFunction: check for null return from
  utilInspectFunction() (no exception when previously nulled) before
  appending to args
- JSBroadcastChannel: move utilInspect null check before options.toObject()
  so the fallback string is returned when options is undefined
- Bun__REPL__formatValue: add RETURN_IF_EXCEPTION after utilInspectFunction()
- Add test for the colors:false-then-colors:true ordering
- Add 30s timeouts for slow debug+ASAN subprocess startup
Copy link
Copy Markdown
Contributor

@claude claude bot left a comment

Choose a reason for hiding this comment

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

LGTM — all previously raised issues have been addressed.

Extended reasoning...

Overview

This PR fixes a RELEASE_ASSERT crash triggered when Bun.inspect() is called on an object with a nodejs.util.inspect.custom symbol while node:util fails to load (e.g., due to a poisoned Array.prototype.forEach). Changes touch UtilInspect.cpp, ZigGlobalObject.cpp, bindings.cpp, JSBroadcastChannel.cpp, and a new test file.

Security risks

None. This is purely a crash-recovery fix for an edge case where module loading fails; it converts a hard abort into a catchable JS exception.

Level of scrutiny

The fix is targeted and follows a well-established pattern in the codebase (JSX509Certificate's use of setMayBeNull). The core changes are mechanical: replace empty RETURN_IF_EXCEPTION bodies with setMayBeNull calls, add null guards at call sites, and reorder an existing guard to before a toObject() call that could throw on undefined.

Prior review outcome

All four issues raised in prior inline comments were resolved: the missing null-check for utilInspectFunction() in the stylize-color initializer, the missing RETURN_IF_EXCEPTION in Bun__REPL__formatValue, the misordered null check in JSBroadcastChannel, and the explicit test timeouts. The author also gave a clear, valid justification for not asserting stderr (ASAN startup output on debug builds).

Other factors

Two new tests cover the repro scenario across repeated calls and with BroadcastChannel. The second test's expected output (nocolors caught followed by colors ok) correctly reflects the lazy-init-once semantics: after the first failure clears both lazy properties to null, subsequent calls return gracefully with undefined as the inspect function argument.

@robobun
Copy link
Copy Markdown
Collaborator Author

robobun commented Apr 13, 2026

CI status: all 59 jobs that ran passed (including debian-13-x64-asan-test-bun). The only failures are darwin-{13,14}-aarch64-test-bun which are marked Expired — they timed out waiting for a macOS ARM64 agent and never ran. Same expiry is affecting other open PRs (e.g. #29239), so this is fleet-wide agent unavailability rather than anything in this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant