From 2505446e60d0efb91e7630b90f766917d23c4b3a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 01:24:01 +0000 Subject: [PATCH 1/6] fetch: release the FetchTasklet through its raw pointer, not a &mut receiver FetchTasklet::on_progress_update(&mut self) ended the final progress hop with FetchTasklet::deref(ptr::from_mut(self)). That release is the tasklet's last ref whenever the HTTP thread has already dropped its own, which is the usual order, so deinit freed the allocation while the &mut self argument was still live. write_end_request(&mut self) had the same shape: its release is the last ref when the response finishes before a streamed request body does, and the promise handlers and the native sink reached it through a &mut receiver. Both now take *mut FetchTasklet, do their &mut work through a call-scoped reborrow (on_progress_update_locked / end_request_body), and release through the raw pointer once that borrow is over, the way callback and resume_request_data_stream already do. The task arm in dispatch.rs hands over task.ptr instead of forming a &mut. A source lint bans the shape tree-wide and ratchets the remaining instances. --- src/runtime/dispatch.rs | 4 +- .../webcore/fetch/FetchRequestBodySink.rs | 12 +- src/runtime/webcore/fetch/FetchTasklet.rs | 142 +++++++----- .../self-receiver-release.test.ts | 206 ++++++++++++++++++ 4 files changed, 304 insertions(+), 60 deletions(-) create mode 100644 test/internal/source-lints/self-receiver-release.test.ts diff --git a/src/runtime/dispatch.rs b/src/runtime/dispatch.rs index df0996b0438a..9052a375739b 100644 --- a/src/runtime/dispatch.rs +++ b/src/runtime/dispatch.rs @@ -348,8 +348,10 @@ pub(crate) fn run_task( | task_tag::ShellYesTask => run_task_cold(task), // ── fetch / S3 ─────────────────────────────────────────────────── + // The final hop releases the tasklet's JS-side ref, which may free it, + // so it takes the raw pointer (no `&mut` at this boundary). task_tag::FetchTasklet => { - cast!(FetchTasklet).on_progress_update()?; + FetchTasklet::on_progress_update(cast_ptr!(FetchTasklet))?; } task_tag::FetchTaskletDeinit => { // SAFETY: posted by `deref_from_thread` with the last ref. diff --git a/src/runtime/webcore/fetch/FetchRequestBodySink.rs b/src/runtime/webcore/fetch/FetchRequestBodySink.rs index 9f738a5ddd5c..eb80451d95bc 100644 --- a/src/runtime/webcore/fetch/FetchRequestBodySink.rs +++ b/src/runtime/webcore/fetch/FetchRequestBodySink.rs @@ -220,13 +220,13 @@ impl FetchRequestBodySink { // field; detach (not cancel) so we don't re-enter the source while // it is still on the stack (FileReader.on_reader_error ref-leak). self.source.clear(); - if let Some(mut task) = self.task.take() { + if let Some(task) = self.task.take() { let err_js = err.map(|e| e.to_js(&task.global_this)); - // SAFETY: the `+1` taken in `start_request_stream` keeps the - // tasklet live while `task` was `Some`; `write_end_request` is - // the balancing release and may free `*self` via `clear_sink`, - // so do not touch `self` afterwards. - unsafe { task.get_mut() }.write_end_request(err_js); + // The `+1` taken in `start_request_stream` kept the tasklet + // live while `task` was `Some`; `write_end_request` is the + // balancing release. It may free the tasklet, and `*self` with + // it via `clear_sink`, so do not touch `self` afterwards. + FetchTasklet::write_end_request(task.as_ptr(), err_js); } return; } diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 1e8ea8278f0c..410dda730c1d 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -311,18 +311,25 @@ impl FetchTasklet { /// /// INVARIANT: every `*mut FetchTasklet` threaded through the HTTP-thread /// callback (`callback`), the drain hook (`on_write_request_data_drain` / - /// `resume_request_data_stream`), and the JS-thread enqueue - /// (`queue` → `node`) was produced by `heap::into_raw(Box)` - /// in `get()` and is kept alive by the intrusive `ref_count` until - /// `deinit`. Access on either thread is serialised: HTTP-thread writes - /// happen under `mutex.lock()` and JS-thread access is single-threaded. + /// `resume_request_data_stream`), the progress hop (`on_progress_update`), + /// the request-body release (`write_end_request`), and the JS-thread + /// enqueue (`queue` → `node`) was produced by + /// `heap::into_raw(Box)` in `get()` and is kept alive by the + /// intrusive `ref_count` until `deinit`. Access on either thread is + /// serialised: HTTP-thread writes happen under `mutex.lock()` and + /// JS-thread access is single-threaded. + /// + /// The entry points above that end by releasing a ref keep the `&mut` + /// returned here scoped to the work that precedes the release and release + /// through the raw pointer: a release may be the last one, and freeing + /// the tasklet through a live `&mut` to it is UB. #[inline] fn from_raw_mut<'a>(this: *mut FetchTasklet) -> &'a mut Self { // SAFETY: see INVARIANT above. unsafe { &mut *this } } - /// Shared variant of [`from_raw_mut`] for paths that only read atomics - /// (`ref_count`, `is_shutting_down`) before deciding whether to upgrade. + /// Shared variant of [`from_raw_mut`] for the parts of an entry point that + /// only need `&self` (atomics, `mutex`, posting a task). #[inline] fn from_raw_ref<'a>(this: *mut FetchTasklet) -> &'a Self { // SAFETY: see [`from_raw_mut`] INVARIANT. @@ -631,8 +638,12 @@ impl FetchTasklet { // +1 on the tasklet; balanced exactly once by `write_end_request` on the // assign_to_stream-result side (on_resolve/on_reject or the synchronous // Fulfilled/Rejected/undefined branches below), or by the sink's - // `finalize` as a fallback if that path never runs. + // `finalize` as a fallback if that path never runs. The synchronous + // branches release it through `self_ptr` while the progress hop that + // called us still holds the JS-side ref, so none of them can free the + // tasklet. self.ref_(); + let self_ptr = std::ptr::from_mut::(self); if stream.is_locked(&global_this) || stream.is_disturbed(&global_this) { let err = jsc::SystemError { @@ -645,11 +656,10 @@ impl FetchTasklet { }; let err_instance = err.to_error_instance(&global_this); err_instance.ensure_still_alive(); - self.write_end_request(Some(err_instance)); + Self::write_end_request(self_ptr, Some(err_instance)); return; } - let self_ptr = std::ptr::from_mut::(self); // `self_ptr` is the live heap tasklet; the +1 above keeps it alive // until `write_end_request`/`finalize` clears `task`. let sink: &mut FetchRequestBodySink = Box::leak(Box::new(FetchRequestBodySink { @@ -683,7 +693,7 @@ impl FetchTasklet { err_js.ensure_still_alive(); err_js }); - self.write_end_request(err_js); + Self::write_end_request(self_ptr, err_js); return; } crate::webcore::readable_stream::NativeWireResult::NotNative => {} @@ -697,7 +707,7 @@ impl FetchTasklet { assignment_result.ensure_still_alive(); if let Some(err) = assignment_result.to_error() { - self.write_end_request(Some(err)); + Self::write_end_request(self_ptr, Some(err)); self.clear_sink(); return; } @@ -715,13 +725,13 @@ impl FetchTasklet { } bun_jsc::js_promise::Status::Fulfilled => { sink.task = None; - self.write_end_request(None); + Self::write_end_request(self_ptr, None); } bun_jsc::js_promise::Status::Rejected => { promise.set_handled(global_this.vm()); let result = promise.result(global_this.vm()); sink.task = None; - self.write_end_request(Some(result)); + Self::write_end_request(self_ptr, Some(result)); } } return; @@ -732,7 +742,7 @@ impl FetchTasklet { // assignToStream. `end()` no longer calls `write_end_request`, so this // path always balances the `+1` itself. sink.task = None; - self.write_end_request(None); + Self::write_end_request(self_ptr, None); } fn on_body_received(&mut self) -> JsTerminatedResult<()> { @@ -913,13 +923,36 @@ impl FetchTasklet { Ok(()) } - pub(crate) fn on_progress_update(&mut self) -> JsTerminatedResult<()> { + /// `task_tag::FetchTasklet` arm: one progress hop posted by `callback`. + /// + /// Takes the raw pointer, like `callback` and `resume_request_data_stream`: + /// the final hop releases the JS-side ref from `get()`, which is the last + /// one once the HTTP thread has dropped its own (the usual order, as + /// `callback` derefs right after posting the final hop), so the release + /// frees the tasklet and must not run while a `&mut self` to it is live. + /// Everything that needs `&mut` runs inside `on_progress_update_locked`, + /// which the JS-side ref outlives. + pub(crate) fn on_progress_update(this: *mut FetchTasklet) -> JsTerminatedResult<()> { jsc::mark_binding!(); bun_output::scoped_log!(FetchTasklet, "onProgressUpdate"); - self.mutex.lock(); - self.has_schedule_callback.store(false, Ordering::Relaxed); - let is_done = !self.result.has_more; + let shared = Self::from_raw_ref(this); + shared.mutex.lock(); + shared.has_schedule_callback.store(false, Ordering::Relaxed); + let is_done = !shared.result.has_more; + let result = Self::from_raw_mut(this).on_progress_update_locked(is_done); + if is_done { + // SAFETY: `this` is the live heap tasklet; this releases the + // JS-side ref from `get()`, and no borrow of `*this` is live. + FetchTasklet::deref(this); + } + result + } + /// Body of [`on_progress_update`](Self::on_progress_update). Entered with + /// `mutex` held; unlocks it on every path. Does not release the JS-side + /// ref, so no release reached from here (`write_end_request` via + /// `cancel_request_body_sink`) can be the tasklet's last one. + fn on_progress_update_locked(&mut self, is_done: bool) -> JsTerminatedResult<()> { let vm = self.global_this.bun_vm(); // teardown forbade script: we cannot touch JS if !vm.script_allowed() { @@ -932,10 +965,6 @@ impl FetchTasklet { } } self.mutex.unlock(); - if is_done { - // SAFETY: `self` is the live heap tasklet; we hold a ref. - FetchTasklet::deref(std::ptr::from_mut(self)); - } return Ok(()); } @@ -952,8 +981,6 @@ impl FetchTasklet { this.cancel_request_body_sink(JSValue::UNDEFINED); let mut poll_ref = core::mem::take(&mut this.poll_ref); poll_ref.unref(bun_io::js_vm_ctx()); - // SAFETY: `this` is the live heap tasklet; we hold a ref. - FetchTasklet::deref(std::ptr::from_mut(this)); } }; @@ -2277,13 +2304,31 @@ impl FetchTasklet { result } - pub(crate) fn write_end_request(&mut self, err: Option) { + /// Ends the streamed request body (terminating chunk + End, or an abort + /// carrying `err`) and releases the ref `start_request_stream` took for it. + /// + /// That ref is the tasklet's last one when the response finished before + /// the upload did: the final progress hop cancels the sink and drops the + /// JS-side ref, and the pump promise then settles into + /// `on_resolve_request_stream` / `on_reject_request_stream`, whose release + /// here frees the tasklet (`FetchRequestBodySink::end_from_stream` releases + /// it the same way). Hence the raw pointer: the `&mut` work happens in + /// `end_request_body`, and nothing borrows the tasklet when it is + /// released. The callers that pass a pointer to their own receiver + /// (`start_request_stream`, `cancel_request_body_sink`) each say why their + /// release cannot be the last one. + pub(crate) fn write_end_request(this: *mut FetchTasklet, err: Option) { bun_output::scoped_log!(FetchTasklet, "writeEndRequest hasError? {}", err.is_some()); - let this_ptr = std::ptr::from_mut(self); + Self::from_raw_mut(this).end_request_body(err); + // SAFETY: `this` is the live heap tasklet; this releases the + // `start_request_stream` ref, and no borrow of `*this` is live. + FetchTasklet::deref(this); + } + + /// The ref-count-neutral part of [`write_end_request`](Self::write_end_request). + fn end_request_body(&mut self, err: Option) { if let Some(js_error) = err { if self.signal_store.aborted.load(Ordering::Relaxed) || self.abort_reason.has() { - // SAFETY: `this_ptr` derived from live `&mut self`; we hold a ref. - FetchTasklet::deref(this_ptr); return; } if !js_error.is_undefined_or_null() { @@ -2292,15 +2337,11 @@ impl FetchTasklet { self.abort_task(); } else { if self.signal_store.aborted.load(Ordering::Relaxed) { - // SAFETY: `this_ptr` derived from live `&mut self`; we hold a ref. - FetchTasklet::deref(this_ptr); return; } if !self.skip_chunked_framing() { // Using chunked transfer encoding, send the terminating chunk let Some(thread_safe_stream_buffer) = self.stream_buffer_mut() else { - // SAFETY: `this_ptr` derived from live `&mut self`; we hold a ref. - FetchTasklet::deref(this_ptr); return; }; // Mutex guards `buffer` against the HTTP thread; released when @@ -2314,8 +2355,6 @@ impl FetchTasklet { .schedule_request_write(http_, http::http_thread::WriteMessageType::End); } } - // SAFETY: `this_ptr` derived from live `&mut self`; we hold a ref. - FetchTasklet::deref(this_ptr); } fn abort_task(&mut self) { @@ -2369,8 +2408,11 @@ impl FetchTasklet { if is_native { // No pump promise exists to balance the `+1` from // `start_request_stream`; `aborted` is set above so - // `write_end_request(Some(_))` is just the balancing deref. - self.write_end_request(Some(reason)); + // `write_end_request(Some(_))` is just the balancing deref. Never + // the last one: the final progress hop ends the sink (through this + // function) before its JS-side ref is released, so an un-ended + // sink means that ref is still held. + Self::write_end_request(std::ptr::from_mut(self), Some(reason)); } } @@ -2580,15 +2622,14 @@ fn on_resolve_request_stream( let args = callframe.arguments(); let this: *mut FetchTasklet = args[args.len() - 1].as_promise_ptr::(); // SAFETY: `as_promise_ptr` recovers the `*mut FetchTasklet` stashed by - // `start_request_stream`; the `ref_()` there keeps it alive, balanced by - // `write_end_request` below. Clear `sink.task` first so the sink's + // `start_request_stream`; the `ref_()` there keeps it alive until + // `write_end_request` below releases it (possibly freeing the tasklet, so + // the borrow taken here ends first). Clear `sink.task` so the sink's // `finalize()` fallback does not release a second time. - unsafe { - if let Some(sink) = (*this).sink_mut() { - sink.task = None; - } - (*this).write_end_request(None); + if let Some(sink) = unsafe { (*this).sink_mut() } { + sink.task = None; } + FetchTasklet::write_end_request(this, None); Ok(JSValue::UNDEFINED) } @@ -2599,16 +2640,11 @@ fn on_reject_request_stream( let args = callframe.arguments(); let this: *mut FetchTasklet = args[args.len() - 1].as_promise_ptr::(); let err = args[0]; - // SAFETY: `as_promise_ptr` recovers the `*mut FetchTasklet` stashed by - // `start_request_stream`; the `ref_()` there keeps it alive, balanced by - // `write_end_request` below. Clear `sink.task` first so the sink's - // `finalize()` fallback does not release a second time. - unsafe { - if let Some(sink) = (*this).sink_mut() { - sink.task = None; - } - (*this).write_end_request(Some(err)); + // SAFETY: as in `on_resolve_request_stream`. + if let Some(sink) = unsafe { (*this).sink_mut() } { + sink.task = None; } + FetchTasklet::write_end_request(this, Some(err)); Ok(JSValue::UNDEFINED) } diff --git a/test/internal/source-lints/self-receiver-release.test.ts b/test/internal/source-lints/self-receiver-release.test.ts new file mode 100644 index 000000000000..a04d4d4617ca --- /dev/null +++ b/test/internal/source-lints/self-receiver-release.test.ts @@ -0,0 +1,206 @@ +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"; + +// An intrusive-refcount release applied to a pointer spelled from a reference +// +// FetchTasklet::deref(std::ptr::from_mut(self)); +// T::deref(std::ptr::from_ref(this).cast_mut()); +// Self::deref_nn(NonNull::from(self)); +// let p = std::ptr::from_mut(self); ... Self::deref(p); +// +// is banned. A release may be the object's last one, and then the destructor +// frees the allocation the reference still points at. While the reference is +// a function (or closure) parameter, which `self` always is, both aliasing +// models reject that deallocation: Tree Borrows (what `bun run rust:miri` uses) +// reports "deallocation through is forbidden ... the strongly protected +// tag disallows deallocations", pointing at the `&mut self` receiver, and +// Stacked Borrows reports "deallocating while item [Unique] is strongly +// protected". `FetchTasklet::on_progress_update(&mut self)` was the canonical +// instance: every fetch's final progress hop released the tasklet's last ref +// from inside its own `&mut self`. +// +// The object was allocated as a raw pointer and every caller of these +// functions has it (a task arm's `task.ptr`, a callback's ctx, a `BackRef`'s +// `as_ptr()`), so the fix is structural: the function that ends in a release +// takes `this: *mut Self`, does its `&mut` work through a call-scoped reborrow +// (`Self::from_raw_mut(this).body()`), and releases through `this` once that +// borrow is over. See `on_progress_update` / `write_end_request` in +// src/runtime/webcore/fetch/FetchTasklet.rs. When the reference is a local +// reborrow rather than a parameter the release is not UB, but the raw pointer +// it was made from is in scope; release through that instead. +// +// This lint only knows the `deref` family of release names; a release spelled +// `unref`/`release` is out of its reach. Siblings: +// fn-long-mut-reborrow.test.ts, frozen-nonnull-reborrow.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)); +})(); + +// `deref(`, `deref_from_thread(`, `deref_nn(`, `deref_with_context(`, +// `rc_deref(`, however qualified. `\b` keeps `some_other_deref(` out. +const RELEASE = String.raw`\b(?:rc_)?deref(?:_from_thread|_nn|_with_context)?\(`; + +// A pointer spelled from a reference. `ptr::from_mut` / `ptr::from_ref` only +// accept references, so any argument counts; the remaining spellings are +// pinned to `self`, where a raw-pointer operand is impossible. +const POINTER_FROM_REFERENCE = [ + String.raw`(?:std::|core::)?ptr::from_mut(?:::<[^>]*>)?\(`, + String.raw`(?:std::|core::)?ptr::from_ref(?:::<[^>]*>)?\([^()]*\)\s*\.cast_mut\(\)`, + String.raw`(?:(?:std|core)::ptr::|ptr::)?NonNull::from\(\s*self\s*\)`, + String.raw`self\s+as\s+\*mut\b`, + String.raw`&raw\s+mut\s+\*\s*self\b`, +].join("|"); + +// Release applied directly to such a pointer. `\s*` between the two so a +// rustfmt line break cannot hide it. +const DIRECT = new RegExp(RELEASE + String.raw`\s*(?:` + POINTER_FROM_REFERENCE + ")", "g"); + +// `let p = std::ptr::from_mut(self);` / `let p = self as *mut Self;`, whose +// binding is then released (`deref(p)`) further down the same function. The +// function ends at the next `fn` item; a closure inside it is still the same +// function for this purpose. +const SELF_POINTER_BINDING = + /let\s+(?:mut\s+)?(\w+)\s*(?::[^=;]*)?=\s*(?:(?:std::|core::)?ptr::from_mut(?:::<[^>]*>)?\(\s*self\s*\)|self\s+as\s+\*mut\b[^;]*)\s*;/g; +const FN_ITEM = /^[ \t]*(?:pub(?:\([^)]*\))?\s+)?(?:(?:const|async|unsafe|extern\s+"[^"]*")\s+)*fn\s/m; + +function releaseOfBinding(name: string): RegExp { + return new RegExp(RELEASE + String.raw`\s*` + name + String.raw`\s*[,)]`); +} + +/** Byte offsets (into `stripped`) of every banned release in one file. */ +function findReleases(stripped: string): number[] { + const hits: number[] = []; + for (const m of stripped.matchAll(DIRECT)) hits.push(m.index); + for (const binding of stripped.matchAll(SELF_POINTER_BINDING)) { + const start = binding.index + binding[0].length; + const rest = stripped.slice(start); + const fnEnd = rest.search(FN_ITEM); + const body = fnEnd === -1 ? rest : rest.slice(0, fnEnd); + const release = body.search(releaseOfBinding(binding[1])); + if (release !== -1) hits.push(start + release); + } + return hits.sort((a, b) => a - b); +} + +function lineOf(text: string, offset: number): number { + return text.slice(0, offset).split("\n").length; +} + +// Documented, ratcheted exceptions: files allowed to keep exactly N of the +// shape. Every entry is a release that has been read and is either known to be +// harmless (another ref provably outlives the call) or tracked for its own +// fix. Lower the count when you convert one; do not add entries. +const ALLOW: Record = { + // Harmless: guarded by `ref_count > 1`; the final release goes through + // `DeferredDerefTask` precisely because the caller still uses the object. + "src/runtime/api/html_rewriter.rs": 1, + // Harmless: `write_sync` balances the `ref_()` it took a few lines up while + // the JS wrapper (the `this` of the call) holds its own ref. + "src/runtime/node/node_zlib_binding.rs": 1, + // Harmless: every `disarm` caller (`arm`, `finalize`, `deinit`) holds or + // has already consumed its own ref across the call. + "src/runtime/valkey_jsc/js_valkey.rs": 1, + // `finalize(&mut self)`, reached through the generated `JSSink` finalize + // thunk, releases the wrapper's ref, which is the last one for an idle + // sink; the second site releases the keep-alive ref while the wrapper's ref + // is still held. Needs the thunk to hand over the raw pointer; tracked + // separately. + "src/runtime/webcore/FileSink.rs": 2, + // Harmless: every `close()` caller releases its own creation ref only after + // `close()` returns. + "src/spawn/process.rs": 1, + // `on_write` releases the `start()` ref after `writer.close()` has let the + // owner drop the creation ref, so on POSIX it frees the writer from inside + // `on_write(&mut self)`; tracked separately. The other three sites run + // while the owner's or the in-flight write's ref is held. + "src/spawn/static_pipe_writer.rs": 4, + // Harmless: `stmt` is a local reborrow and the queued request holds its own + // ref on the statement until `release_statement`. + "src/sql_jsc/postgres/PostgresSQLConnection.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 doc comments + // describing this shape) don't count. `[ \t]*`, not `\s*`, so blank lines + // survive and reported line numbers stay right. + const stripped = content.replace(/^[ \t]*\/\/.*$/gm, ""); + for (const offset of findReleases(stripped)) { + counts[source] = (counts[source] ?? 0) + 1; + if (counts[source] > (ALLOW[source] ?? 0)) { + offenders.push(`${source}:${lineOf(stripped, offset)}`); + } + } +} + +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 patterns match the banned spellings and nothing else", () => { + const banned = [ + "FetchTasklet::deref(std::ptr::from_mut(self));", + "FetchTasklet::deref(std::ptr::from_mut(this));", + "unsafe { FileSink::deref(std::ptr::from_mut::(self)) };", + "unsafe { RefCount::::deref(\n std::ptr::from_mut::(self),\n) };", + "unsafe { T::deref(std::ptr::from_ref::(this).cast_mut()) };", + "Self::deref_nn(NonNull::from(self));", + "Self::deref_from_thread(self as *mut Self);", + "unsafe { T::rc_deref(&raw mut *self) };", + "let this_ptr = std::ptr::from_mut(self);\nif done {\n FetchTasklet::deref(this_ptr);\n return;\n}", + "let this: *mut Self = self as *mut Self;\nSelf::deref(this);", + ]; + const allowed = [ + "FetchTasklet::deref(this);", + "FetchTasklet::deref(task.as_ptr());", + "unsafe { ThreadSafeRefCount::::deref(this) };", + "let self_ptr = std::ptr::from_mut::(self);\nSelf::write_end_request(self_ptr, None);", + // The binding is released, but in the next function, where it is a + // raw-pointer parameter of the same name. + "let this = std::ptr::from_mut(self);\nregister(this);\n}\n\nfn resume(this: *mut Self) {\n Self::deref(this);\n}", + "signal.clean_native_bindings(std::ptr::from_mut(self).cast::());", + "let value = strong.deref();", + "some_other_deref(std::ptr::from_mut(self));", + ]; + expect(banned.map(s => findReleases(s).length)).toEqual(banned.map(() => 1)); + expect(allowed.map(s => findReleases(s).length)).toEqual(allowed.map(() => 0)); +}); + +test("refcount release through a pointer spelled from the receiver is banned", () => { + expect(offenders).toEqual([]); +}); + +test("allowlisted files still carry exactly their documented count", () => { + // Ratchet: when an allowlisted release is converted, lower its entry so the + // shape cannot come back into that file. + for (const [source, n] of Object.entries(ALLOW)) { + expect({ source, count: counts[source] ?? 0 }).toEqual({ source, count: n }); + } +}); From 067addfef93bc64ce0b462b0ba62d7584271383e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 03:26:03 +0000 Subject: [PATCH 2/6] fetch: release tasklet refs through ScopedRef guards, delete FetchTasklet::deref Every JS-thread release of a FetchTasklet ref (the final progress hop, the drain hop, write_end_request, the sink's finalize fallback and the teardown release of a queued hop) now adopts the ref it owns into a bun_ptr::ScopedRef at the raw-pointer entry point, before any reference to the tasklet is formed; the guard releases when it drops, after the body's borrow is gone. With no raw JS-thread release left in the file, a &mut self method has nothing to release itself with. deref_from_thread stays: it is the HTTP thread's release and hops the destroy to the JS thread rather than freeing in place. The lint also bans ScopedRef::adopt on a pointer spelled from the receiver, which is the same bug with a guard around it. --- .../webcore/fetch/FetchRequestBodySink.rs | 6 +- src/runtime/webcore/fetch/FetchTasklet.rs | 90 +++++++++---------- .../self-receiver-release.test.ts | 37 +++++--- 3 files changed, 68 insertions(+), 65 deletions(-) diff --git a/src/runtime/webcore/fetch/FetchRequestBodySink.rs b/src/runtime/webcore/fetch/FetchRequestBodySink.rs index eb80451d95bc..543672206a76 100644 --- a/src/runtime/webcore/fetch/FetchRequestBodySink.rs +++ b/src/runtime/webcore/fetch/FetchRequestBodySink.rs @@ -1,5 +1,5 @@ use bun_collections::ByteVecExt; -use bun_ptr::BackRef; +use bun_ptr::{BackRef, ScopedRef}; use bun_sys::Error as SysError; use crate::webcore::blob::SizeType as BlobSizeType; @@ -253,7 +253,9 @@ impl FetchRequestBodySink { if let Some(task) = task { // Balances the `ref_()` taken in `start_request_stream` when the // assign_to_stream-result handler never ran to release it. - FetchTasklet::deref(task.as_ptr()); + // SAFETY: that handler clears `task` before releasing, so `task` + // being `Some` means the ref is still held and the tasklet live. + drop(unsafe { ScopedRef::::adopt(task.as_ptr()) }); } } diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 410dda730c1d..71f323fe7a0e 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -21,6 +21,7 @@ use bun_jsc::virtual_machine::VirtualMachine; use bun_jsc::{ self as jsc, GlobalRef, JSGlobalObject, JSValue, JsResult, StringJsc, StrongOptional, }; +use bun_ptr::ScopedRef; use bun_sys::FdExt; use bun_threading::Mutex; use bun_url::URL as ZigURL; @@ -72,7 +73,8 @@ impl Taskable for FetchTasklet { /// `on_progress_update` would have dropped. The HTTP thread is parked / /// this VM's requests are back, so a 1→0 here deinits against a live heap. unsafe fn release_unrun(this: *mut Self) { - FetchTasklet::deref(this); + // SAFETY: fn contract; the queued hop's ref is the one adopted. + drop(unsafe { ScopedRef::adopt(this) }); } } @@ -319,10 +321,11 @@ impl FetchTasklet { /// serialised: HTTP-thread writes happen under `mutex.lock()` and /// JS-thread access is single-threaded. /// - /// The entry points above that end by releasing a ref keep the `&mut` - /// returned here scoped to the work that precedes the release and release - /// through the raw pointer: a release may be the last one, and freeing - /// the tasklet through a live `&mut` to it is UB. + /// An entry point that owns one of those refs adopts it into a + /// [`ScopedRef`] from the raw pointer before calling this, and lets the + /// guard drop after the borrow is dead: the release may be the last one, + /// and freeing the tasklet while a `&mut` to it is live is UB. There is no + /// JS-thread release a `&mut self` method could call instead. #[inline] fn from_raw_mut<'a>(this: *mut FetchTasklet) -> &'a mut Self { // SAFETY: see INVARIANT above. @@ -400,23 +403,15 @@ impl FetchTasklet { .map(|p| unsafe { &mut *p.as_ptr() }) } + /// Takes a ref. Each one is released on the JS thread by a `ScopedRef` + /// adopted at the entry point that owns it (see `from_raw_mut`), or on the + /// HTTP thread by `deref_from_thread`. fn ref_(&self) { // SAFETY: `self` is live; `ref_` only touches the interior-mutable // atomic counter. unsafe { bun_ptr::ThreadSafeRefCount::::ref_(core::ptr::from_ref(self).cast_mut()) }; } - /// # Safety - /// Caller holds a ref; `this` must be a live heap allocation from `get()`. - // Forwards `this` to ThreadSafeRefCount without dereferencing; signature must stay - // `*mut` because the call may drop the last ref and free the allocation, so a `&mut` - // here would be UB. - #[allow(clippy::not_unsafe_ptr_arg_deref)] - pub(crate) fn deref(this: *mut FetchTasklet) { - // SAFETY: caller contract. - unsafe { bun_ptr::ThreadSafeRefCount::::deref(this) }; - } - /// # Safety /// Caller holds a ref; `this` must be a live heap allocation from `get()`. // Forwards `this` to ThreadSafeRefCount/dealloc without dereferencing; signature must @@ -926,12 +921,12 @@ impl FetchTasklet { /// `task_tag::FetchTasklet` arm: one progress hop posted by `callback`. /// /// Takes the raw pointer, like `callback` and `resume_request_data_stream`: - /// the final hop releases the JS-side ref from `get()`, which is the last - /// one once the HTTP thread has dropped its own (the usual order, as - /// `callback` derefs right after posting the final hop), so the release - /// frees the tasklet and must not run while a `&mut self` to it is live. - /// Everything that needs `&mut` runs inside `on_progress_update_locked`, - /// which the JS-side ref outlives. + /// the final hop owns the JS-side ref from `get()`, which is the last one + /// once the HTTP thread has dropped its own (the usual order, as + /// `callback` derefs right after posting the final hop), so releasing it + /// frees the tasklet. The ref is adopted into a guard here and everything + /// that needs `&mut` runs inside `on_progress_update_locked`; the guard + /// drops, and the tasklet with it, only after that borrow is gone. pub(crate) fn on_progress_update(this: *mut FetchTasklet) -> JsTerminatedResult<()> { jsc::mark_binding!(); bun_output::scoped_log!(FetchTasklet, "onProgressUpdate"); @@ -939,13 +934,11 @@ impl FetchTasklet { shared.mutex.lock(); shared.has_schedule_callback.store(false, Ordering::Relaxed); let is_done = !shared.result.has_more; - let result = Self::from_raw_mut(this).on_progress_update_locked(is_done); - if is_done { - // SAFETY: `this` is the live heap tasklet; this releases the - // JS-side ref from `get()`, and no borrow of `*this` is live. - FetchTasklet::deref(this); - } - result + // SAFETY: `this` is the live heap tasklet and the final hop owns the + // JS-side ref from `get()`. + let _js_ref = is_done.then(|| unsafe { ScopedRef::adopt(this) }); + // `_js_ref` drops after this call returns. + Self::from_raw_mut(this).on_progress_update_locked(is_done) } /// Body of [`on_progress_update`](Self::on_progress_update). Entered with @@ -2206,22 +2199,19 @@ impl FetchTasklet { /// This is ALWAYS called from the main thread // ConcurrentTask::from_callback expects `fn(*mut T) -> bun_event_loop::JsResult<()>`. fn resume_request_data_stream(this: *mut FetchTasklet) -> ElJsResult<()> { - let this_ref = Self::from_raw_mut(this); bun_output::scoped_log!(FetchTasklet, "resumeRequestDataStream"); - let result = (|| { - if this_ref.signal_aborted() { - // already aborted; nothing to drain - return; - } - let global_this = this_ref.global_this; - if let Some(sink) = this_ref.sink_mut() { - sink.on_drain(&global_this); - } - })(); - // deref when done because we ref inside onWriteRequestDataDrain - // SAFETY: `this` is the live heap tasklet; we hold a ref. - FetchTasklet::deref(this); - let () = result; + // SAFETY: `this` is the live heap tasklet; this hop owns the ref + // `on_write_request_data_drain` took. + let _drain_ref = unsafe { ScopedRef::adopt(this) }; + let this_ref = Self::from_raw_mut(this); + if this_ref.signal_aborted() { + // already aborted; nothing to drain + return Ok(()); + } + let global_this = this_ref.global_this; + if let Some(sink) = this_ref.sink_mut() { + sink.on_drain(&global_this); + } Ok(()) } @@ -2312,17 +2302,17 @@ impl FetchTasklet { /// JS-side ref, and the pump promise then settles into /// `on_resolve_request_stream` / `on_reject_request_stream`, whose release /// here frees the tasklet (`FetchRequestBodySink::end_from_stream` releases - /// it the same way). Hence the raw pointer: the `&mut` work happens in - /// `end_request_body`, and nothing borrows the tasklet when it is - /// released. The callers that pass a pointer to their own receiver + /// it the same way). Hence the raw pointer: the ref is adopted into a guard + /// that drops after `end_request_body`, the `&mut` part, has returned. The + /// callers that pass a pointer to their own receiver /// (`start_request_stream`, `cancel_request_body_sink`) each say why their /// release cannot be the last one. pub(crate) fn write_end_request(this: *mut FetchTasklet, err: Option) { bun_output::scoped_log!(FetchTasklet, "writeEndRequest hasError? {}", err.is_some()); + // SAFETY: `this` is the live heap tasklet; the caller owns the + // `start_request_stream` ref. + let _body_ref = unsafe { ScopedRef::adopt(this) }; Self::from_raw_mut(this).end_request_body(err); - // SAFETY: `this` is the live heap tasklet; this releases the - // `start_request_stream` ref, and no borrow of `*this` is live. - FetchTasklet::deref(this); } /// The ref-count-neutral part of [`write_end_request`](Self::write_end_request). diff --git a/test/internal/source-lints/self-receiver-release.test.ts b/test/internal/source-lints/self-receiver-release.test.ts index a04d4d4617ca..93ba2ac506b3 100644 --- a/test/internal/source-lints/self-receiver-release.test.ts +++ b/test/internal/source-lints/self-receiver-release.test.ts @@ -9,6 +9,7 @@ import { globAllSources } from "../../../scripts/glob-sources.ts"; // FetchTasklet::deref(std::ptr::from_mut(self)); // T::deref(std::ptr::from_ref(this).cast_mut()); // Self::deref_nn(NonNull::from(self)); +// drop(ScopedRef::adopt(std::ptr::from_mut(self))); // let p = std::ptr::from_mut(self); ... Self::deref(p); // // is banned. A release may be the object's last one, and then the destructor @@ -24,16 +25,19 @@ import { globAllSources } from "../../../scripts/glob-sources.ts"; // // The object was allocated as a raw pointer and every caller of these // functions has it (a task arm's `task.ptr`, a callback's ctx, a `BackRef`'s -// `as_ptr()`), so the fix is structural: the function that ends in a release -// takes `this: *mut Self`, does its `&mut` work through a call-scoped reborrow -// (`Self::from_raw_mut(this).body()`), and releases through `this` once that -// borrow is over. See `on_progress_update` / `write_end_request` in -// src/runtime/webcore/fetch/FetchTasklet.rs. When the reference is a local -// reborrow rather than a parameter the release is not UB, but the raw pointer -// it was made from is in scope; release through that instead. +// `as_ptr()`), so the fix is structural: the function that owns the ref takes +// `this: *mut Self`, adopts the ref into a `bun_ptr::ScopedRef` before any +// reference to `*this` exists, and does its `&mut` work through a call-scoped +// reborrow (`Self::from_raw_mut(this).body()`); the guard releases when it +// drops, after that borrow is gone. Once every release of a type is a guard, +// the type needs no raw release function at all, and a `&mut self` method has +// nothing left to misuse (src/runtime/webcore/fetch/FetchTasklet.rs). When the +// reference is a local reborrow rather than a parameter the release is not UB, +// but the raw pointer it was made from is in scope; adopt that instead. // -// This lint only knows the `deref` family of release names; a release spelled -// `unref`/`release` is out of its reach. Siblings: +// This lint knows the `deref` family of release names and `ScopedRef::adopt` +// (`ScopedRef::new` takes its own ref and is balanced, so it is not a release); +// a release spelled `unref`/`release` is out of its reach. Siblings: // fn-long-mut-reborrow.test.ts, frozen-nonnull-reborrow.test.ts. const root = path.resolve(import.meta.dir, "..", "..", ".."); @@ -53,8 +57,9 @@ const tracked: Set | null = (() => { })(); // `deref(`, `deref_from_thread(`, `deref_nn(`, `deref_with_context(`, -// `rc_deref(`, however qualified. `\b` keeps `some_other_deref(` out. -const RELEASE = String.raw`\b(?:rc_)?deref(?:_from_thread|_nn|_with_context)?\(`; +// `rc_deref(`, however qualified (`\b` keeps `some_other_deref(` out), and +// `ScopedRef::adopt(` / `ScopedRef::::adopt(`. +const RELEASE = String.raw`(?:\b(?:rc_)?deref(?:_from_thread|_nn|_with_context)?|\bScopedRef(?:::<[^>]*>)?::adopt)\(`; // A pointer spelled from a reference. `ptr::from_mut` / `ptr::from_ref` only // accept references, so any argument counts; the remaining spellings are @@ -174,12 +179,18 @@ test("the patterns match the banned spellings and nothing else", () => { "Self::deref_nn(NonNull::from(self));", "Self::deref_from_thread(self as *mut Self);", "unsafe { T::rc_deref(&raw mut *self) };", + "drop(unsafe { ScopedRef::adopt(std::ptr::from_mut(self)) });", + "let _ref = unsafe { ScopedRef::::adopt(\n std::ptr::from_mut::(self),\n) };", "let this_ptr = std::ptr::from_mut(self);\nif done {\n FetchTasklet::deref(this_ptr);\n return;\n}", "let this: *mut Self = self as *mut Self;\nSelf::deref(this);", + "let this = std::ptr::from_mut(self);\nlet _guard = unsafe { ScopedRef::adopt(this) };", ]; const allowed = [ - "FetchTasklet::deref(this);", - "FetchTasklet::deref(task.as_ptr());", + "FetchTasklet::deref_from_thread(task);", + "let _js_ref = is_done.then(|| unsafe { ScopedRef::adopt(this) });", + "drop(unsafe { ScopedRef::::adopt(task.as_ptr()) });", + // `new` takes a ref of its own and releases that one: balanced. + "let _guard = unsafe { ScopedRef::new(std::ptr::from_mut::(self)) };", "unsafe { ThreadSafeRefCount::::deref(this) };", "let self_ptr = std::ptr::from_mut::(self);\nSelf::write_end_request(self_ptr, None);", // The binding is released, but in the next function, where it is a From b80c34a817569125b122db0a2e24a07e9b5ef823 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 03:47:36 +0000 Subject: [PATCH 3/6] fetch: shorten the ownership comments; lint the stored forms of the receiver pointer The lint now also catches a receiver-derived pointer that is stored first (NonNull::from(self), ptr::from_ref(self).cast_mut(), &raw mut *self) and released later in the same function, including through .as_ptr(). That finds one more balanced release (PostgresSQLQuery::do_run), allowlisted. --- src/runtime/dispatch.rs | 2 - .../webcore/fetch/FetchRequestBodySink.rs | 6 +- src/runtime/webcore/fetch/FetchTasklet.rs | 71 ++++++------------- .../self-receiver-release.test.ts | 30 +++++--- 4 files changed, 45 insertions(+), 64 deletions(-) diff --git a/src/runtime/dispatch.rs b/src/runtime/dispatch.rs index 9052a375739b..08f58d04483a 100644 --- a/src/runtime/dispatch.rs +++ b/src/runtime/dispatch.rs @@ -348,8 +348,6 @@ pub(crate) fn run_task( | task_tag::ShellYesTask => run_task_cold(task), // ── fetch / S3 ─────────────────────────────────────────────────── - // The final hop releases the tasklet's JS-side ref, which may free it, - // so it takes the raw pointer (no `&mut` at this boundary). task_tag::FetchTasklet => { FetchTasklet::on_progress_update(cast_ptr!(FetchTasklet))?; } diff --git a/src/runtime/webcore/fetch/FetchRequestBodySink.rs b/src/runtime/webcore/fetch/FetchRequestBodySink.rs index 543672206a76..cb4417051028 100644 --- a/src/runtime/webcore/fetch/FetchRequestBodySink.rs +++ b/src/runtime/webcore/fetch/FetchRequestBodySink.rs @@ -222,10 +222,8 @@ impl FetchRequestBodySink { self.source.clear(); if let Some(task) = self.task.take() { let err_js = err.map(|e| e.to_js(&task.global_this)); - // The `+1` taken in `start_request_stream` kept the tasklet - // live while `task` was `Some`; `write_end_request` is the - // balancing release. It may free the tasklet, and `*self` with - // it via `clear_sink`, so do not touch `self` afterwards. + // Releases the `start_request_stream` ref; that may free the + // tasklet and, through `clear_sink`, `*self`. FetchTasklet::write_end_request(task.as_ptr(), err_js); } return; diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 71f323fe7a0e..50f248147e0d 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -321,11 +321,9 @@ impl FetchTasklet { /// serialised: HTTP-thread writes happen under `mutex.lock()` and /// JS-thread access is single-threaded. /// - /// An entry point that owns one of those refs adopts it into a - /// [`ScopedRef`] from the raw pointer before calling this, and lets the - /// guard drop after the borrow is dead: the release may be the last one, - /// and freeing the tasklet while a `&mut` to it is live is UB. There is no - /// JS-thread release a `&mut self` method could call instead. + /// An entry point that owns a ref adopts it into a [`ScopedRef`] before + /// calling this; the guard (which may free the tasklet) drops after the + /// borrow is gone. #[inline] fn from_raw_mut<'a>(this: *mut FetchTasklet) -> &'a mut Self { // SAFETY: see INVARIANT above. @@ -403,9 +401,8 @@ impl FetchTasklet { .map(|p| unsafe { &mut *p.as_ptr() }) } - /// Takes a ref. Each one is released on the JS thread by a `ScopedRef` - /// adopted at the entry point that owns it (see `from_raw_mut`), or on the - /// HTTP thread by `deref_from_thread`. + /// Released by a `ScopedRef` adopted at the JS-thread entry point that + /// owns the ref, or by `deref_from_thread` on the HTTP thread. fn ref_(&self) { // SAFETY: `self` is live; `ref_` only touches the interior-mutable // atomic counter. @@ -634,9 +631,8 @@ impl FetchTasklet { // assign_to_stream-result side (on_resolve/on_reject or the synchronous // Fulfilled/Rejected/undefined branches below), or by the sink's // `finalize` as a fallback if that path never runs. The synchronous - // branches release it through `self_ptr` while the progress hop that - // called us still holds the JS-side ref, so none of them can free the - // tasklet. + // branches release it while our caller, the progress hop, still holds + // the JS-side ref. self.ref_(); let self_ptr = std::ptr::from_mut::(self); @@ -918,15 +914,9 @@ impl FetchTasklet { Ok(()) } - /// `task_tag::FetchTasklet` arm: one progress hop posted by `callback`. - /// - /// Takes the raw pointer, like `callback` and `resume_request_data_stream`: - /// the final hop owns the JS-side ref from `get()`, which is the last one - /// once the HTTP thread has dropped its own (the usual order, as - /// `callback` derefs right after posting the final hop), so releasing it - /// frees the tasklet. The ref is adopted into a guard here and everything - /// that needs `&mut` runs inside `on_progress_update_locked`; the guard - /// drops, and the tasklet with it, only after that borrow is gone. + /// `task_tag::FetchTasklet` arm: one progress hop posted by `callback`. The + /// final hop owns the JS-side ref from `get()`, normally the last one + /// (`callback` drops the HTTP thread's right after posting the hop). pub(crate) fn on_progress_update(this: *mut FetchTasklet) -> JsTerminatedResult<()> { jsc::mark_binding!(); bun_output::scoped_log!(FetchTasklet, "onProgressUpdate"); @@ -934,17 +924,13 @@ impl FetchTasklet { shared.mutex.lock(); shared.has_schedule_callback.store(false, Ordering::Relaxed); let is_done = !shared.result.has_more; - // SAFETY: `this` is the live heap tasklet and the final hop owns the - // JS-side ref from `get()`. + // SAFETY: `this` is live; the final hop owns the JS-side ref. let _js_ref = is_done.then(|| unsafe { ScopedRef::adopt(this) }); - // `_js_ref` drops after this call returns. Self::from_raw_mut(this).on_progress_update_locked(is_done) } - /// Body of [`on_progress_update`](Self::on_progress_update). Entered with - /// `mutex` held; unlocks it on every path. Does not release the JS-side - /// ref, so no release reached from here (`write_end_request` via - /// `cancel_request_body_sink`) can be the tasklet's last one. + /// Entered with `mutex` held; unlocks it on every path. The caller holds + /// the JS-side ref throughout, so no release reached from here is the last. fn on_progress_update_locked(&mut self, is_done: bool) -> JsTerminatedResult<()> { let vm = self.global_this.bun_vm(); // teardown forbade script: we cannot touch JS @@ -2296,21 +2282,12 @@ impl FetchTasklet { /// Ends the streamed request body (terminating chunk + End, or an abort /// carrying `err`) and releases the ref `start_request_stream` took for it. - /// - /// That ref is the tasklet's last one when the response finished before - /// the upload did: the final progress hop cancels the sink and drops the - /// JS-side ref, and the pump promise then settles into - /// `on_resolve_request_stream` / `on_reject_request_stream`, whose release - /// here frees the tasklet (`FetchRequestBodySink::end_from_stream` releases - /// it the same way). Hence the raw pointer: the ref is adopted into a guard - /// that drops after `end_request_body`, the `&mut` part, has returned. The - /// callers that pass a pointer to their own receiver - /// (`start_request_stream`, `cancel_request_body_sink`) each say why their - /// release cannot be the last one. + /// That ref is the last one when the response finished before the upload + /// (the pump promise settles after the final hop), so this frees the + /// tasklet from the promise handlers and `end_from_stream`. pub(crate) fn write_end_request(this: *mut FetchTasklet, err: Option) { bun_output::scoped_log!(FetchTasklet, "writeEndRequest hasError? {}", err.is_some()); - // SAFETY: `this` is the live heap tasklet; the caller owns the - // `start_request_stream` ref. + // SAFETY: `this` is live; the caller owns the `start_request_stream` ref. let _body_ref = unsafe { ScopedRef::adopt(this) }; Self::from_raw_mut(this).end_request_body(err); } @@ -2398,10 +2375,8 @@ impl FetchTasklet { if is_native { // No pump promise exists to balance the `+1` from // `start_request_stream`; `aborted` is set above so - // `write_end_request(Some(_))` is just the balancing deref. Never - // the last one: the final progress hop ends the sink (through this - // function) before its JS-side ref is released, so an un-ended - // sink means that ref is still held. + // `write_end_request(Some(_))` is just the balancing deref, never + // the last: the final hop ends the sink here before dropping its ref. Self::write_end_request(std::ptr::from_mut(self), Some(reason)); } } @@ -2611,11 +2586,9 @@ fn on_resolve_request_stream( ) -> JsResult { let args = callframe.arguments(); let this: *mut FetchTasklet = args[args.len() - 1].as_promise_ptr::(); - // SAFETY: `as_promise_ptr` recovers the `*mut FetchTasklet` stashed by - // `start_request_stream`; the `ref_()` there keeps it alive until - // `write_end_request` below releases it (possibly freeing the tasklet, so - // the borrow taken here ends first). Clear `sink.task` so the sink's - // `finalize()` fallback does not release a second time. + // SAFETY: `start_request_stream` stashed `this` and still holds the ref that + // `write_end_request` below releases. Clearing `sink.task` first keeps the + // sink's `finalize()` fallback from releasing it again. if let Some(sink) = unsafe { (*this).sink_mut() } { sink.task = None; } diff --git a/test/internal/source-lints/self-receiver-release.test.ts b/test/internal/source-lints/self-receiver-release.test.ts index 93ba2ac506b3..3775649a0cab 100644 --- a/test/internal/source-lints/self-receiver-release.test.ts +++ b/test/internal/source-lints/self-receiver-release.test.ts @@ -10,7 +10,7 @@ import { globAllSources } from "../../../scripts/glob-sources.ts"; // T::deref(std::ptr::from_ref(this).cast_mut()); // Self::deref_nn(NonNull::from(self)); // drop(ScopedRef::adopt(std::ptr::from_mut(self))); -// let p = std::ptr::from_mut(self); ... Self::deref(p); +// let p = NonNull::from(self); ... Self::deref_nn(p); // // is banned. A release may be the object's last one, and then the destructor // frees the allocation the reference still points at. While the reference is @@ -76,23 +76,27 @@ const POINTER_FROM_REFERENCE = [ // rustfmt line break cannot hide it. const DIRECT = new RegExp(RELEASE + String.raw`\s*(?:` + POINTER_FROM_REFERENCE + ")", "g"); -// `let p = std::ptr::from_mut(self);` / `let p = self as *mut Self;`, whose -// binding is then released (`deref(p)`) further down the same function. The +// The same pointer stored first (`let p = std::ptr::from_mut(self);`, +// `let p = NonNull::from(self);`, ...) and released (`deref(p)`, +// `deref_nn(p)`, `deref(p.as_ptr())`) further down the same function. The // function ends at the next `fn` item; a closure inside it is still the same // function for this purpose. -const SELF_POINTER_BINDING = - /let\s+(?:mut\s+)?(\w+)\s*(?::[^=;]*)?=\s*(?:(?:std::|core::)?ptr::from_mut(?:::<[^>]*>)?\(\s*self\s*\)|self\s+as\s+\*mut\b[^;]*)\s*;/g; +const POINTER_BINDING = new RegExp( + String.raw`let\s+(?:mut\s+)?(\w+)\s*(?::[^=;]*)?=\s*(?:` + POINTER_FROM_REFERENCE + String.raw`)[^;]*;`, + "g", +); const FN_ITEM = /^[ \t]*(?:pub(?:\([^)]*\))?\s+)?(?:(?:const|async|unsafe|extern\s+"[^"]*")\s+)*fn\s/m; function releaseOfBinding(name: string): RegExp { - return new RegExp(RELEASE + String.raw`\s*` + name + String.raw`\s*[,)]`); + // `name` is a `\w+` capture, so it needs no escaping. + return new RegExp(RELEASE + String.raw`\s*` + name + String.raw`(?:\.as_ptr\(\))?\s*[,)]`); } /** Byte offsets (into `stripped`) of every banned release in one file. */ function findReleases(stripped: string): number[] { const hits: number[] = []; for (const m of stripped.matchAll(DIRECT)) hits.push(m.index); - for (const binding of stripped.matchAll(SELF_POINTER_BINDING)) { + for (const binding of stripped.matchAll(POINTER_BINDING)) { const start = binding.index + binding[0].length; const rest = stripped.slice(start); const fnEnd = rest.search(FN_ITEM); @@ -124,8 +128,8 @@ const ALLOW: Record = { // `finalize(&mut self)`, reached through the generated `JSSink` finalize // thunk, releases the wrapper's ref, which is the last one for an idle // sink; the second site releases the keep-alive ref while the wrapper's ref - // is still held. Needs the thunk to hand over the raw pointer; tracked - // separately. + // is still held. #37716 converts the thunk and removes both sites: drop + // this entry when it lands. "src/runtime/webcore/FileSink.rs": 2, // Harmless: every `close()` caller releases its own creation ref only after // `close()` returns. @@ -138,6 +142,9 @@ const ALLOW: Record = { // Harmless: `stmt` is a local reborrow and the queued request holds its own // ref on the statement until `release_statement`. "src/sql_jsc/postgres/PostgresSQLConnection.rs": 1, + // Harmless: `do_run`'s error paths undo the `ref_()` taken a few lines up + // while the on-stack JS wrapper holds its own ref. + "src/sql_jsc/postgres/PostgresSQLQuery.rs": 1, }; const counts: Record = {}; @@ -184,8 +191,13 @@ test("the patterns match the banned spellings and nothing else", () => { "let this_ptr = std::ptr::from_mut(self);\nif done {\n FetchTasklet::deref(this_ptr);\n return;\n}", "let this: *mut Self = self as *mut Self;\nSelf::deref(this);", "let this = std::ptr::from_mut(self);\nlet _guard = unsafe { ScopedRef::adopt(this) };", + "let this = NonNull::from(self);\nSelf::deref_nn(this);", + "let this = core::ptr::NonNull::from(self);\nunsafe { T::deref(this.as_ptr()) };", + "let p = std::ptr::from_ref::(this).cast_mut();\nunsafe { T::deref(p) };", + "let p = &raw mut *self;\nunsafe { RefCount::::deref(p) };", ]; const allowed = [ + "let this = NonNull::from(self);\nregister(this);", "FetchTasklet::deref_from_thread(task);", "let _js_ref = is_done.then(|| unsafe { ScopedRef::adopt(this) });", "drop(unsafe { ScopedRef::::adopt(task.as_ptr()) });", From 0b8e3e6ba8849219b5b12f9b381fcd71251b2edb Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:30:06 +0000 Subject: [PATCH 4/6] fetch: stash the allocation pointer for the deferred releases; end the sink through its pointer The two pointers start_request_stream stores for later (the sink's back-pointer and the promise ctx) were made from the hop's &mut, so the last-ref releases made through them later used a borrow that accesses through the allocation pointer had since invalidated (Miri rejects that under both models). The hop now passes its allocation pointer down and those two stashes are made from it; the releases that happen inside the frame keep going through self. FetchRequestBodySink::end_from_stream takes the sink pointer, like NetworkSink's, since its release can free the tasklet and the sink. Lint: also match a bare self argument (the &mut -> *mut coercion), not counting fn definitions; allowlist the sites that surfaces; state the spellings the lint does not see; drop the FileSink entry (#37716 landed). --- src/runtime/webcore.rs | 7 +- .../webcore/fetch/FetchRequestBodySink.rs | 40 +++++++--- src/runtime/webcore/fetch/FetchTasklet.rs | 42 ++++++---- .../self-receiver-release.test.ts | 77 +++++++++++++------ 4 files changed, 115 insertions(+), 51 deletions(-) diff --git a/src/runtime/webcore.rs b/src/runtime/webcore.rs index 271109c2f172..26229ed80b2d 100644 --- a/src/runtime/webcore.rs +++ b/src/runtime/webcore.rs @@ -413,9 +413,10 @@ impl SinkHandle { match *self { SinkHandle::None => {} SinkHandle::ServerResponse(any) => any.end_chunk(err.as_ref()), - // SAFETY: live backref; ByteStream clears sink before free. - SinkHandle::FetchRequestBody(mut p) => unsafe { p.get_mut() }.end_from_stream(err), - // Raw-ptr dispatch: may re-borrow and free the sink (see its doc). + // Raw-ptr dispatch for both: the call may free the sink (see their docs). + SinkHandle::FetchRequestBody(p) => { + fetch::FetchRequestBodySink::end_from_stream(p.as_ptr(), err) + } SinkHandle::S3Upload(p) => streams::NetworkSink::end_from_stream(p.as_ptr(), err), SinkHandle::FileSink(p) => p.end_from_stream(err), SinkHandle::HTMLRewriter(p) => p.end_from_stream(err), diff --git a/src/runtime/webcore/fetch/FetchRequestBodySink.rs b/src/runtime/webcore/fetch/FetchRequestBodySink.rs index cb4417051028..0c2958390e12 100644 --- a/src/runtime/webcore/fetch/FetchRequestBodySink.rs +++ b/src/runtime/webcore/fetch/FetchRequestBodySink.rs @@ -199,17 +199,39 @@ impl FetchRequestBodySink { )) } + /// JS entry (`end_from_js` / the JSSink forwarder). Only JS-pump sinks have + /// a JS object, so this only ever reaches the non-releasing branch. pub fn end(&mut self, err: Option) -> bun_sys::Result<()> { - self.end_from_stream(err.map(StreamError::Error)); + Self::end_from_stream(self, err.map(StreamError::Error)); bun_sys::Result::Ok(()) } /// Native-path terminator called from `SinkHandle::end`. Carries the full /// `StreamError` so a JS-valued upstream error (e.g. fetch reset) reaches /// `write_end_request(Some(js))` instead of being silently dropped to EOF. - pub fn end_from_stream(&mut self, err: Option) { - if self.ended { + /// + /// Takes the raw pointer, like `NetworkSink::end_from_stream`: the release + /// at the end may free the tasklet and, through `clear_sink`, this sink. + #[allow(clippy::not_unsafe_ptr_arg_deref)] + pub fn end_from_stream(this: *mut Self, err: Option) { + // SAFETY: `this` is the live sink behind the handle; the borrow is + // scoped to this call and nothing touches `*this` after it. + let Some((task, err_js)) = (unsafe { (*this).end_and_take_task(err) }) else { return; + }; + FetchTasklet::write_end_request(task.as_ptr(), err_js); + } + + /// Marks the sink ended. For a native source, returns the tasklet ref to + /// release (the one `start_request_stream` took) and the error to end it + /// with; the JS pump path closes the source instead and its pump promise + /// does the releasing. + fn end_and_take_task( + &mut self, + err: Option, + ) -> Option<(BackRef, Option)> { + if self.ended { + return None; } self.ended = true; if matches!( @@ -220,20 +242,16 @@ impl FetchRequestBodySink { // field; detach (not cancel) so we don't re-enter the source while // it is still on the stack (FileReader.on_reader_error ref-leak). self.source.clear(); - if let Some(task) = self.task.take() { - let err_js = err.map(|e| e.to_js(&task.global_this)); - // Releases the `start_request_stream` ref; that may free the - // tasklet and, through `clear_sink`, `*self`. - FetchTasklet::write_end_request(task.as_ptr(), err_js); - } - return; + let task = self.task.take()?; + let err_js = err.map(|e| e.to_js(&task.global_this)); + return Some((task, err_js)); } - // JS pump path: the assign_to_stream result handler is the single balancing release. let sys_err = match err { Some(StreamError::Error(e)) => Some(e), _ => None, }; self.source.close(sys_err); + None } pub fn end_from_js(&mut self, _global_this: &JSGlobalObject) -> bun_sys::Result { diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 50f248147e0d..b9c9edd9b8e2 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -314,12 +314,13 @@ impl FetchTasklet { /// INVARIANT: every `*mut FetchTasklet` threaded through the HTTP-thread /// callback (`callback`), the drain hook (`on_write_request_data_drain` / /// `resume_request_data_stream`), the progress hop (`on_progress_update`), - /// the request-body release (`write_end_request`), and the JS-thread - /// enqueue (`queue` → `node`) was produced by + /// the stashes `start_request_stream` makes for `write_end_request`, and + /// the JS-thread enqueue (`queue` → `node`) was produced by /// `heap::into_raw(Box)` in `get()` and is kept alive by the - /// intrusive `ref_count` until `deinit`. Access on either thread is - /// serialised: HTTP-thread writes happen under `mutex.lock()` and - /// JS-thread access is single-threaded. + /// intrusive `ref_count` until `deinit`; `write_end_request`'s in-frame + /// callers pass a pointer made from the `&mut` the hop currently holds. + /// Access on either thread is serialised: HTTP-thread writes happen under + /// `mutex.lock()` and JS-thread access is single-threaded. /// /// An entry point that owns a ref adopts it into a [`ScopedRef`] before /// calling this; the guard (which may free the tasklet) drops after the @@ -609,7 +610,11 @@ impl FetchTasklet { self.get_current_response().map(|r| unsafe { &mut *r }) } - fn start_request_stream(&mut self) { + /// `this` is the allocation pointer the hop was dispatched with; it is what + /// gets stashed past this frame (the sink's back-pointer and the promise + /// ctx), so the releases made through those later carry the allocation's + /// provenance rather than that of a borrow that has long ended. + fn start_request_stream(&mut self, this: *mut FetchTasklet) { self.is_waiting_request_stream_start = false; debug_assert!(matches!( self.request_body, @@ -631,8 +636,8 @@ impl FetchTasklet { // assign_to_stream-result side (on_resolve/on_reject or the synchronous // Fulfilled/Rejected/undefined branches below), or by the sink's // `finalize` as a fallback if that path never runs. The synchronous - // branches release it while our caller, the progress hop, still holds - // the JS-side ref. + // branches release it inside this frame, while our caller, the + // progress hop, still holds the JS-side ref; they go through `self`. self.ref_(); let self_ptr = std::ptr::from_mut::(self); @@ -651,10 +656,12 @@ impl FetchTasklet { return; } - // `self_ptr` is the live heap tasklet; the +1 above keeps it alive - // until `write_end_request`/`finalize` clears `task`. + // SAFETY: `this` is this tasklet's allocation pointer (see the fn doc); + // the +1 above keeps it live until `write_end_request` / `finalize` + // takes `task`. + let task = unsafe { bun_ptr::BackRef::from_raw_mut(this) }; let sink: &mut FetchRequestBodySink = Box::leak(Box::new(FetchRequestBodySink { - task: Some(bun_ptr::BackRef::new_mut(self)), + task: Some(task), high_water_mark: 16384, ..Default::default() })); @@ -709,7 +716,7 @@ impl FetchTasklet { bun_jsc::js_promise::Status::Pending => { assignment_result.then( &global_this, - self_ptr, + this, on_resolve_request_stream_shim, on_reject_request_stream_shim, ); @@ -926,12 +933,17 @@ impl FetchTasklet { let is_done = !shared.result.has_more; // SAFETY: `this` is live; the final hop owns the JS-side ref. let _js_ref = is_done.then(|| unsafe { ScopedRef::adopt(this) }); - Self::from_raw_mut(this).on_progress_update_locked(is_done) + Self::from_raw_mut(this).on_progress_update_locked(this, is_done) } /// Entered with `mutex` held; unlocks it on every path. The caller holds /// the JS-side ref throughout, so no release reached from here is the last. - fn on_progress_update_locked(&mut self, is_done: bool) -> JsTerminatedResult<()> { + /// `this` is only passed on to `start_request_stream` for stashing. + fn on_progress_update_locked( + &mut self, + this: *mut FetchTasklet, + is_done: bool, + ) -> JsTerminatedResult<()> { let vm = self.global_this.bun_vm(); // teardown forbade script: we cannot touch JS if !vm.script_allowed() { @@ -965,7 +977,7 @@ impl FetchTasklet { if self.is_waiting_request_stream_start && self.result.can_stream { // start streaming - self.start_request_stream(); + self.start_request_stream(this); // Makes wpt-h2 number-chunk test deterministic. // `assign_to_stream` kicks off `await reader.read()`; an invalid // chunk type (e.g. a JS number) throws inside `sink.write` and lands in diff --git a/test/internal/source-lints/self-receiver-release.test.ts b/test/internal/source-lints/self-receiver-release.test.ts index 3775649a0cab..c24cd67a5d94 100644 --- a/test/internal/source-lints/self-receiver-release.test.ts +++ b/test/internal/source-lints/self-receiver-release.test.ts @@ -7,6 +7,7 @@ import { globAllSources } from "../../../scripts/glob-sources.ts"; // An intrusive-refcount release applied to a pointer spelled from a reference // // FetchTasklet::deref(std::ptr::from_mut(self)); +// RefCount::::deref(self); // the same, by coercion // T::deref(std::ptr::from_ref(this).cast_mut()); // Self::deref_nn(NonNull::from(self)); // drop(ScopedRef::adopt(std::ptr::from_mut(self))); @@ -35,10 +36,21 @@ import { globAllSources } from "../../../scripts/glob-sources.ts"; // reference is a local reborrow rather than a parameter the release is not UB, // but the raw pointer it was made from is in scope; adopt that instead. // -// This lint knows the `deref` family of release names and `ScopedRef::adopt` -// (`ScopedRef::new` takes its own ref and is balanced, so it is not a release); -// a release spelled `unref`/`release` is out of its reach. Siblings: -// fn-long-mut-reborrow.test.ts, frozen-nonnull-reborrow.test.ts. +// Scope: this is a ratchet over the spellings a grep can see, namely the +// `deref` family of release names and `ScopedRef::adopt` applied to the +// receiver spelled as a pointer inline, to the receiver itself (a bare `self` +// argument to a `*mut` parameter is always a coerced reference; raw-pointer +// receivers do not exist outside bun_alloc), or to a local bound to one of +// those. It does not see the same release behind a helper, above all +// `deref(self.as_ctx_ptr())`, which is how the R-2 `&self` wrappers +// (Subprocess, sockets, websocket_client, the SQL connections, ...) spell it +// and which bun_ptr's `AsCtxPtr` doc currently endorses; nor a `&mut`-typed +// local, a guard (`ScopedRef::new`, `ref_scope`) whose drop happens to be the +// last release, or a release named `unref`/`release`. Those populations need +// their own audit; do not read a clean run here as proof that a type is free +// of the bug. Siblings: self-receiver-reclaim.test.ts (the same shape with a +// free instead of a release), fn-long-mut-reborrow.test.ts, +// frozen-nonnull-reborrow.test.ts. const root = path.resolve(import.meta.dir, "..", "..", ".."); const rustSources = globAllSources().rust.filter(p => p.endsWith(".rs")); @@ -56,10 +68,11 @@ const tracked: Set | null = (() => { return new Set(r.stdout.toString().split("\0").filter(Boolean)); })(); -// `deref(`, `deref_from_thread(`, `deref_nn(`, `deref_with_context(`, -// `rc_deref(`, however qualified (`\b` keeps `some_other_deref(` out), and -// `ScopedRef::adopt(` / `ScopedRef::::adopt(`. -const RELEASE = String.raw`(?:\b(?:rc_)?deref(?:_from_thread|_nn|_with_context)?|\bScopedRef(?:::<[^>]*>)?::adopt)\(`; +// A call to `deref(`, `deref_from_thread(`, `deref_nn(`, `deref_with_context(`, +// `rc_deref(`, however qualified (`\b` keeps `some_other_deref(` out), or to +// `ScopedRef::adopt(` / `ScopedRef::::adopt(`. The lookbehind keeps the +// definition `fn deref(self)` of a by-value release from counting as a call. +const RELEASE = String.raw`(?]*>)?::adopt)\(`; // A pointer spelled from a reference. `ptr::from_mut` / `ptr::from_ref` only // accept references, so any argument counts; the remaining spellings are @@ -72,17 +85,18 @@ const POINTER_FROM_REFERENCE = [ String.raw`&raw\s+mut\s+\*\s*self\b`, ].join("|"); -// Release applied directly to such a pointer. `\s*` between the two so a -// rustfmt line break cannot hide it. -const DIRECT = new RegExp(RELEASE + String.raw`\s*(?:` + POINTER_FROM_REFERENCE + ")", "g"); +// Release applied directly to such a pointer, or to `self` itself, which the +// `*mut` parameter coerces. `\s*` between the two so a rustfmt line break +// cannot hide it. +const DIRECT = new RegExp(RELEASE + String.raw`\s*(?:` + POINTER_FROM_REFERENCE + String.raw`|self\s*(?=[,)]))`, "g"); // The same pointer stored first (`let p = std::ptr::from_mut(self);`, -// `let p = NonNull::from(self);`, ...) and released (`deref(p)`, -// `deref_nn(p)`, `deref(p.as_ptr())`) further down the same function. The -// function ends at the next `fn` item; a closure inside it is still the same -// function for this purpose. +// `let p = NonNull::from(self);`, `let p: *mut Self = self;`, ...) and +// released (`deref(p)`, `deref_nn(p)`, `deref(p.as_ptr())`) further down the +// same function. The function ends at the next `fn` item; a closure inside it +// is still the same function for this purpose. const POINTER_BINDING = new RegExp( - String.raw`let\s+(?:mut\s+)?(\w+)\s*(?::[^=;]*)?=\s*(?:` + POINTER_FROM_REFERENCE + String.raw`)[^;]*;`, + String.raw`let\s+(?:mut\s+)?(\w+)\s*(?::[^=;]*)?=\s*(?:(?:` + POINTER_FROM_REFERENCE + String.raw`)[^;]*|self\s*);`, "g", ); const FN_ITEM = /^[ \t]*(?:pub(?:\([^)]*\))?\s+)?(?:(?:const|async|unsafe|extern\s+"[^"]*")\s+)*fn\s/m; @@ -116,6 +130,20 @@ function lineOf(text: string, offset: number): number { // harmless (another ref provably outlives the call) or tracked for its own // fix. Lower the count when you convert one; do not add entries. const ALLOW: Record = { + // `on_close` releases under a `ref_scope` guard (that guard's own drop is + // then the last release, still inside `&mut self`); `maybe_release`'s + // close branch releases what can be the last ref. Tracked separately. + "src/http/h2_client/ClientSession.rs": 2, + // Harmless: `detach` drops a per-stream ref while the session's owner holds + // its own. + "src/http/h3_client/ClientSession.rs": 1, + // `detach_and_deref(&mut self)` can drop the tunnel's last ref. Tracked + // separately. + "src/http/ProxyTunnel.rs": 1, + // `on_reader_done` / `on_reader_error(&mut self)` release the reader's own + // ref right after `on_close_io` dropped the owner's, so it frees the reader; + // the parent macro has the raw pointer to pass instead. Tracked separately. + "src/runtime/api/bun/subprocess/SubprocessPipeReader.rs": 2, // Harmless: guarded by `ref_count > 1`; the final release goes through // `DeferredDerefTask` precisely because the caller still uses the object. "src/runtime/api/html_rewriter.rs": 1, @@ -125,12 +153,6 @@ const ALLOW: Record = { // Harmless: every `disarm` caller (`arm`, `finalize`, `deinit`) holds or // has already consumed its own ref across the call. "src/runtime/valkey_jsc/js_valkey.rs": 1, - // `finalize(&mut self)`, reached through the generated `JSSink` finalize - // thunk, releases the wrapper's ref, which is the last one for an idle - // sink; the second site releases the keep-alive ref while the wrapper's ref - // is still held. #37716 converts the thunk and removes both sites: drop - // this entry when it lands. - "src/runtime/webcore/FileSink.rs": 2, // Harmless: every `close()` caller releases its own creation ref only after // `close()` returns. "src/spawn/process.rs": 1, @@ -195,9 +217,20 @@ test("the patterns match the banned spellings and nothing else", () => { "let this = core::ptr::NonNull::from(self);\nunsafe { T::deref(this.as_ptr()) };", "let p = std::ptr::from_ref::(this).cast_mut();\nunsafe { T::deref(p) };", "let p = &raw mut *self;\nunsafe { RefCount::::deref(p) };", + "unsafe { RefCount::::deref(self) };", + "unsafe { PipeReader::deref(self) }", + "drop(unsafe { ScopedRef::adopt(self) });", + "let this: *mut Self = self;\nunsafe { Self::deref(this) };", ]; const allowed = [ "let this = NonNull::from(self);\nregister(this);", + // Not seen, by the scope note at the top; listed so the boundary is explicit. + "unsafe { Self::deref(self.as_ctx_ptr()) };", + "let x = self.field;\nSelf::deref(x);", + "self.deref();", + // Definitions of by-value releases, not calls. + "pub fn deref(self) {\n dispatch!(self, (), |_T, ctx| ctx.deref())\n}", + "unsafe fn rc_deref(self, ctx: ()) {}", "FetchTasklet::deref_from_thread(task);", "let _js_ref = is_done.then(|| unsafe { ScopedRef::adopt(this) });", "drop(unsafe { ScopedRef::::adopt(task.as_ptr()) });", From 639f783ceda9242e6e1534bf2ea4177f53c7d6ba Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:48:41 +0000 Subject: [PATCH 5/6] fetch: let the sink's JS end() skip the releasing entry point instead of explaining why it never releases --- .../webcore/fetch/FetchRequestBodySink.rs | 18 ++++++++---------- src/runtime/webcore/fetch/FetchTasklet.rs | 7 +++---- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/runtime/webcore/fetch/FetchRequestBodySink.rs b/src/runtime/webcore/fetch/FetchRequestBodySink.rs index 0c2958390e12..a718933ffbdf 100644 --- a/src/runtime/webcore/fetch/FetchRequestBodySink.rs +++ b/src/runtime/webcore/fetch/FetchRequestBodySink.rs @@ -199,19 +199,19 @@ impl FetchRequestBodySink { )) } - /// JS entry (`end_from_js` / the JSSink forwarder). Only JS-pump sinks have - /// a JS object, so this only ever reaches the non-releasing branch. + /// JS entry: only a JS-pump sink has a JS object, and its release belongs + /// to the pump promise, so there is never a ref to release here. pub fn end(&mut self, err: Option) -> bun_sys::Result<()> { - Self::end_from_stream(self, err.map(StreamError::Error)); + let release = self.end_and_take_task(err.map(StreamError::Error)); + debug_assert!(release.is_none()); bun_sys::Result::Ok(()) } /// Native-path terminator called from `SinkHandle::end`. Carries the full /// `StreamError` so a JS-valued upstream error (e.g. fetch reset) reaches /// `write_end_request(Some(js))` instead of being silently dropped to EOF. - /// - /// Takes the raw pointer, like `NetworkSink::end_from_stream`: the release - /// at the end may free the tasklet and, through `clear_sink`, this sink. + /// Raw pointer like `NetworkSink::end_from_stream`: the release may free + /// the tasklet and, through `clear_sink`, this sink. #[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn end_from_stream(this: *mut Self, err: Option) { // SAFETY: `this` is the live sink behind the handle; the borrow is @@ -222,10 +222,8 @@ impl FetchRequestBodySink { FetchTasklet::write_end_request(task.as_ptr(), err_js); } - /// Marks the sink ended. For a native source, returns the tasklet ref to - /// release (the one `start_request_stream` took) and the error to end it - /// with; the JS pump path closes the source instead and its pump promise - /// does the releasing. + /// Marks the sink ended; for a native source, hands back the tasklet ref + /// `start_request_stream` took and the error to end the request with. fn end_and_take_task( &mut self, err: Option, diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index b9c9edd9b8e2..58da7c7a6834 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -610,10 +610,9 @@ impl FetchTasklet { self.get_current_response().map(|r| unsafe { &mut *r }) } - /// `this` is the allocation pointer the hop was dispatched with; it is what - /// gets stashed past this frame (the sink's back-pointer and the promise - /// ctx), so the releases made through those later carry the allocation's - /// provenance rather than that of a borrow that has long ended. + /// `this` is the allocation pointer; it is what gets stashed past this frame + /// (the sink's back-pointer, the promise ctx), so the later releases through + /// those carry the allocation's provenance, not a dead borrow's. fn start_request_stream(&mut self, this: *mut FetchTasklet) { self.is_waiting_request_stream_start = false; debug_assert!(matches!( From 939b42e0041ff455251863145b6f4f9293dfbeec Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 19:47:08 +0000 Subject: [PATCH 6/6] ci: retrigger