Skip to content

blob: delete Blob::deinit, which freed the allocation through &mut self #37672

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

blob: delete Blob::deinit, which freed the allocation through &mut self #37672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 15 additions & 27 deletions src/jsc/webcore_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,11 @@ const _: () = {
};

impl Blob {
/// Heap-promote and mark as
/// heap-allocated so `deinit` knows to free the heap box.
/// Heap-promote with one count. The allocation is freed by [`Blob__deref`]
/// releasing the last count (JS wrapper finalizer, C++ `BlobRefPtr`,
/// [`bun_ptr::ExternalShared`]) and by nothing else. A `Blob` that is not
/// promoted needs no teardown call: its store ref, name and content type
/// are released by their own drops.
#[inline]
pub fn new(mut blob: Blob) -> *mut Blob {
blob.ref_count = bun_ptr::RawRefCount::init(1);
Expand All @@ -234,8 +237,8 @@ impl Blob {
);
// SAFETY: `self` is the allocation `Blob::new` produced and the JS
// wrapper's `+1` is the count released here. `into_raw` hands the box
// over without dropping it; `Blob__deref` runs `deinit()` (which
// `drop(heap::take)`s) when the count reaches zero.
// over without dropping it; `Blob__deref` frees the allocation when
// the count reaches zero.
unsafe { Blob__deref(bun_core::heap::into_raw(self)) }
}

Expand Down Expand Up @@ -450,21 +453,6 @@ impl Blob {
store::Data::S3(s3) => Some(s3.path()),
}
}

/// Tear down owned resources; if
/// heap-allocated, also frees the heap box.
pub fn deinit(&mut self) {
self.detach();
self.name.set(bun_core::String::dead());

self.content_type.set(BlobContentType::default());

if self.is_heap_allocated() {
// SAFETY: `self` is the `*mut Blob` originally produced by
// `Blob::new` (`heap::alloc`).
unsafe { drop(bun_core::heap::take(std::ptr::from_mut::<Blob>(self))) };
}
}
}

