From 039d46e3cc682910bebefa0e6c4dd13a58495afa Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 00:17:00 +0000 Subject: [PATCH] blob: hand the read handler's pointer to on_read_bytes instead of &mut self ReadBytesHandler::on_read_bytes took `&mut self`, and its only implementor, BlobReadChain in Image.rs, reclaimed the Box it lives in through that receiver (heap::take(ptr::from_mut(self))). Freeing the allocation behind a reference argument while the call is still running is rejected by both Stacked Borrows and Tree Borrows, whether or not the reference is used again. The trait method now takes `this: *mut Self`, the same shape as ReadFileCompletion::run next to it: read_bytes_to_handler passes the ctx pointer it was given straight through at all four delivery sites (file completion, file cancel, S3 callback, synchronous in-memory), and the Image impl reclaims that pointer. The contract that exactly one delivery happens regardless of the return value is documented on read_bytes_to_handler. A source lint pins the shape tree-wide; Blob::deinit, which is being converted in a separate change, is allowlisted with a ratchet. Image tests now also cover the S3 and zero-length in-memory deliveries, and the worker-refusal suite gets a row for the cancel delivery. --- src/runtime/image/Image.rs | 19 +- src/runtime/webcore/Blob.rs | 54 ++++-- .../self-receiver-reclaim.test.ts | 172 ++++++++++++++++++ test/js/bun/image/image.test.ts | 57 ++++++ .../workers/worker-refused-completion.test.ts | 7 + 5 files changed, 285 insertions(+), 24 deletions(-) create mode 100644 test/internal/source-lints/self-receiver-reclaim.test.ts diff --git a/src/runtime/image/Image.rs b/src/runtime/image/Image.rs index 8949749ace9a..7a02e6a4d7ff 100644 --- a/src/runtime/image/Image.rs +++ b/src/runtime/image/Image.rs @@ -1302,9 +1302,11 @@ impl<'a> BlobReadChain<'a> { // file/S3). Ownership of the chain transfers there; the trait impl // below reconstructs the Box and frees it. let raw = bun_core::heap::into_raw(chain); - // SAFETY: `raw` is freshly leaked and uniquely owned by the read - // dispatch; reclaimed in `::on_read_bytes`. - unsafe { blob.read_bytes_to_handler(&raw mut *raw, global) }.map_err(jsc::JsError::from)?; + // SAFETY: `raw` is freshly leaked and not used again here; the read + // dispatch hands it to `on_read_bytes` below exactly once, also when it + // returns `Err` (a termination hit while delivering synchronously, i.e. + // after the chain has already been reclaimed). + unsafe { blob.read_bytes_to_handler(raw, global) }.map_err(jsc::JsError::from)?; Ok(promise) } @@ -1377,12 +1379,11 @@ impl<'a> BlobReadChain<'a> { } impl<'a> ReadBytesHandler for BlobReadChain<'a> { - fn on_read_bytes(&mut self, result: ReadBytesResult) { - // SAFETY: `self` is the `&mut *heap::alloc(chain)` handed to - // `read_bytes_to_handler` in `start()`; we are the sole consumer on - // the JS thread. Reconstruct the Box so the body can move fields out - // and free the allocation. - let boxed = unsafe { bun_core::heap::take(std::ptr::from_mut::(self)) }; + unsafe fn on_read_bytes(this: *mut Self, result: ReadBytesResult) { + // SAFETY: `this` is the Box `start()` leaked into `read_bytes_to_handler`, + // handed back to us exactly once (trait contract); nothing else points + // at it, so reclaiming it here is the chain's one and only free. + let boxed = unsafe { bun_core::heap::take(this) }; boxed.on_read_bytes_impl(result); } } diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index c0072d8e89a8..31a7836288d1 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -99,7 +99,18 @@ pub enum ReadBytesResult { /// Handler trait for `read_bytes_to_handler` — the body only requires /// `on_read_bytes`. pub trait ReadBytesHandler { - fn on_read_bytes(&mut self, result: ReadBytesResult); + /// Invoked exactly once, on the JS thread, with the `ctx` given to + /// `read_bytes_to_handler`; ownership of `*this` comes back to the handler + /// here, and a heap-allocated one reclaims itself (`heap::take(this)`). + /// That is why the receiver is a raw pointer, as in + /// `read_file::ReadFileCompletion::run`: freeing the allocation behind a + /// `&mut self` argument is UB under the aliasing model (the argument is + /// protected for the whole call), even if `self` is never touched again. + /// + /// # Safety + /// `this` is the `ctx` passed to `read_bytes_to_handler`, still live, and + /// the caller does not use it afterwards. + unsafe fn on_read_bytes(this: *mut Self, result: ReadBytesResult); } // ────────────────────────────────────────────────────────────────────────── @@ -147,8 +158,10 @@ pub trait BlobExt { ) -> JsTerminatedResult; fn do_read_file(&self, global: &JSGlobalObject) -> JSValue; /// # Safety - /// `ctx` must be a valid, exclusively-accessible `*mut H` that stays alive - /// until `H::on_read_bytes` is invoked (synchronously or via the async task). + /// `ctx` must be a valid, exclusively-accessible `*mut H`. Ownership of + /// `*ctx` passes to the single `H::on_read_bytes(ctx, ..)` this makes + /// (synchronously or from the async completion), whatever this returns; + /// the caller must not use `ctx` afterwards. unsafe fn read_bytes_to_handler( &self, ctx: *mut H, @@ -510,9 +523,18 @@ impl BlobExt for Blob { /// callers that already special-case `shared_view()` can keep doing that and /// only call this when it's empty. /// + /// Every store kind hands `ctx` to exactly one `H::on_read_bytes` call: + /// in-memory stores synchronously below, file stores from the read's + /// `run`/`cancel` completion (exactly one of which fires), S3 from the + /// download callback (which `execute_simple_s3_request` also invokes + /// synchronously when it cannot start the request). An `Err` here is a + /// termination raised by a synchronous delivery, so the handler has + /// already been consumed in that case too. + /// /// # Safety - /// `ctx` must be a valid, exclusively-accessible `*mut H` that stays alive - /// until `H::on_read_bytes` is invoked. + /// `ctx` must be a valid, exclusively-accessible `*mut H`. Ownership of + /// `*ctx` passes to the single `H::on_read_bytes(ctx, ..)` this makes, + /// whatever this returns; the caller must not use `ctx` afterwards. unsafe fn read_bytes_to_handler( &self, ctx: *mut H, @@ -531,12 +553,12 @@ impl BlobExt for Blob { } read_file::ReadFileResultType::Err(e) => ReadBytesResult::Err(Box::new(e)), }; - // SAFETY: `c` is the `*mut H` passed by the caller and kept alive - // across the async read by contract; exclusive borrow scoped to the call. - H::on_read_bytes(unsafe { &mut *c }, result); + // SAFETY: `c` is the `ctx` handed to `read_bytes_to_handler`, + // and the read completion fires exactly once (`call` or + // `cancel`), so this is its single delivery. + unsafe { H::on_read_bytes(c, result) }; } fn cancel(c: *mut H) { - // The caller owns `H` and waits for exactly one `on_read_bytes`. let err = jsc::SystemError { code: BunString::static_("ECANCELED").into(), message: BunString::static_( @@ -547,7 +569,7 @@ impl BlobExt for Blob { ..Default::default() }; // SAFETY: as for `call`. - H::on_read_bytes(unsafe { &mut *c }, ReadBytesResult::Err(Box::new(err))); + unsafe { H::on_read_bytes(c, ReadBytesResult::Err(Box::new(err))) }; } } self.do_read_file_internal::>(ctx, global); @@ -565,9 +587,10 @@ impl BlobExt for Blob { self.blob.deinit(); let ctx = self.ctx; drop(self); - // SAFETY: caller-owned ctx, kept alive by contract; exclusive - // borrow scoped to the call. - H::on_read_bytes(unsafe { &mut *ctx }, r); + // SAFETY: `ctx` is the pointer handed to `read_bytes_to_handler`; + // the download callback (and so `done`) runs exactly once, and + // the `Task` that held the pointer is gone. + unsafe { H::on_read_bytes(ctx, r) }; } fn cb( result: crate::webcore::__s3_client::S3DownloadResult, @@ -661,8 +684,9 @@ impl BlobExt for Blob { // In-memory or detached. let view = self.shared_view(); let owned = view.to_vec(); - // SAFETY: caller-owned ctx. - H::on_read_bytes(unsafe { &mut *ctx }, ReadBytesResult::Ok(owned)); + // SAFETY: `ctx` is this call's handler (fn contract) and this is its + // only delivery. + unsafe { H::on_read_bytes(ctx, ReadBytesResult::Ok(owned)) }; Ok(()) } diff --git a/test/internal/source-lints/self-receiver-reclaim.test.ts b/test/internal/source-lints/self-receiver-reclaim.test.ts new file mode 100644 index 000000000000..9bbe587dccc9 --- /dev/null +++ b/test/internal/source-lints/self-receiver-reclaim.test.ts @@ -0,0 +1,172 @@ +import { file } from "bun"; +import { expect, test } from "bun:test"; +import { realpathSync } from "fs"; +import path from "path"; +import { globAllSources } from "../../../scripts/glob-sources.ts"; + +// A method must not reclaim its own receiver's allocation: `heap::take` / +// `heap::destroy` / `Box::from_raw` applied to `self` or to a pointer spelled +// from `self` (`ptr::from_mut(self)`, `self as *mut _`, `&raw mut *self`, ...) +// inside a `&self` / `&mut self` method is banned. +// +// Two things are wrong with that shape, independently of whether the receiver +// really is a heap allocation: +// +// - A reference proves nothing about ownership: any `&mut T` into the +// object, however it was obtained, lets the method free an allocation +// somebody else holds the pointer to (or a stack local that was never +// heap-allocated at all). +// - Even on the intended path it is UB under the aliasing models: a +// reference argument is protected for the duration of the call, and +// deallocating protected memory is rejected by both Stacked Borrows +// ("deallocating while item is strongly protected") and Tree Borrows (the +// model `bun run rust:miri` uses), even if `self` is never touched again. +// The free has to go through the raw pointer the owner actually holds, +// which is why the tree's functions that end in a free take +// `this: *mut Self` (see `ReadBytesHandler::on_read_bytes` in +// src/runtime/webcore/Blob.rs, `ReadFileCompletion::run` in +// src/runtime/webcore/blob/read_file.rs, and the comments on `deinit` in +// src/sql_jsc/postgres/PostgresSQLConnection.rs). +// +// Scope: the single-expression spellings below, with `self` as the receiver. +// A self-derived pointer stashed in a local and freed later, a helper that +// takes the pointer and frees it (`Self::destroy(ptr::from_mut(self))`), and +// reference *parameters* (`fn f(this: &mut T)` freeing `this`) are outside this +// lint; they are the same bug, convert them on sight. +// +// Sibling guards: fn-long-mut-reborrow.test.ts, frozen-nonnull-reborrow.test.ts, +// unsound-erased-box.test.ts. + +const root = path.resolve(import.meta.dir, "..", "..", ".."); +const rustSources = globAllSources().rust.filter(p => p.endsWith(".rs")); + +// Only scan files tracked in HEAD (a `git stash` round-trip can leave stray +// `.rs` files in the working tree; CI runs on a clean checkout). Same guard as +// dead-code-escapes.test.ts. +const tracked: Set | null = (() => { + const r = Bun.spawnSync({ + cmd: ["git", "-C", root, "ls-tree", "-r", "--name-only", "-z", "HEAD"], + stdout: "pipe", + stderr: "ignore", + }); + if (!r.success) return null; + return new Set(r.stdout.toString().split("\0").filter(Boolean)); +})(); + +// Everything that turns a raw pointer back into an owning `Box` (and so frees +// it on drop), optionally path-qualified and turbofished. +const RECLAIM = String.raw`(?:heap::(?:take|destroy)|Box(?:::<[^>]*>)?::from_(?:raw|non_null))(?:::<[^>]*>)?\s*\(\s*`; + +// The ways of spelling "`self`, as a raw pointer" as the first argument. +// `(?!\s*\.)` after a bare `self` keeps `&raw mut *self.field` (a field's +// pointee) and similar out of it; the bare `self` form needs the closing paren +// (optionally after rustfmt's trailing comma) for the same reason. +const SELF_AS_POINTER = [ + // `heap::take(self)`: `&mut T` coerces to `*mut T` at the call. + String.raw`self\s*,?\s*\)`, + String.raw`(?:[\w:]+::)?from_(?:mut|ref)(?:::<[^>]*>)?\(\s*self\s*\)`, + String.raw`(?:[\w:]+::)?NonNull::from\(\s*self\s*\)`, + String.raw`self\s+as\s+\*(?:mut|const)\b`, + String.raw`&\s*(?:raw\s+(?:mut|const)|mut)\s+\*\s*self\b(?!\s*\.)`, + String.raw`(?:[\w:]+::)?addr_of(?:_mut)?!\s*\(\s*\*\s*self\s*\)`, +].join("|"); + +const BANNED = new RegExp(`${RECLAIM}(?:${SELF_AS_POINTER})`, "g"); + +// Documented, ratcheted exceptions: files allowed to keep exactly N of the +// shape. Prefer converting over adding an entry here. +const ALLOW: Record = { + // `Blob::deinit(&mut self)` frees heap-allocated blobs through its receiver. + // It is being converted separately (#37672); delete this entry when that + // lands. + "src/jsc/webcore_types.rs": 1, +}; + +const counts: Record = {}; +const offenders: string[] = []; +let scanned = 0; +for (const abs of rustSources) { + const source = path.relative(root, abs).replaceAll(path.sep, "/"); + // `src/cli` is a symlink into `src/runtime/cli`; count each file once under + // its canonical path. + if (path.relative(root, realpathSync(abs)).replaceAll(path.sep, "/") !== source) continue; + if (tracked !== null && !tracked.has(source)) continue; + scanned++; + const content = await file(abs).text(); + // Strip full-line comments so prose mentions (including the in-tree comments + // describing this hazard) don't count. `[ \t]*`, not `\s*`: `\s` crosses + // newlines and would swallow blank lines, shifting the reported line numbers. + const stripped = content.replace(/^[ \t]*\/\/.*$/gm, ""); + for (const m of stripped.matchAll(BANNED)) { + const line = stripped.slice(0, m.index).split("\n").length; + counts[source] = (counts[source] ?? 0) + 1; + if ((counts[source] ?? 0) > (ALLOW[source] ?? 0)) { + offenders.push(`${source}:${line}: ${m[0].replace(/\s+/g, " ")}`); + } + } +} + +function matches(snippet: string): boolean { + BANNED.lastIndex = 0; + return BANNED.test(snippet); +} + +test("scans a non-empty set of tracked Rust sources", () => { + // Guards against the tracked/realpath filters above over-firing and leaving + // nothing to scan, which would make the ban below pass vacuously. + expect(scanned).toBeGreaterThan(0); +}); + +test("the pattern recognizes the spellings it claims to", () => { + const banned = [ + // `::on_read_bytes(&mut self)`, as it + // was before the trait handed the pointer over. + "let boxed = unsafe { bun_core::heap::take(std::ptr::from_mut::(self)) };", + // `Blob::deinit(&mut self)`. + "unsafe { drop(bun_core::heap::take(std::ptr::from_mut::(self))) };", + "unsafe { bun_core::heap::destroy(self) };", + "drop(unsafe { Box::from_raw(self) });", + "unsafe { heap::destroy(ptr::from_ref(self).cast_mut()) }", + "unsafe { bun_core::heap::take(self as *const _ as *mut _) }", + "drop(unsafe { Box::from_raw(self as *mut Self) });", + "drop(unsafe { Box::from_raw(&raw mut *self) });", + "drop(unsafe { Box::from_raw(&mut *self) });", + "unsafe { heap::destroy(core::ptr::addr_of_mut!(*self)) }", + "unsafe { Box::from_non_null(NonNull::from(self)) }", + // rustfmt-wrapped calls. + "unsafe {\n bun_core::heap::take(\n std::ptr::from_mut::(self),\n )\n}", + "unsafe {\n bun_core::heap::destroy(\n self,\n )\n}", + ]; + const allowed = [ + // Freeing something the receiver owns is fine. + "unsafe { drop(bun_core::heap::take(self.worker_pool)) };", + "drop(unsafe { bun_core::heap::take(self.0.as_ptr()) });", + "unsafe { crate::heap::destroy(self.ptr.as_ptr()) };", + "drop(unsafe { Box::from_raw(self.walker) });", + "drop(unsafe { Box::from_raw(&raw mut *self.inner) });", + "unsafe { heap::take(std::ptr::from_mut(self.inner)) }", + // Raw-pointer receivers and other parameters are the intended shape / + // out of scope. + "unsafe { drop(bun_core::heap::take(this)) };", + "unsafe { heap::take(self_ptr) }", + "unsafe { bun_core::heap::destroy(std::ptr::from_mut::(self_)) };", + "unsafe { heap::take(ptr::from_mut(other)) }", + // Producing a pointer from `self` without reclaiming it is fine. + "let this = std::ptr::from_ref::(self).cast_mut();", + "Self::finalize(core::ptr::from_mut(self));", + ]; + expect(banned.filter(s => !matches(s))).toEqual([]); + expect(allowed.filter(matches)).toEqual([]); +}); + +test("no method reclaims its own receiver's allocation", () => { + expect(offenders).toEqual([]); +}); + +test("allowlisted files still carry exactly their documented count", () => { + // Ratchet: once an allowlisted instance is converted, delete its entry so + // a new one cannot take its place. + for (const [f, n] of Object.entries(ALLOW)) { + expect(counts[f] ?? 0).toBe(n); + } +}); diff --git a/test/js/bun/image/image.test.ts b/test/js/bun/image/image.test.ts index c5a7ffbf4bb3..b29502b1e1b4 100644 --- a/test/js/bun/image/image.test.ts +++ b/test/js/bun/image/image.test.ts @@ -1,3 +1,4 @@ +import { S3Client } from "bun"; import { afterAll, describe, expect, test } from "bun:test"; import { isMacOS, isWindows, tempDir } from "harness"; import zlib from "node:zlib"; @@ -177,6 +178,62 @@ describe("Bun.Image", () => { expect((await res.bytes()).subarray(8, 12)).toEqual(Buffer.from("WEBP")); }); + // Store-backed Blob sources are read at terminal time through the Blob's + // own store dispatch. The Bun.file() test above covers the file store; this + // covers the S3 download callback (bytes and error arm) and the synchronous + // in-memory delivery. + test("S3 and zero-length in-memory Blob sources are read through the same chain", async () => { + using server = Bun.serve({ + port: 0, + fetch(req) { + const { pathname } = new URL(req.url); + if (req.method === "GET" && pathname.endsWith("/src.png")) { + return new Response(cornersPng, { headers: { "Content-Type": "image/png" } }); + } + return new Response("", { status: 404 }); + }, + }); + const client = new S3Client({ + accessKeyId: "test", + secretAccessKey: "test", + region: "us-east-1", + bucket: "images", + endpoint: server.url.href, + }); + + // The S3 client sends every request through an ambient HTTP_PROXY, even + // one to the loopback endpoint above (#32045). Blank the variables for the + // duration of the test; an assignment (not a delete) is what the native + // env loader observes, and an empty value means "no proxy". + const proxyKeys = ["HTTP_PROXY", "http_proxy", "HTTPS_PROXY", "https_proxy"] as const; + const savedProxyEnv = Object.fromEntries(proxyKeys.map(k => [k, process.env[k]])); + for (const k of proxyKeys) process.env[k] = ""; + try { + const fromS3 = new Bun.Image(client.file("src.png")); + expect(await fromS3.metadata()).toEqual({ width: 4, height: 3, format: "png" }); + // Second terminal reuses the downloaded bytes. + expect((await fromS3.png().bytes())[0]).toBe(0x89); + expect(await client.file("src.png").image().metadata()).toEqual({ width: 4, height: 3, format: "png" }); + + // Download failure rejects the terminal with the S3 error. + expect( + await new Bun.Image(client.file("missing.png")).metadata().then( + () => null, + (e: any) => e.code, + ), + ).toBe("NoSuchKey"); + } finally { + for (const k of proxyKeys) process.env[k] = savedProxyEnv[k] ?? ""; + } + + // A zero-length slice of an in-memory Blob still has a store but nothing + // to copy at construction, so it is delivered synchronously at terminal + // time; the empty buffer then fails to decode. + const empty = new Blob([cornersPng]).slice(0, 0); + await expect(new Bun.Image(empty).metadata()).rejects.toThrow(/unrecognised format/); + await expect(empty.image().png().bytes()).rejects.toThrow(/unrecognised format/); + }); + test("metadata() reads PNG dimensions", async () => { const img = new Bun.Image(cornersPng); const meta = await img.metadata(); diff --git a/test/js/web/workers/worker-refused-completion.test.ts b/test/js/web/workers/worker-refused-completion.test.ts index c09f56f9ac97..20936d3485ed 100644 --- a/test/js/web/workers/worker-refused-completion.test.ts +++ b/test/js/web/workers/worker-refused-completion.test.ts @@ -41,6 +41,13 @@ const ROWS: Row[] = [ worker: `Bun.file(process.execPath).slice(0, 65536).text();`, refused: "blob::read_file::ReadFile", }, + { + // Same read job, different completion: the image's read chain is handed + // ECANCELED at teardown and has to free itself. + name: "Bun.Image(Bun.file()).metadata()", + worker: `new Bun.Image(Bun.file(process.execPath).slice(0, 65536)).metadata().catch(() => {});`, + refused: "blob::read_file::ReadFile", + }, { name: "crypto.pbkdf2", worker: `require("node:crypto").pbkdf2("p", "s", 1000, 32, "sha256", () => {});`,