fix(allocator): remove unsound impl Sync for Allocator
#13033
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
CodSpeed Instrumentation Performance ReportMerging #13033 will not alter performanceComparing Summary
|
Member
|
You need to fix breaking changes in rolldown. |
Member
Author
I know! 😢 So far I've only fixed the ones in Oxc... |
overlookmotel
changed the base branch from
08-12-fix_linter_prevent_unsound_use_of_allocator_across_threads
to
graphite-base/13033
overlookmotel
force-pushed
the
08-12-fix_allocator_remove_unsound_impl_sync_for_allocator_
branch
from
25af23b to
6b33f35
Compare
overlookmotel
force-pushed
the
graphite-base/13033
branch
from
5589e02 to
1585688
Compare
overlookmotel
changed the base branch from
graphite-base/13033
to
08-12-fix_linter_prevent_unsound_use_of_allocator_across_threads
This was referenced Aug 13, 2025
graphite-app
bot
changed the base branch from
08-12-fix_linter_prevent_unsound_use_of_allocator_across_threads
to
graphite-base/13033
graphite-app
bot
force-pushed
the
graphite-base/13033
branch
from
1585688 to
b0558a4
Compare
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_unsound_impl_sync_for_allocator_
branch
from
6b33f35 to
68b7ce3
Compare
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_unsound_impl_sync_for_allocator_
branch
from
68b7ce3 to
b76e7e6
Compare
overlookmotel
force-pushed
the
08-12-fix_allocator_remove_unsound_impl_sync_for_allocator_
branch
from
b76e7e6 to
2535244
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR removes unsafe Send and Sync implementations for the Allocator type to fix thread safety issues. The Allocator is not thread-safe and allowing concurrent access could cause data corruption or out-of-bounds writes.
- Removes unsafe
impl Send for Allocatorandimpl Sync for Allocator - Improves memory safety by preventing unsound concurrent access to the allocator
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
camc314
approved these changes
Aug 19, 2025
Contributor
Merge activity
|
`Allocator` should not be `Sync`. `Allocator` is not thread-safe and allocating into it in multiple threads can cause data corruption or, in worst case, writing out of bounds. Remove the unsound `impl Sync for Allocator`. This PR also removes `impl Send for Allocator`, but that doesn't change anything. `bumpalo::Bump` is already `Send`, so `Allocator` is automatically `Send` too. It doesn't need an explicit `Send` impl.
graphite-app
bot
force-pushed
the
08-12-fix_allocator_remove_unsound_impl_sync_for_allocator_
branch
from
2535244 to
d2e8cb6
Compare
graphite-app bot
pushed a commit
that referenced
this pull request
Aug 19, 2025
…3042) Previous PRs removed unsound `Sync` impl for `Allocator` (#13033) and `Send` impl for `Vec` (#13041). Previously `ScopingCell` was `Send` and `Sync` (and therefore `Scoping` was too). The recent PRs mean that `ScopingCell` lost those traits too. This PR implements both traits again for `ScopingCell` by restricting its API slightly, so now it is `Send` and `Sync` again, but this time in a manner which maintains soundness. The comments in the code outline the logic of why I believe it to be sound. Rolldown relies on `ScopingCell` being `Send` and `Sync`. After this PR, this stack does not require any changes in Rolldown. I've checked that `cargo ck` in Rolldown passes when using the version of Oxc from this branch.
graphite-app bot
pushed a commit
that referenced
this pull request
Aug 19, 2025
…ocationStats` (#13043) `AllocationStats` (introduced in #12555 and #12937) previously had to contain `AtomicUsize`s because `Allocator` was `Sync`. #13033 removed the `Sync` impl for `Allocator`, so now there's no need for synchronization in `AllocationStats`, and these fields can be `Cell<usize>` instead.
graphite-app
bot
deleted the
08-12-fix_allocator_remove_unsound_impl_sync_for_allocator_
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allocatorshould not beSync.Allocatoris not thread-safe and allocating into it in multiple threads can cause data corruption or, in worst case, writing out of bounds.Remove the unsound
impl Sync for Allocator.This PR also removes
impl Send for Allocator, but that doesn't change anything.bumpalo::Bumpis alreadySend, soAllocatoris automaticallySendtoo. It doesn't need an explicitSendimpl.