Repository navigation
Conversation
…FO writer still waits The three callers that join an array of parts get one test: the bytes of a file part, and the synchronous ENOENT for a part that is missing. The FIFO test waits until the writer runs, and asserts after the constructor that the writer has not exited. A constructor that opens the FIFO now fails that assertion and does not run into the timeout.
A part that is not a regular file throws a TypeError, and that included the null device. main gave an empty part for it, and Node does too. The null device has no bytes and a read of it cannot block, so it is an empty part again. The part is the null device when it is a character device with the st_rdev of the platform's null device. That needs one more stat, and only for a character device. A path is still not opened for it. Each other device, a pipe and a socket still throw.
|
Status: ready for review. This PR is stacked on #33600 (base Reproduce on the head of #33600: Proof: |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the is_ok_and -> match rewrite of the pre-open stat check: a stat error still falls through to open(2) exactly as before, and error polarity is unchanged. The early Ok(()) for a null-device part pushes nothing to the joiner, which is the same shape as the existing cap == 0 empty-part return, and both push_file_part call sites in from_js_without_defer_gc go through the shared helper, so no sibling site is missed.
Extended reasoning...
The change is confined to push_file_part in src/runtime/webcore/Blob.rs (a new is_null_device helper and a closure-to-Result rewrite of the refusal path) plus new tests in test/js/web/fetch/blob.test.ts; it touches no auth, crypto, or injection surface. Three inline findings were posted (a per-call re-stat of /dev/null that fails in sandboxes without /dev, a test that does not fail under USE_SYSTEM_BUN=1, and a FIFO assertion that can pass while the FIFO was opened), so a human look is already signalled; this note only records the refactor and empty-part paths that were checked and found behavior-preserving.
On Linux the null device is major 1, minor 3, so no stat of /dev/null is needed. A process that has no /dev in its root, with an fd on the null device that it got from its parent, now also gets an empty part. The tests for the null device now have a regular file as one more part, so they fail on a build that drops each file part.
There was a problem hiding this comment.
I re-reviewed the new commit (dd1127f) and found no bugs; it addresses the Linux device-number and test-liveness points from my earlier pass. A human look is still worthwhile because the is_null_device cfg branches for Windows and macOS cannot be type-checked or exercised from this Linux machine, and the PR is stacked on an unmerged base.
What was reviewed:
is_null_device: Linuxmakedev(1, 3)matches the kernel encodinglinux_syscall.rswrites intost_rdev(0x103); the non-Linux branch compares against a runtime stat of/dev/null/\\.\NUL, whose libuv-derivedst_rdevisFILE_DEVICE_NULL << 16on Windows.- Both
push_file_partsites (pre-open path stat, post-open fstat) route through the sameno_bytes_or_refusedclosure; the stat-error arm still falls through toopenso the path-qualified error is unchanged, and the directory path still reaches EISDIR. - New tests now include a regular-file sibling so a build that drops file parts fails them; the
/dev/zerorefusal runs in a child so a too-wide rule cannot hang the test process.
Extended reasoning...
The diff touches src/runtime/webcore/Blob.rs (a new is_null_device helper with linux/android, windows and other-unix cfg branches, wired into the two non-regular-file checks of the cold push_file_part) and test/js/web/fetch/blob.test.ts (three new or changed tests). No security-sensitive surface: it widens one refusal to return an empty part only for a character device whose st_rdev matches the platform null device. The bug hunt ran dry with no findings, and the latest commit addressed two of three prior inline notes; the third (the FIFO writer exitCode snapshot) remains an open optional thread. I could not run cargo check here, so the Windows and macOS cfg branches are reviewed by reading only, and the PR is stacked on #33600, which is why this is a defer rather than an approve.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
Follow-up to #33600, stacked on its branch. Related to #25851.
Problem
new Blob([Bun.file("/dev/null"), "x"])throwsTypeError: Blob parts backed by a pipe, socket or device cannot be read synchronously; await .bytes() or .arrayBuffer() first. main and Node v26.3.0 give a Blob of size 1.push_file_part(src/runtime/webcore/Blob.rs) refuses each part that is not a regular file. That includes the null device.Fix
st_rdevof/dev/nullor\\.\NULelsewhere. This covers a path, an fd and a symbolic link.test/js/web/fetch/blob.test.tson Linux and on a Windows x64 build. The 2 new tests fail on the head of Blob: read file-backed parts in multi-part new Blob([...]) #33600 and on bun 1.4.2.Background
st_rdevis thestatfield that names the device behind a device file.Downsides
statelsewhere. No other part reaches that code./dev/zeroand a/dev/stdinthat is a pipe still throw. Node gives an empty part for both.bun+0 bytes,bun-profile-3,440 bytes.Notes
Results (release builds, Linux x64)
Node takes the length of a part from the stat size, so
/dev/zeroand a pipe are an empty part there. bun throws for them so that no bytes are dropped without a signal.new Blob(["x", part])Bun.file("/dev/null")TypeErrorBun.file(fd)on/dev/nullTypeError/dev/nullTypeErrorBun.stdinwith< /dev/nullTypeErrorBun.file("/dev/zero")TypeErrorTypeError/dev/full,/dev/urandom,/dev/ttyTypeErrorTypeErrorBun.stdinas a pipeTypeErrorTypeErrormain gives size 1 in each row because it drops each file-backed part. That is the bug that #33600 corrects. For the null device the result was right by accident.
Windows (debug build of main plus this branch)
Bun.file("/dev/null"),NUL,nul,\\.\NUL,os.devNulland an fd onos.devNullare an empty part.CON,\\.\CONandCONIN$throw theTypeError.Bun.stdinas a pipe throws theTypeError. libuv givesst_rdev = FILE_DEVICE_NULL << 16for the null device and other values for a console and a pipe.Linux
The kernel fixes the null device at major 1, minor 3 (
Documentation/admin-guide/devices.txt). So the check needs no path. A process that has no/devin its root, with an fd on the null device from its parent, gets an empty part too. I could not run that case:chrootis not permitted in the build container.Tests
bun bd test test/js/web/fetch/blob.test.ts(debug, ASAN): 122 pass, 1 skip (theEACCEStest does not run as root). As uid 65534 the block gives 13 pass.src/of the head of Blob: read file-backed parts in multi-part new Blob([...]) #33600: "a null device part is an empty part" and "only the null device is an empty part" fail with theTypeError. The other tests pass.bun scripts/rust-check-all.tsforaarch64-apple-darwin,x86_64-pc-windows-msvc,aarch64-linux-androidandx86_64-unknown-freebsd: 4 ok.cargo clippy -p bun_runtimehas no finding inBlob.rs./dev/zerocase runs in a child process. If the rule became too wide, a read of/dev/zeroin the test process would not return.In-memory parts
The two call sites in
from_js_without_defer_gcare not changed. The new code is inpush_file_part, which is#[cold], and inis_null_device.When #33600 merges
I will rebase this branch on main and set the base of this PR to main.
no test proof · iteration 11 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/blob.test.ts