Skip to content

Bun.mmap: throw on non-object options instead of asserting#30404

Merged
alii merged 1 commit into
mainfrom
farm/f6cb7a25/mmap-non-object-options
May 8, 2026
Merged

Bun.mmap: throw on non-object options instead of asserting#30404
alii merged 1 commit into
mainfrom
farm/f6cb7a25/mmap-non-object-options

Conversation

@robobun

@robobun robobun commented May 8, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Bun.mmap(path, 256) (or any non-object second argument) hit a debug assertion in JSValue.get() because the options value was passed straight to getBooleanLoose without first checking that it is an object.

Now non-object, non-nullish values throw ERR_INVALID_ARG_TYPE, and undefined/null are treated the same as omitting the options argument.

panic(main thread): reached unreachable code
bun.debugAssert
jsc.JSValue.JSValue.get                 src/jsc/JSValue.zig:1534
jsc.JSValue.JSValue.getBooleanLoose     src/jsc/JSValue.zig:1867
runtime.api.BunObject.mmapFile          src/runtime/api/BunObject.zig:1219

How did you verify your code works?

Added a test to test/js/bun/util/mmap.test.js covering number/string/boolean (throw) and undefined/null (no throw).

Found by Fuzzilli (fingerprint b1832bde6df73226).

Bun.mmap(path, 256) hit a debug assertion in JSValue.get() because the
options argument was passed through to getBooleanLoose without checking
that it is an object. Validate the argument and throw ERR_INVALID_ARG_TYPE
for non-object values; undefined and null are treated the same as omitted.
@github-actions github-actions Bot added the claude label May 8, 2026
@robobun

robobun commented May 8, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 9:12 AM PT - May 8th, 2026

@robobun, your commit 6418e2c has 3 failures in Build #52834 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 30404

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

bun-30404 --bun

@coderabbitai

coderabbitai Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack
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: 9815f33f-afb4-495b-bcbd-5593be8a6421

📥 Commits

Reviewing files that changed from the base of the PR and between d5945cf and 6418e2c.

📒 Files selected for processing (2)
  • src/runtime/api/BunObject.zig
  • test/js/bun/util/mmap.test.js

Walkthrough

This PR adds input validation to the Bun.mmap() function. The implementation refactors the options parsing in mmapFile to explicitly check that the opts parameter is either null, undefined, or an object, rejecting any other type with a descriptive error. A corresponding test case verifies the validation behavior.

Changes

Bun.mmap options validation

Layer / File(s) Summary
Options validation in mmapFile
src/runtime/api/BunObject.zig
The mmapFile function's options parsing is refactored to validate that opts is an object (or null/undefined), explicitly rejecting non-object values with an "Expected options to be an object" error. Existing option key handling (shared, sync, size, offset) is preserved within the validation check.
Options validation test
test/js/bun/util/mmap.test.js
A test case is added to verify that Bun.mmap() throws when passed a non-object option value (number, string, or boolean) and does not throw when options is undefined or null.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: replacing debug assertion behavior with explicit error throwing for non-object options in Bun.mmap.
Description check ✅ Passed The description covers both required template sections with clear explanations of the bug fix and verification approach through added tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

@claude claude Bot left a comment

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.

LGTM — straightforward input validation fix following the established pattern in this file.

Extended reasoning...

Overview

This PR fixes a fuzzer-found debug assertion in Bun.mmap() when a non-object value is passed as the second (options) argument. The fix wraps the existing options-parsing block in src/runtime/api/BunObject.zig with an opts.isObject() check, throws ERR_INVALID_ARG_TYPE for non-object/non-nullish values, and treats undefined/null as no options. A new test in test/js/bun/util/mmap.test.js covers number/string/boolean (throw) and undefined/null (no throw).

Security risks

None. This is purely input validation that converts a debug-build crash into a proper JS exception. No auth, crypto, permissions, or data exposure paths are touched. The actual mmap syscall path and its arguments are unchanged.

Level of scrutiny

Low. The diff is almost entirely re-indentation — the only behavioral additions are the isObject() guard and the else if (!opts.isUndefinedOrNull()) throw branch. This exact pattern is already used in the same file by which(), JSZlib.getOptions(), and JSZstd.getOptions(), so there's no novel design here.

Other factors

The bug hunting system found no issues. The change is self-contained to one function, has direct test coverage for all new branches, and was found via Fuzzilli (fingerprint referenced in the description). No outstanding reviewer comments.

@robobun

robobun commented May 8, 2026

Copy link
Copy Markdown
Collaborator Author

All 4 failing shards are pre-existing flakes unrelated to this change (which only touches Bun.mmap argument validation):

Test Platform Also failing in recent builds
test/js/bun/test/parallel/test-http-should-emit-close-when-connection-is-aborted.ts (timeout) win x64, win x64-baseline #52833 #52832 #52829 #52828 #52827 #52824 #52822 #52821 #52818 #52816
test/js/bun/s3/s3-storage-class.test.ts (S3Error: UnknownError) darwin aarch64 #52833 #52828 #52827 #52824
test/js/third_party/@duckdb/node-api/duckdb.test.ts (ASAN SEGV in __memcpy_avx512 inside uninstrumented duckdb .node) debian x64-asan

The mmap.test.ts changes pass on every platform — no mmap appears in any failure annotation.

@alii alii merged commit 3457c1e into main May 8, 2026
78 of 79 checks passed
@alii alii deleted the farm/f6cb7a25/mmap-non-object-options branch May 8, 2026 20:26
springmin pushed a commit to springmin/bun that referenced this pull request May 10, 2026
…0404)

## What does this PR do?

`Bun.mmap(path, 256)` (or any non-object second argument) hit a debug
assertion in `JSValue.get()` because the options value was passed
straight to `getBooleanLoose` without first checking that it is an
object.

Now non-object, non-nullish values throw `ERR_INVALID_ARG_TYPE`, and
`undefined`/`null` are treated the same as omitting the options
argument.

```
panic(main thread): reached unreachable code
bun.debugAssert
jsc.JSValue.JSValue.get                 src/jsc/JSValue.zig:1534
jsc.JSValue.JSValue.getBooleanLoose     src/jsc/JSValue.zig:1867
runtime.api.BunObject.mmapFile          src/runtime/api/BunObject.zig:1219
```

## How did you verify your code works?

Added a test to `test/js/bun/util/mmap.test.js` covering
number/string/boolean (throw) and undefined/null (no throw).

Found by Fuzzilli (fingerprint `b1832bde6df73226`).

---

Co-authored-by: Alistair Smith <hi@alistair.sh>
Signed-off-by: Sisyphus <sisyphus@ohos-bun.dev>
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.

2 participants