From e87c0014b4a68357ba5da80ee85b3937d4bc0de8 Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Sat, 18 Jan 2025 01:23:55 +0000 Subject: [PATCH] fix(allocator): statically prevent memory leaks in allocator (#8570) Prevent memory leaks by statically preventing `Drop` types from being allocated in the arena. Attempting to allocate any `Drop` type in the arena now produces a compilation failure. The stabilization of `const {}` blocks in Rust 1.79.0 gave the mechanism required to enforce this at compile time without a mess of generics and traits, and in a way which should not hurt compile times (and zero runtime cost). This PR is what discovered `CompactString`s being stored in arena in the mangler (fixed in #8557). Note: The compilation failure occurs in `cargo build` not `cargo check`. So unfortunately errors don't appear in Rust Analyser, only when you run `cargo build`. From what I've read, stable Rust does not offer any solution to this at present. But the errors are reasonably clear what the problem is, and point to the line where it occurs. --- crates/oxc_allocator/src/boxed.rs | 27 +++++++++++++++----- crates/oxc_allocator/src/hash_map.rs | 26 ++++++++++++++----- crates/oxc_allocator/src/lib.rs | 38 +++++++++++++++++++++------- crates/oxc_allocator/src/vec.rs | 32 ++++++++++++++++++----- 4 files changed, 96 insertions(+), 27 deletions(-) diff --git a/crates/oxc_allocator/src/boxed.rs b/crates/oxc_allocator/src/boxed.rs index c5e1e05aa79b8..68a4a133485c1 100644 --- a/crates/oxc_allocator/src/boxed.rs +++ b/crates/oxc_allocator/src/boxed.rs @@ -7,6 +7,7 @@ use std::{ fmt::{self, Debug, Formatter}, hash::{Hash, Hasher}, marker::PhantomData, + mem::needs_drop, ops::{self, Deref}, ptr::{self, NonNull}, }; @@ -18,14 +19,16 @@ use crate::Allocator; /// A `Box` without [`Drop`], which stores its data in the arena allocator. /// -/// Should only be used for storing AST types. +/// ## No `Drop`s /// -/// Must NOT be used to store types which have a [`Drop`] implementation. -/// `T::drop` will NOT be called on the `Box`'s contents when the `Box` is dropped. -/// If `T` owns memory outside of the arena, this will be a memory leak. +/// Objects allocated into Oxc memory arenas are never [`Dropped`](Drop). Memory is released in bulk +/// when the allocator is dropped, without dropping the individual objects in the arena. /// -/// Note: This is not a soundness issue, as Rust does not support relying on `drop` -/// being called to guarantee soundness. +/// Therefore, it would produce a memory leak if you allocated [`Drop`] types into the arena +/// which own memory allocations outside the arena. +/// +/// Static checks make this impossible to do. [`Box::new_in`] will refuse to compile if called +/// with a [`Drop`] type. pub struct Box<'alloc, T: ?Sized>(NonNull, PhantomData<(&'alloc (), T)>); impl Box<'_, T> { @@ -68,6 +71,10 @@ impl Box<'_, T> { /// let in_arena: Box = Box::new_in(5, &arena); /// ``` pub fn new_in(value: T, allocator: &Allocator) -> Self { + const { + assert!(!needs_drop::(), "Cannot create a Box where T is a Drop type"); + } + Self(NonNull::from(allocator.alloc(value)), PhantomData) } @@ -78,6 +85,10 @@ impl Box<'_, T> { /// Only purpose is for mocking types without allocating for const assertions. #[allow(unsafe_code, clippy::missing_safety_doc)] pub const unsafe fn dangling() -> Self { + const { + assert!(!needs_drop::(), "Cannot create a Box where T is a Drop type"); + } + Self(NonNull::dangling(), PhantomData) } } @@ -97,6 +108,10 @@ impl Box<'_, T> { /// `ptr` must have been created from a `*mut T` or `&mut T` (not a `*const T` / `&T`). #[inline] pub(crate) const unsafe fn from_non_null(ptr: NonNull) -> Self { + const { + assert!(!needs_drop::(), "Cannot create a Box where T is a Drop type"); + } + Self(ptr, PhantomData) } diff --git a/crates/oxc_allocator/src/hash_map.rs b/crates/oxc_allocator/src/hash_map.rs index cded3efcd9ada..acc5ce8bfe7d9 100644 --- a/crates/oxc_allocator/src/hash_map.rs +++ b/crates/oxc_allocator/src/hash_map.rs @@ -9,7 +9,7 @@ use std::{ hash::Hash, - mem::ManuallyDrop, + mem::{needs_drop, ManuallyDrop}, ops::{Deref, DerefMut}, }; @@ -36,12 +36,16 @@ type FxHashMap<'alloc, K, V> = hashbrown::HashMap(ManuallyDrop>); @@ -56,6 +60,11 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> { /// until it is first inserted into. #[inline(always)] pub fn new_in(allocator: &'alloc Allocator) -> Self { + const { + assert!(!needs_drop::(), "Cannot create a HashMap where K is a Drop type"); + assert!(!needs_drop::(), "Cannot create a HashMap where V is a Drop type"); + } + let inner = FxHashMap::with_hasher_in(FxBuildHasher, allocator.bump()); Self(ManuallyDrop::new(inner)) } @@ -66,6 +75,11 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> { /// If capacity is 0, the hash map will not allocate. #[inline(always)] pub fn with_capacity_in(capacity: usize, allocator: &'alloc Allocator) -> Self { + const { + assert!(!needs_drop::(), "Cannot create a HashMap where K is a Drop type"); + assert!(!needs_drop::(), "Cannot create a HashMap where V is a Drop type"); + } + let inner = FxHashMap::with_capacity_and_hasher_in(capacity, FxBuildHasher, allocator.bump()); Self(ManuallyDrop::new(inner)) diff --git a/crates/oxc_allocator/src/lib.rs b/crates/oxc_allocator/src/lib.rs index f446c3f4c6ac6..0e3396cb41eb8 100644 --- a/crates/oxc_allocator/src/lib.rs +++ b/crates/oxc_allocator/src/lib.rs @@ -5,27 +5,41 @@ //! memory management data types from `std` adapted to use this arena. //! //! ## No `Drop`s -//! Objects allocated into oxc memory arenas are never [`Dropped`](Drop), making -//! it relatively easy to leak memory if you're not careful. Memory is released -//! in bulk when the allocator is dropped. +//! +//! Objects allocated into Oxc memory arenas are never [`Dropped`](Drop). +//! Memory is released in bulk when the allocator is dropped, without dropping the individual +//! objects in the arena. +//! +//! Therefore, it would produce a memory leak if you allocated [`Drop`] types into the arena +//! which own memory allocations outside the arena. +//! +//! Static checks make this impossible to do. [`Allocator::alloc`], [`Box::new_in`], [`Vec::new_in`], +//! and all other methods which store data in the arena will refuse to compile if called with +//! a [`Drop`] type. //! //! ## Examples -//! ``` +//! +//! ```ignore //! use oxc_allocator::{Allocator, Box}; //! //! struct Foo { //! pub a: i32 //! } +//! //! impl std::ops::Drop for Foo { -//! fn drop(&mut self) { -//! // Arena boxes are never dropped. -//! unreachable!(); -//! } +//! fn drop(&mut self) {} +//! } +//! +//! struct Bar { +//! v: std::vec::Vec, //! } //! //! let allocator = Allocator::default(); +//! +//! // This will fail to compile because `Foo` implements `Drop` //! let foo = Box::new_in(Foo { a: 0 }, &allocator); -//! drop(foo); +//! // This will fail to compile because `Bar` contains a `std::vec::Vec`, and it implements `Drop` +//! let bar = Box::new_in(Bar { v: vec![1, 2, 3] }, &allocator); //! ``` //! //! Consumers of the [`oxc` umbrella crate](https://crates.io/crates/oxc) pass @@ -41,6 +55,8 @@ #![warn(missing_docs)] +use std::mem::needs_drop; + use bumpalo::Bump; mod address; @@ -92,6 +108,10 @@ impl Allocator { #[expect(clippy::inline_always)] #[inline(always)] pub fn alloc(&self, val: T) -> &mut T { + const { + assert!(!needs_drop::(), "Cannot allocate Drop type in arena"); + } + self.bump.alloc(val) } diff --git a/crates/oxc_allocator/src/vec.rs b/crates/oxc_allocator/src/vec.rs index 3589a4b32c46d..08c4168b1eb3c 100644 --- a/crates/oxc_allocator/src/vec.rs +++ b/crates/oxc_allocator/src/vec.rs @@ -9,7 +9,7 @@ use std::{ self, fmt::{self, Debug}, hash::{Hash, Hasher}, - mem::ManuallyDrop, + mem::{needs_drop, ManuallyDrop}, ops, ptr::NonNull, slice::SliceIndex, @@ -25,12 +25,16 @@ use crate::{Allocator, Box, String}; /// A `Vec` without [`Drop`], which stores its data in the arena allocator. /// -/// Must NOT be used to store types which have a [`Drop`] implementation. -/// `T::drop` will NOT be called on the `Vec`'s contents when the `Vec` is dropped. -/// If `T` owns memory outside of the arena, this will be a memory leak. +/// ## No `Drop`s /// -/// Note: This is not a soundness issue, as Rust does not support relying on `drop` -/// being called to guarantee soundness. +/// Objects allocated into Oxc memory arenas are never [`Dropped`](Drop). Memory is released in bulk +/// when the allocator is dropped, without dropping the individual objects in the arena. +/// +/// Therefore, it would produce a memory leak if you allocated [`Drop`] types into the arena +/// which own memory allocations outside the arena. +/// +/// Static checks make this impossible to do. [`Vec::new_in`] and all other methods which create +/// a [`Vec`] will refuse to compile if called with a [`Drop`] type. #[derive(PartialEq, Eq)] pub struct Vec<'alloc, T>(pub(crate) ManuallyDrop>); @@ -56,6 +60,10 @@ impl<'alloc, T> Vec<'alloc, T> { /// ``` #[inline(always)] pub fn new_in(allocator: &'alloc Allocator) -> Self { + const { + assert!(!needs_drop::(), "Cannot create a Vec where T is a Drop type"); + } + Self(ManuallyDrop::new(InnerVec::new_in(allocator.bump()))) } @@ -108,6 +116,10 @@ impl<'alloc, T> Vec<'alloc, T> { /// ``` #[inline(always)] pub fn with_capacity_in(capacity: usize, allocator: &'alloc Allocator) -> Self { + const { + assert!(!needs_drop::(), "Cannot create a Vec where T is a Drop type"); + } + Self(ManuallyDrop::new(InnerVec::with_capacity_in(capacity, allocator.bump()))) } @@ -117,6 +129,10 @@ impl<'alloc, T> Vec<'alloc, T> { /// This is behaviorially identical to [`FromIterator::from_iter`]. #[inline] pub fn from_iter_in>(iter: I, allocator: &'alloc Allocator) -> Self { + const { + assert!(!needs_drop::(), "Cannot create a Vec where T is a Drop type"); + } + let iter = iter.into_iter(); let hint = iter.size_hint(); let capacity = hint.1.unwrap_or(hint.0); @@ -143,6 +159,10 @@ impl<'alloc, T> Vec<'alloc, T> { /// ``` #[inline] pub fn from_array_in(array: [T; N], allocator: &'alloc Allocator) -> Self { + const { + assert!(!needs_drop::(), "Cannot create a Vec where T is a Drop type"); + } + let boxed = Box::new_in(array, allocator); let ptr = Box::into_non_null(boxed).as_ptr().cast::(); // SAFETY: `ptr` has correct alignment - it was just allocated as `[T; N]`.