blob: hand the read handler's pointer to on_read_bytes instead of &mut self

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 `<BlobReadChain as ReadBytesHandler>::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>(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);
     }
 }

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<JSValue>;
     fn do_read_file<F: read_file::ReadFileToJs>(&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<H: ReadBytesHandler>(
         &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<H: ReadBytesHandler>(
         &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::<H, Adapter<H>>(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(())
     }
 

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<string> | 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<string, number> = {
+  // `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<string, number> = {};
+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 = [
+    // `<BlobReadChain as ReadBytesHandler>::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>(self)) };",
+    // `Blob::deinit(&mut self)`.
+    "unsafe { drop(bun_core::heap::take(std::ptr::from_mut::<Blob>(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::<Blob>(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::<Blob>(self_)) };",
+    "unsafe { heap::take(ptr::from_mut(other)) }",
+    // Producing a pointer from `self` without reclaiming it is fine.
+    "let this = std::ptr::from_ref::<Blob>(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);
+  }
+});

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();

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", () => {});`,