Skip to content

Commit 18ad3c0

Browse files
committed
refactor(allocator): harden safety of FixedSizeAllocator::new (#13124)
Tighten up safety of `FixedSizeAllocator::new`. 1. Panic early on big-endian system. This should not be possible, but adding an assertion before the allocation happens makes absolutely sure the same panic can't happen in `Allocator::from_raw_parts`. If it did, the allocation would not be freed, which would be a huge memory leak. 2. Wrap `Allocator` in `ManuallyDrop` as soon as it's created. If code later in the function panicked, `Allocator` would be dropped, which would be UB. `ManuallyDrop` prevents that happening, so a panic would instead cause a memory leak - still bad, but better than UB. Neither of these scenarios should be possible, but all the code around `FixedSizedAllocator` is pretty labyrinthine (bad design on my part). So it's better to code defensively in case of a bug elsewhere. Both changes have 0 performance impact.
1 parent bef2ace commit 18ad3c0

File tree

1 file changed

+15
-1
lines changed

1 file changed

+15
-1
lines changed

crates/oxc_allocator/src/pool_fixed_size.rs

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,13 +207,26 @@ impl FixedSizeAllocator {
207207
/// Create a new [`FixedSizeAllocator`].
208208
#[expect(clippy::items_after_statements)]
209209
pub fn new(id: u32) -> Self {
210+
// Only support little-endian systems. `Allocator::from_raw_parts` includes this same assertion.
211+
// This module is only compiled on 64-bit little-endian systems, so it should be impossible for
212+
// this panic to occur. But we want to make absolutely sure that if there's a mistake elsewhere,
213+
// `Allocator::from_raw_parts` cannot panic, as that'd result in a large memory leak.
214+
// Compiler will optimize this out.
215+
#[expect(clippy::manual_assert)]
216+
if cfg!(target_endian = "big") {
217+
panic!("`FixedSizeAllocator` is not supported on big-endian systems.");
218+
}
219+
210220
// Allocate block of memory.
211221
// SAFETY: `ALLOC_LAYOUT` does not have zero size.
212222
let alloc_ptr = unsafe { System.alloc(ALLOC_LAYOUT) };
213223
let Some(alloc_ptr) = NonNull::new(alloc_ptr) else {
214224
alloc::handle_alloc_error(ALLOC_LAYOUT);
215225
};
216226

227+
// All code in the rest of this function is infallible, so the allocation will always end up
228+
// owned by a `FixedSizeAllocator`, which takes care of freeing the memory correctly on drop
229+
217230
// Get pointer to use for allocator chunk, aligned to 4 GiB.
218231
// `alloc_ptr` is aligned on 2 GiB, so `alloc_ptr % FOUR_GIB` is either 0 or `TWO_GIB`.
219232
//
@@ -243,6 +256,7 @@ impl FixedSizeAllocator {
243256
// the allocation we just made.
244257
// `chunk_ptr` has high alignment (4 GiB). `CHUNK_SIZE` is large and a multiple of 16.
245258
let allocator = unsafe { Allocator::from_raw_parts(chunk_ptr, CHUNK_SIZE) };
259+
let allocator = ManuallyDrop::new(allocator);
246260

247261
// Write `FixedSizeAllocatorMetadata` to after space reserved for `RawTransferMetadata`,
248262
// which is after the end of the allocator chunk
@@ -257,7 +271,7 @@ impl FixedSizeAllocator {
257271
metadata_ptr.write(metadata);
258272
}
259273

260-
Self { allocator: ManuallyDrop::new(allocator) }
274+
Self { allocator }
261275
}
262276

263277
/// Reset this [`FixedSizeAllocator`].

0 commit comments

Comments
 (0)