refactor(allocator): harden safety of FixedSizeAllocator::new
#13124
Conversation
github-actions
bot
added
the
C-cleanup
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
overlookmotel
force-pushed
the
08-15-refactor_allocator_harden_safety_of_fixedsizeallocator_new_
branch
from
27cd512 to
aba0758
Compare
CodSpeed Instrumentation Performance ReportMerging #13124 will not alter performanceComparing Summary
Footnotes |
There was a problem hiding this comment.
Pull Request Overview
This PR hardens the safety of FixedSizeAllocator::new by adding defensive programming measures to prevent undefined behavior and memory leaks in edge cases.
- Adds early panic on big-endian systems before allocation to prevent memory leaks
- Wraps
AllocatorinManuallyDropimmediately after creation to prevent UB if later code panics - Moves the
ManuallyDrop::newcall earlier in the function for better safety
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
I don't think anyone else has foggiest idea about this code. Even I only barely do, and I wrote it! So merging without review. |
Merge activity
|
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.
graphite-app
bot
force-pushed
the
08-15-refactor_allocator_harden_safety_of_fixedsizeallocator_new_
branch
from
aba0758 to
18ad3c0
Compare
graphite-app
bot
deleted the
08-15-refactor_allocator_harden_safety_of_fixedsizeallocator_new_
branch
Tighten up safety of
FixedSizeAllocator::new.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.Wrap
AllocatorinManuallyDropas soon as it's created. If code later in the function panicked,Allocatorwould be dropped, which would be UB.ManuallyDropprevents 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
FixedSizedAllocatoris 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.