// SAFETY: `Blob__ref`/`Blob__deref` operate on the intrusive `ref_count` and
Expand All @@ -487,8 +475,9 @@ unsafe impl bun_ptr::ExternalSharedDescriptor for Blob {
/// # Safety
/// `this` must point to a live `Blob` produced by [`Blob::new`], and the call
/// must happen on the thread that owns it (the count is not atomic). A
/// by-value `Blob` has a count of zero; bumping it would make a later
/// [`Blob::deinit`] free an address that was never heap-allocated.
/// by-value `Blob` has a count of zero; giving it one would make the
/// [`Blob__deref`] that later releases it free an address that was never
/// heap-allocated.
#[unsafe(no_mangle)]
unsafe extern "C" fn Blob__ref(this: *mut Blob) {
// SAFETY: caller contract above.
Expand All @@ -512,18 +501,17 @@ unsafe extern "C" fn Blob__ref(this: *mut Blob) {
/// last count frees the `Blob`, so `this` is dangling once this returns.
#[unsafe(no_mangle)]
unsafe extern "C" fn Blob__deref(this: *mut Blob) {
// SAFETY: caller contract above. `deinit` frees the allocation, so `this`
// is not touched after it.
// SAFETY: caller contract above: `this` is the `Blob::new` allocation and
// the count released here was owned by the caller. When it was the last
// one nothing else refers to the allocation, so the box is freed here;
// dropping it releases the store ref, name and content type.
unsafe {
debug_assert!(
(*this).is_heap_allocated(),
"cannot deref: this Blob is not heap-allocated"
);
if (*this).ref_count.decrement() == bun_ptr::raw_ref_count::DecrementResult::ShouldDestroy {
// `deinit` has its own `is_heap_allocated()` guard around the
// `drop(heap::take)`, so re-arm so it returns true.
(*this).ref_count.increment();
(*this).deinit();
bun_core::heap::destroy(this);
}
}
}
Expand Down
12 changes: 3 additions & 9 deletions src/runtime/jsc_hooks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4473,16 +4473,10 @@ unsafe fn transpile_file(
return ptr::null_mut();
}
};
// Deinit the blob (if any) on scope exit.
// Note: reshaped for borrowck — capture the `is_some()` flag *before*
// moving the option into the scopeguard so the `transpile_async` predicate
// can still read it without aliasing the guard's `&mut`.
// `blob_to_deinit` stays alive (and so does the store `lr` /
// `virtual_source_to_use` point into) until this function returns, where
// its drop releases the store.
let had_blob = blob_to_deinit.is_some();
let _blob_guard = scopeguard::guard(blob_to_deinit, |mut slot| {
if let Some(mut blob) = slot.take() {
blob.deinit();
}
});

// ── force_loader / require.extensions override ──────────────────────────
if let Some(loader_type) = force_loader_type {
Expand Down
11 changes: 0 additions & 11 deletions src/runtime/server/FileRoute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ use crate::webcore::body::Value as BodyValue;
use crate::webcore::{Blob, FetchHeaders, Response};

#[derive(bun_ptr::CellRefCounted)]
#[ref_count(destroy = FileRoute::deinit)]
pub struct FileRoute {
// Owned via intrusive refcount; the
// raw `*mut FileRoute` is round-tripped through `FileResponseStream`'s
Expand Down Expand Up @@ -131,16 +130,6 @@ impl FileRoute {
}))
}

fn deinit(this: *mut FileRoute) {
// SAFETY: `this` was allocated via heap::alloc in init_from_blob/from_js and the
// intrusive ref_count has reached 0.
// `headers` is freed by its own Drop when the Box is dropped.
unsafe {
(*this).blob.deinit();
drop(bun_core::heap::take(this));
}
}

pub fn from_js(global: &JSGlobalObject, argument: JSValue) -> JsResult<Option<*mut FileRoute>> {
// `as_class_ref` is the safe shared-borrow downcast (one audited
// unsafe in `JSValue`); `get_body_value`/`get_init_headers`/
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/shell/Builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ impl Drop for PinnedArrayBuf {
}

/// Refcounted wrapper around a `webcore.Blob`. `Arc` provides the refcount;
/// `Drop` runs `Blob::deinit`.
/// dropping the last one releases the blob's store.
pub struct BuiltinBlob {
pub(crate) blob: crate::webcore::Blob,
}
Expand Down
61 changes: 17 additions & 44 deletions src/runtime/webcore/Blob.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ pub use bun_jsc::generated::JSBlob as js;

// ──────────────────────────────────────────────────────────────────────────
// BlobExt — `bun_runtime`-tier behaviour layered on the `bun_jsc` data type.
// Inherent methods (`new`/`init`/`shared_view`/`dupe`/`detach`/`deinit`/…)
// Inherent methods (`new`/`init`/`shared_view`/`dupe`/`detach`/…)
// live on `bun_jsc::webcore_types::Blob`; everything that touches the event
// loop / S3 / fs / `VirtualMachine` is here.
// ──────────────────────────────────────────────────────────────────────────
Expand Down Expand Up @@ -584,7 +584,6 @@ impl BlobExt for Blob {
impl<H: ReadBytesHandler> Task<H> {
fn done(mut self: Box<Self>, r: ReadBytesResult) {
self.poll.unref(bun_io::js_vm_ctx());
self.blob.deinit();
let ctx = self.ctx;
drop(self);
// SAFETY: `ctx` is the pointer handed to `read_bytes_to_handler`;
Expand Down Expand Up @@ -650,7 +649,7 @@ impl BlobExt for Blob {
payer = s3.request_payer;
}
// SAFETY: `path` borrows the store held by `t.blob` (a fresh +1 ref);
// it stays valid until `Task::done` deinits the blob in the callback.
// it stays valid until `Task::done` drops the task in the callback.
let path = unsafe { &*path };
let t_ptr = bun_core::heap::into_raw(t).cast::<c_void>();
if self.offset.get() > 0 || self.size.get() != MAX_SIZE {
Expand Down Expand Up @@ -4075,16 +4074,15 @@ fn on_structured_clone_deserialize<B: AsRef<[u8]>>(
// Bun.file() keeps its window's end. MAX_SIZE means unknown.
let mut file_size: Option<u64> = None;

let blob: *mut Blob = match store_tag {
store::SerializeTag::Bytes => 'bytes: {
// `blob` stays by value until every field has been read: an early return
// below drops it, which releases its store (and with it the payload
// bytes). It is heap-promoted only once the record has fully parsed.
let blob: Blob = match store_tag {
store::SerializeTag::Bytes => {
let bytes_len = reader.read_int_le::<u32>()?;
let bytes = read_slice(reader, bytes_len as usize)?;

let blob = Blob::init(bytes, global_this);
// `blob` now owns `bytes` (via its Store when non-empty). If any
// of the remaining reads fail before we heap-promote it, Drop on
// `blob` releases the store so the payload bytes don't leak.
let guard = scopeguard::guard(blob, |mut b| b.deinit());

'versions: {
if version == 1 {
Expand All @@ -4094,8 +4092,7 @@ fn on_structured_clone_deserialize<B: AsRef<[u8]>>(
let name_len = reader.read_int_le::<u32>()?;
let name = read_slice(reader, name_len as usize)?;

// ScopeGuard derefs to its inner Blob.
if let Some(store) = (*guard).store() {
if let Some(store) = blob.store() {
if let store::Data::Bytes(bytes_store) = &mut store.data_mut() {
// Transfer ownership of the local `name: Vec<u8>` into
// `stored_name` (a `Box<[u8]>`); freed by `Bytes::Drop`.
Expand All @@ -4109,10 +4106,9 @@ fn on_structured_clone_deserialize<B: AsRef<[u8]>>(
}
}

let blob = scopeguard::ScopeGuard::into_inner(guard);
break 'bytes Blob::new(blob);
blob
}
store::SerializeTag::File => 'file: {
store::SerializeTag::File => {
use crate::node::types::PathOrFileDescriptorSerializeTag;
if version >= 4 {
file_size = Some(reader.read_int_le::<u64>()?);
Expand All @@ -4132,11 +4128,7 @@ fn on_structured_clone_deserialize<B: AsRef<[u8]>>(
return Err(crate::Error::InvalidValue);
}
let mut path_or_fd = PathOrFileDescriptor::Fd(fd);
break 'file Blob::new(Blob::find_or_create_file_from_path(
&mut path_or_fd,
global_this,
true,
));
Blob::find_or_create_file_from_path(&mut path_or_fd, global_this, true)
}
PathOrFileDescriptorSerializeTag::Path => {
let path_len = reader.read_int_le::<u32>()?;
Expand All @@ -4154,25 +4146,12 @@ fn on_structured_clone_deserialize<B: AsRef<[u8]>>(
let mut dest = PathOrFileDescriptor::Path(node::PathLike::String(
bun_ptr::cow_slice::CowSlice::init_owned(path.into_boxed_slice()),
));
break 'file Blob::new(Blob::find_or_create_file_from_path(
&mut dest,
global_this,
true,
));
Blob::find_or_create_file_from_path(&mut dest, global_this, true)
}
}
}
store::SerializeTag::Empty => Blob::new(Blob::init_empty(global_this)),
store::SerializeTag::Empty => Blob::init_empty(global_this),
};
// `blob` is heap-allocated past this point; on any remaining error
// (truncated trailer fields) tear down both the heap object and its
// store. `content_type` is handled by its own Drop above since it
// hasn't been attached to `blob` yet.
// SAFETY: blob is a freshly-allocated heap pointer from Blob::new.
let blob_guard = scopeguard::guard(blob, |b| unsafe { (*b).deinit() });
// SAFETY: `blob_guard` holds the sole pointer to the fresh heap allocation.
// Shared access only — Blob state is Cell/JsCell-based.
let blob = unsafe { &**blob_guard };

'versions: {
if version == 1 {
Expand All @@ -4198,11 +4177,6 @@ fn on_structured_clone_deserialize<B: AsRef<[u8]>>(
}
}

debug_assert!(
blob.is_heap_allocated(),
"expected blob to be heap-allocated"
);

// `offset` comes from untrusted bytes. Clamp it so a crafted payload cannot
// make shared_view() slice past the end of the backing store (OOB heap read).
blob.offset.set(offset as SizeType); // intentional truncate
Expand All @@ -4229,9 +4203,9 @@ fn on_structured_clone_deserialize<B: AsRef<[u8]>>(
blob.content_type_was_set.set(content_type_was_set);
}

let blob_ptr = scopeguard::ScopeGuard::into_inner(blob_guard);
// SAFETY: blob_ptr is valid; toJS is infallible. Explicit `&mut *` forces
// the inherent `Blob::to_js(&mut self)` over `JsClass::to_js(self)`.
let blob_ptr = Blob::new(blob);
// SAFETY: `blob_ptr` is the fresh `Blob::new` allocation; `to_js` hands it
// to the JS wrapper, whose finalizer releases the count `new` gave it.
Ok(unsafe { BlobExt::to_js(&*blob_ptr, global_this) })
}

Expand Down Expand Up @@ -5881,9 +5855,8 @@ impl S3BlobDownloadTask {

impl Drop for S3BlobDownloadTask {
fn drop(&mut self) {
Blob::deinit(&mut self.blob);
self.poll_ref.unref(bun_io::js_vm_ctx());
// promise: Drop handles deinit.
// `blob` and `promise` are released by their own drops.
}
}

Expand Down
20 changes: 10 additions & 10 deletions src/runtime/webcore/Body.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,8 +768,7 @@ impl Value {
Value::Empty => ReadableStream::empty(global_this),
Value::Null => Ok(JSValue::NULL),
Value::InternalBlob(_) | Value::Blob(_) | Value::WTFStringImpl(_) => {
// `deinit` must run on every exit incl. `?` paths.
let blob = scopeguard::guard(self.use_(), |mut b| b.deinit());
let blob = self.use_();
blob.resolve_size();
let blob_size = blob.size.get();
let value = ReadableStream::from_blob_copy_ref(global_this, &blob, blob_size)?;
Expand Down Expand Up @@ -821,7 +820,7 @@ impl Value {
}
Value::Blob(_) => {
let stream = {
let blob = scopeguard::guard(self.use_(), |mut b| b.deinit());
let blob = self.use_();
blob.resolve_size();
if blob.needs_to_read_file() || blob.is_s3() {
let blob_size = blob.size.get();
Expand Down Expand Up @@ -1216,8 +1215,8 @@ impl Value {
match self {
Value::Blob(b) => {
// `Value` has `Drop`, so we cannot move the `Blob` out by
// value (E0509). `mem::take` leaves a default `Blob` whose `deinit()`
// (run by `Value::drop` on the assignment below) is a no-op.
// value (E0509). `mem::take` leaves a default `Blob`, which
// owns nothing, for the assignment below to drop.
let new_blob = core::mem::take(b);
*self = Value::Used;
debug_assert!(!new_blob.is_heap_allocated()); // owned by Body
Expand Down Expand Up @@ -1429,8 +1428,9 @@ impl Value {
}
return;
}
// Assignment runs `Drop` on the old variant: deref WTFStringImpl, deinit
// Blob, free InternalBlob's Vec, reset Error. Null/Used/Empty are no-ops.
// Assignment drops the old variant: deref WTFStringImpl, release the
// Blob's store, free InternalBlob's Vec, reset Error. Null/Used/Empty
// are no-ops.
*self = Value::Null;
}
}
Expand All @@ -1454,10 +1454,10 @@ impl Drop for Value {
}
}
Value::WTFStringImpl(s) => wtf_impl(s).deref(),
Value::Blob(b) => b.deinit(),
Value::Error(e) => e.reset(),
// `InternalBlob`'s `Vec<u8>` is freed by the compiler's drop glue.
Value::InternalBlob(_) | Value::Used | Value::Empty | Value::Null => {}
// `Blob`'s store ref / name / content type and `InternalBlob`'s
// `Vec<u8>` are freed by the compiler's drop glue.
Value::Blob(_) | Value::InternalBlob(_) | Value::Used | Value::Empty | Value::Null => {}
}
}
}
Expand Down
7 changes: 0 additions & 7 deletions src/runtime/webcore/ObjectURLRegistry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,6 @@ impl Entry {
}
}

impl Drop for Entry {
fn drop(&mut self) {
self.blob.deinit();
// The allocation itself is freed by the `Box<Entry>` drop.
}
}

impl ObjectURLRegistry {
pub(crate) fn register(&self, vm: &mut VirtualMachine, blob: &Blob) -> UUID {
let uuid = vm.rare_data().next_uuid();
Expand Down
13 changes: 5 additions & 8 deletions test/internal/source-lints/self-receiver-reclaim.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,9 @@ const SELF_AS_POINTER = [
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,
};
// shape. Empty by design: the whole tree is at zero. Prefer converting over
// adding an entry here.
const ALLOW: Record<string, number> = {};

const counts: Record<string, number> = {};
const offenders: string[] = [];
Expand Down Expand Up @@ -122,7 +118,8 @@ test("the pattern recognizes the spellings it claims to", () => {
// `<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)`.
// `Blob::deinit(&mut self)`, as it was before it was deleted in favour of
// the field drops.
"unsafe { drop(bun_core::heap::take(std::ptr::from_mut::<Blob>(self))) };",
"unsafe { bun_core::heap::destroy(self) };",
"drop(unsafe { Box::from_raw(self) });",
Expand Down
4 changes: 2 additions & 2 deletions test/internal/source-lints/unsafe-refcount-exports.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import { globAllSources } from "../../../scripts/glob-sources.ts";
// Adjusting an intrusive count is only sound when the caller holds a count on
// an object that is actually refcounted, and no signature can prove that:
// releasing a count nobody owns frees the object out from under its owner, and
// bumping the count of a by-value instance turns its ordinary teardown into a
// free of a non-heap address. That obligation has to be an `unsafe` contract on
// bumping the count of a by-value instance makes the release that later
// balances it free a non-heap address. That obligation has to be an `unsafe` contract on
// the export itself, because the same symbol is callable from Rust (the
// `ExternalSharedDescriptor` impl, finalizers) as well as from the C++
// `RefDerefTraits` it exists for.
Expand Down
Loading