Skip to content

socket: refuse a file- or S3-backed Blob in write() and end() - #46833

Open
robobun wants to merge 1 commit into
mainfrom
robobun/3a525a1e/socket-refuse-lazy-blob
Open

robobun wants to merge 1 commit into
mainfrom
robobun/3a525a1e/socket-refuse-lazy-blob

Conversation

@robobun

@robobun robobun commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Related to #43931

Problem

  • socket.write(s3file) returns 0 on a Bun.connect / Bun.listen socket. socket.end(s3file) returns 0 and closes the connection. Nothing is sent, and nothing throws.
  • The guard in NewSocket::write_or_end (src/runtime/socket/socket_body.rs:2894) tests needs_to_read_file(), true for a file store only. buffer.slice() is empty for an S3 store.

Fix

Background

Downsides

  • Behavior change: on Bun 1.4.4 these calls return 0, or -1 on a half-closed socket. Now they throw. No user reported the bug.
  • write(x, byteOffset, byteLength) of a string or buffer costs 8 more instructions (787 to 795). write(string) costs 18 fewer.
  • Not fixed: a printed Request with an S3 body still writes 0 (s3: leave the size of an S3 blob unknown in resolve_size #46653). A refused Request or Response is spent.
Notes

Origin. A fuzz run found it. #43931 described the class and was closed as not a user report. #46673 lists the other sites. #14516 wrote the guard before the S3 store existed (#15740). No S3 request is made before or after.

Repro. A loopback Bun.serve stands in for S3.

const log = [];
const srv = Bun.serve({ port: 0, hostname: "127.0.0.1", fetch(q) { log.push(q.method); return new Response("0123456789"); } });
const s3 = new Bun.S3Client({ endpoint: srv.url.href, accessKeyId: "k", secretAccessKey: "s", bucket: "b" });
let got = 0;
const peer = Bun.listen({ hostname: "127.0.0.1", port: 0, socket: { data(_, d) { got += d.length; } } });
Bun.connect({ hostname: "127.0.0.1", port: peer.port, socket: { data() {}, open(s) {
  const t = f => { try { return "returned " + f(); } catch (e) { return "throws " + e.message; } };
  console.log("write(s3file):", t(() => s.write(s3.file("o")))); // 1.4.4 and main: returned 0
  console.log("end(s3file)  :", t(() => s.end(s3.file("o"))));   // 1.4.4 and main: returned 0, and the peer sees FIN
  setTimeout(() => { console.log("peer read", got, "bytes | S3 requests:", log.length); process.exit(0); }, 300);
} } });

What moves. The left column is the 1.4.4 release binary (38afee6) and main 6292c65: each row gave the same result on both. There, write(Bun.file(path)) throws File blob not supported yet in this function. "Lazy" is an S3File, a slice of one, new Blob([s3file]), new Response([s3file]), or a Request with such a body.

Call Bun 1.4.4 and main This PR
write(lazy) returns 0 throws
end(lazy) returns 0, the peer sees FIN throws, the socket stays open
write(s3file, 0, 4) ERR_OUT_OF_RANGE with a maximum of 0 throws the refusal
write(lazy) on a half-closed socket returns -1 throws
write(Bun.file(path)), and a Request with that body throws the old text throws the new text
write(s3file.slice(0, 0)) returns 0 throws, like Bun.file(path).slice(0, 0)
a string, a buffer, an in-memory Blob, a buffered Request or Response sent the same
  • The refusal comes where the type error for write(123) comes: before the socket state is read. A detached socket still returns -1.
  • One exception: a Request or Response whose lazy body has a resolved size of 0 still writes 0, because to_blob() drops its store. That is a printed S3 body (s3: leave the size of an S3 blob unknown in resolve_size #46653), an empty slice of an S3 file, or an empty file whose size was read. For the last two, 0 bytes is the right result.
  • new Response(s3file) (no array) has no buffered body. It throws Only buffered Request/Response bodies are supported for now. before and after.
  • node:net is not affected. Its Writable refuses a Blob chunk with ERR_INVALID_ARG_TYPE, as Node does.

Tests. Debug (ASAN) build of this branch, Linux x64.

  • "refuses a file- or S3-backed Blob, sends nothing and stays open", over tcp and tls: fails on the 1.4.4 release binary and on main (release build of 6292c65), passes here. It checks the error of write and end for six inputs and for an explicit range, a flush() and a later write on the same socket, and that the S3 stand-in saw no request before a sentinel read.
  • "sends an in-memory Blob and the buffered body of a Request or Response, in order", over tcp and tls: the control. It passes on 1.4.4, on main and here. It fails if the new read refuses or misreads a Blob that is in memory, a sliced one included. It is the first test that pins a Blob, a Request or a Response as socket data.
  • The whole file: 197 pass, 7 skip, 1 fail. The failure is should not call drain before handshake, which connects to www.example.com. This machine has no DNS, and the test fails the same way on main.
  • I ran these on Linux x64 only. CI build 132356 passed on all 181 jobs, which include macOS, Windows and arm64.

Cost. Release builds (bun run build:release) of main 6292c65 with and without the commit, Linux x64. gdb stepi from the first instruction of NewSocket<false>::write_or_end::<false> to its return, callees included, 8 calls on one loopback connection. Every later call gave the same count. The counts leave out mimalloc, whose refill path adds 92 on some calls in both builds.

data write(x) write(x, 0, 1)
string 789 → 771 787 → 795
Uint8Array 679 → 669 685 → 693
in-memory Blob 540 → 498 526 → 522
new Response("ef") 1035 → 993 1021 → 1017
new Response(new Blob(["ef"])) 865 → 823 851 → 847
  • First call of write("x"): 1414 to 1396. A string over TLS: 2933 to 2915. end("x"), from jsTCPSocketPrototypeFunction_end: 1442 to 1420.
  • main reads the payload twice when byteLength is not given. The saving is that read.
  • The 8 on an explicit range: the read now comes before the two integer conversions, and its pointer and length are saved across them. A variant that reads after the conversions measured the same (772 and 795), so the read stays where the guard was.
  • On the head of net, tls: consolidate the open node:net, node:tls, Bun.connect, Bun.listen and usockets pull requests #46522 the same change measures 750 to 704 for write("x") and 737 to 736 for write("x", 0, 1).
  • mimalloc calls per write_or_end, before and after: a string 0, an in-memory Blob 1 malloc and 1 free, new Response("ef") 3 and 3.
  • A refused write(s3file) and a refused end(s3file) enter 0 syscalls (gdb catch syscall). On main, end(s3file) enters setsockopt, epoll_ctl and close, and no sendto.
  • A refused call costs about 5,400 instructions and 8 mallocs. The Bun.file() refusal on main costs about 5,100 and 6: the message now has one formatted argument.
  • Size (llvm-size -A, llvm-nm -S): .text 82,253,657 to 82,253,145. write_or_end::<false> 1902 to 1749 bytes (TCP) and 1958 to 1809 (TLS).

Other open PRs.

Not in it.

Self-review. It found that the first cut went too far. That cut also kept a refused Request body unread and refused a printed S3 Request. It made three empty file-backed bodies throw where 0 was correct, and it changed node:http's private _write. I removed it. The review also asked for the sentinel read, the flush(), the sliced control rows and this cost table. The one point I did not take: it asked for a PR based on the branch of #46522. I based it on main, so that it has CI against main and can merge on its own. Also considered: a throw in the argument parser. Every write would then pass the caller's name to it.

`write_or_end` reads the payload once, through the new
`BlobOrStringOrBuffer::in_memory_view()`. The accessor returns `None` for a
Blob whose bytes are not in memory. The call then throws before it sends
or ends anything, and the socket stays open.

The guard tested `needs_to_read_file()`, which is true for a file store
only. An S3 store passed it, `slice()` was empty, `write` returned 0, and
`end` returned 0 and sent FIN.
@robobun

robobun commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on the Bun 1.4.4 release binary (38afee6) and on release and debug builds of main 6292c65. The script is in the Notes of the description. socket.write(s3file) returned 0. socket.end(s3file) returned 0 and the peer saw FIN. The S3 stand-in received no request.

The fix and its tests are in this PR: #46833. Its base is main. #46522 leaves this change for a separate review here. If #46522 merges first, I rebase this PR (one line conflicts).

@robobun

robobun commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:36 PM PT - Oct 24th, 2026

✅ @robobun, your commit 5534f6ccee0ed320a409406354b40b7246375f54 passed in Build #132356! 🎉


🧪   To try this PR locally:

bunx bun-pr 46833

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

bun-46833 --bun

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 179664e and 5534f6c.

Files selected for processing: 3
  • src/runtime/node/types.rs
  • src/runtime/socket/socket_body.rs
  • test/js/bun/net/socket.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Walkthrough

Socket.write() and Socket.end() now use in-memory byte views for synchronous payloads. They reject payloads without an in-memory view and direct callers to await blob.bytes(). TCP and TLS tests cover rejected payloads, in-memory payloads, and subsequent writes.

Changes

Socket payload handling

Layer / File(s) Summary
Expose in-memory payload bytes
src/runtime/node/types.rs
BlobOrStringOrBuffer now returns an optional in-memory byte view. Blob values use Blob::in_memory_view(); string and buffer values return their contained bytes.
Use views for socket writes and ends
src/runtime/socket/socket_body.rs, test/js/bun/net/socket.test.ts
NewSocket::write_or_end rejects payloads without an in-memory view and uses the view for default lengths and byte-range slicing. TCP and TLS tests cover rejected file- or S3-backed blobs, successful in-memory payloads, and subsequent writes.

Possibly related issues

  • oven-sh/bun#43381 — This change covers socket behavior after Socket.end() rejects an unsupported file- or S3-backed Blob.

Suggested reviewers: alii, cirospaciari, dylan-conway

Priority: Normal

Merge Risk: Minimal · up to 5534f

The change can merge after normal checks; no concrete unresolved risk has been established.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: refusing file- or S3-backed Blobs in socket write and end operations.
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.
Description check Passed The description clearly explains the problem, implementation, scope, behavioral changes, limitations, and verification results. It does not use the exact template headings, but it provides the require…
Full details: Description check

Explanation

The description clearly explains the problem, implementation, scope, behavioral changes, limitations, and verification results. It does not use the exact template headings, but it provides the required information, including detailed test coverage and known environment-specific failure.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@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.

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 data slice borrowed at src/runtime/socket/socket_body.rs:2894 and held across the byteOffset/byteLength coercions — is_any_int and to_int64 are pure numeric checks that cannot run user JS, and buffer stays alive until the function returns, so the view cannot dangle. The default byteLength and the bytes source now reading data instead of buffer.slice() is identical for every in-memory input (both resolve to shared_view() / str.slice()), and the control test pins that for sliced and offset Blobs over both tcp and tls.

Extended reasoning...

The change replaces the File-only guard in the shared write/end body of NewSocket (both SSL variants, via the IS_END const generic) with a fallible BlobOrStringOrBuffer::in_memory_view() so S3-backed Blobs are refused with a thrown Error before any socket state is touched, and adds tcp/tls tests with a loopback Bun.serve as the S3 stand-in. It touches no auth, crypto, or injection surface. The refactor was verified behavior-preserving for in-memory inputs and the borrowed slice is sound, but the inline findings about sibling guards and the consumed Request/Response body are substantive enough that a human should weigh them before merging.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟣 src/runtime/node/types.rs — pre-existing: callers that pass an S3File where a Bun.file() is refused still get empty bytes written silently, exactly as on the base branch. The sibling guards still test only needs_to_read_file(), which is false for an S3 store, so the blob passes and slice() yields nothing. Fix: route every file-or-S3 refusal through the same predicate the socket now uses (blob.in_memory_view().is_none()), which covers the 2 sites listed. Same pattern at 2 sites (src/runtime/node/types.rs:153, src/runtime/socket/uws_jsc.rs:193). Bun.build({ files: { "a.ts": s3file } }) then builds an empty module instead of throwing "File blob cannot be used here".

    Why this was flagged

    A user passes an S3File (client.file("key")) as a value in Bun.build's files option, which reaches BlobOrStringOrBuffer::from_js at src/runtime/api/JSBundler.rs:80 with FileBlobs::Reject. The guard at src/runtime/node/types.rs:153 checks only blob.needs_to_read_file(), which is false for an S3 store, so the S3File is accepted; JSBundler.rs:90 then copies blob_or_string.slice(), which is empty for an S3 store, and the virtual file is registered with zero bytes. A Bun.file(path) at the same spot throws "File blob cannot be used here". The node:http server socket write at src/runtime/socket/uws_jsc.rs:193 has the identical needs_to_read_file()-only guard and the identical empty-slice write at uws_jsc.rs:211. This PR fixes the third instance of the pattern in socket_body.rs:2894 with in_memory_view() but leaves these two unchanged; the description defers them to another issue. Behaviour is the same as on the base branch, so this is pre-existing, but the fix belongs in the shared helper so all three sites agree.

    Verification: pre-existing. Triggering condition: a user passes an S3File to any caller that parses with FileBlobs::Reject, e.g. a value in Bun.build's files option. src/runtime/node/types.rs:153 blob.needs_to_read_file() is false for store::Data::S3, so the S3 blob is accepted and slice() returns &[], so the virtual file is registered with zero bytes. The base branch fails the same way.

Comment thread src/runtime/node/types.rs
Comment thread src/runtime/socket/socket_body.rs
Comment thread src/runtime/node/types.rs
@robobun

robobun commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

On the two sites outside the diff: both are excluded on purpose, and the description names them under "Not in it".

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

I said on #46522 that I'd fold this in, and I've decided against it. The change itself reads well; the reasons are about #46522.

It stands well on its own with base main. #46522 rewrites write_or_end and keeps the old guard, so after it merges this needs a small rebase: the guard is the same four lines there. I've listed this PR under "Not included" in #46522.

@robobun

robobun commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

This PR stays on base main and waits for its own review.

This branch has not been deployed

No deployments
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