fix(linter): prevent unsound use of Allocator across threads (#13032)

overlookmotel · overlookmotel · commit b0558a46fea1 · 2025-08-13T11:47:57.000Z
`Allocator` is currently `Sync`, but it shouldn't be - `Allocator` is not thread-safe and allocating into it in multiple threads can cause data corruption or, in worst case, writing out of bounds. I intend to remove the `impl Sync for Allocator` in a PR to follow.

The code altered in this PR was relying on `Allocator` being `Sync`. It was *not* actually dangerous as it stood, because access to the `Allocator` was synchronized by always holding a lock on the `messages` `Mutex` while using it. However, I suspect this was more by accident by design, and it was rather fragile.

This PR introduces a `MessageCloner` struct which provides a safe API which ensures access to the `Allocator` is correctly synchronized.

It's unfortunate to introduce a 2nd `Mutex`, and the `Mutex` inside `MessageCloner` is accessed more than necessary - on each turn of the loop in the `map` loops, instead of just once at the top of the loop. However, I couldn't find a way to achieve that without resorting to fragile unsafe code.

In any case, from what I can understand, the ultimate callers of both `run_source` and `run_test_source` convert the cloned `Message`s into other owning types (`DiagnosticReport` or `Report`) anyway. Therefore the double-conversion may be wasteful anyway, and we might be better off trying to convert to these owned types earlier, and avoid using an `Allocator` at all in these methods.

I also am wondering in what circumstances error messages would contain arena-allocated strings, and whether `Message` could instead contain `Cow&lt;'static, str&gt;`s, in which case the `Allocator` becomes unnecessary, and cloning would be cheaper.

Delving into the depths of the linter to understand all this is a bit beyond me, so I hope we can accept this imperfect solution for now, and refactor to remove/optimize it later on.
diff --git a/crates/oxc_language_server/src/linter/isolated_lint_handler.rs b/crates/oxc_language_server/src/linter/isolated_lint_handler.rs
@@ -92,9 +92,9 @@ impl IsolatedLintHandler {
             return None;
         }
 
-        let allocator = Allocator::default();
+        let mut allocator = Allocator::default();
 
-        Some(self.lint_path(&allocator, &path, content).map_or(vec![], |errors| {
+        Some(self.lint_path(&mut allocator, &path, content).map_or(vec![], |errors| {
             let mut diagnostics: Vec<DiagnosticReport> = errors
                 .iter()
                 .map(|e| message_with_position_to_lsp_diagnostic_report(e, uri))
@@ -143,7 +143,7 @@ impl IsolatedLintHandler {
 
     fn lint_path<'a>(
         &mut self,
-        allocator: &'a Allocator,
+        allocator: &'a mut Allocator,
         path: &Path,
         source_text: Option<String>,
     ) -> Option<Vec<MessageWithPosition<'a>>> {
diff --git a/crates/oxc_linter/src/service/mod.rs b/crates/oxc_linter/src/service/mod.rs
@@ -96,7 +96,7 @@ impl LintService {
     #[cfg(feature = "language_server")]
     pub fn run_source<'a>(
         &mut self,
-        allocator: &'a oxc_allocator::Allocator,
+        allocator: &'a mut oxc_allocator::Allocator,
     ) -> Vec<crate::MessageWithPosition<'a>> {
         self.runtime.run_source(allocator)
     }
@@ -105,7 +105,7 @@ impl LintService {
     #[cfg(test)]
     pub(crate) fn run_test_source<'a>(
         &mut self,
-        allocator: &'a oxc_allocator::Allocator,
+        allocator: &'a mut oxc_allocator::Allocator,
         check_syntax_errors: bool,
         tx_error: &DiagnosticSender,
     ) -> Vec<crate::Message<'a>> {
diff --git a/crates/oxc_linter/src/service/runtime.rs b/crates/oxc_linter/src/service/runtime.rs
@@ -173,6 +173,72 @@ impl RuntimeFileSystem for OsFileSystem {
     }
 }
 
+/// [`MessageCloner`] is a wrapper around an `&Allocator` which allows it to be safely shared across threads,
+/// in order to clone [`Message`]s into it.
+///
+/// `Allocator` is not thread safe (it is not `Sync`), so cannot be shared across threads.
+/// It would be undefined behavior to allocate into an `Allocator` from multiple threads simultaneously.
+///
+/// `MessageCloner` ensures only one thread at a time can utilize the `Allocator`, by taking an
+/// exclusive `&mut Allocator` to start with, and synchronising access to the `Allocator` with a `Mutex`.
+///
+/// This type is wrapped in a module so that other code cannot access the inner `UnsafeAllocatorRef`
+/// directly, and must go via the [`MessageCloner::clone_message`] method.
+mod message_cloner {
+    use std::sync::Mutex;
+
+    use oxc_allocator::{Allocator, CloneIn};
+
+    use crate::Message;
+
+    /// Unsafe wrapper around an `&Allocator` which makes it `Send`.
+    struct UnsafeAllocatorRef<'a>(&'a Allocator);
+
+    // SAFETY: It is sound to implement `Send` for `UnsafeAllocatorRef` because:
+    // * The only way to construct an `UnsafeAllocatorRef` is via `MessageCloner::new`, which takes
+    //   an exclusive `&mut Allocator`, ensuring no other references to the same `Allocator` exist.
+    // * The lifetime `'a` ensures that the reference to the `Allocator` cannot outlive the original
+    //   mutable borrow, preventing aliasing or concurrent mutation.
+    // * All access to the `Allocator` via `UnsafeAllocatorRef` is synchronized by a `Mutex` inside
+    //   `MessageCloner`, so only one thread can access the allocator at a time.
+    // * The module encapsulation prevents direct access to `UnsafeAllocatorRef`, so it cannot be
+    //   misused outside of the intended, synchronized context.
+    //
+    // Therefore, although `Allocator` is not `Sync`, it is safe to send `UnsafeAllocatorRef` between
+    // threads as long as it is only accessed via the `Mutex` in `MessageCloner`.
+    unsafe impl Send for UnsafeAllocatorRef<'_> {}
+
+    /// Wrapper around an [`Allocator`] which allows safely using it on multiple threads to
+    /// clone [`Message`]s into.
+    pub struct MessageCloner<'a>(Mutex<UnsafeAllocatorRef<'a>>);
+
+    impl<'a> MessageCloner<'a> {
+        /// Wrap an [`Allocator`] in a [`MessageCloner`].
+        ///
+        /// This method takes a `&mut Allocator`, to ensure that no other references to the `Allocator`
+        /// can exist, which guarantees no other threads can allocate with the `Allocator` while this
+        /// `MessageCloner` exists.
+        #[inline]
+        #[expect(clippy::needless_pass_by_ref_mut)]
+        pub fn new(allocator: &'a mut Allocator) -> Self {
+            Self(Mutex::new(UnsafeAllocatorRef(allocator)))
+        }
+
+        /// Clone a [`Message`] into the [`Allocator`] held by this [`MessageCloner`].
+        ///
+        /// # Panics
+        /// Panics if the underlying `Mutex` is poisoned.
+        pub fn clone_message(&self, message: &Message) -> Message<'a> {
+            // Obtain an exclusive lock on the `Mutex` during `clone_in` operation,
+            // to ensure no other thread can be simultaneously using the `Allocator`
+            let guard = self.0.lock().unwrap();
+            let allocator = guard.0;
+            message.clone_in(allocator)
+        }
+    }
+}
+use message_cloner::MessageCloner;
+
 impl Runtime {
     pub(super) fn new(
         linter: Linter,
@@ -588,12 +654,12 @@ impl Runtime {
     #[cfg(feature = "language_server")]
     pub(super) fn run_source<'a>(
         &mut self,
-        allocator: &'a oxc_allocator::Allocator,
+        allocator: &'a mut oxc_allocator::Allocator,
     ) -> Vec<MessageWithPosition<'a>> {
-        use oxc_allocator::CloneIn;
-        use oxc_data_structures::rope::Rope;
         use std::sync::Mutex;
 
+        use oxc_data_structures::rope::Rope;
+
         use crate::{
             FixWithPosition,
             fixer::{Fix, PossibleFixesWithPosition},
@@ -614,6 +680,9 @@ impl Runtime {
             }
         }
 
+        // Wrap allocator in `MessageCloner` so can clone `Message`s into it
+        let message_cloner = MessageCloner::new(allocator);
+
         let messages = Mutex::new(Vec::<MessageWithPosition<'a>>::new());
         let (sender, _receiver) = mpsc::channel();
         rayon::scope(|scope| {
@@ -655,7 +724,7 @@ impl Runtime {
 
                             messages.lock().unwrap().extend(section_messages.iter().map(
                                 |message| {
-                                    let message = message.clone_in(allocator);
+                                    let message = message_cloner.clone_message(message);
 
                                     let labels = &message.error.labels.clone().map(|labels| {
                                         labels
@@ -742,13 +811,15 @@ impl Runtime {
     #[cfg(test)]
     pub(super) fn run_test_source<'a>(
         &mut self,
-        allocator: &'a Allocator,
+        allocator: &'a mut Allocator,
         check_syntax_errors: bool,
         tx_error: &DiagnosticSender,
     ) -> Vec<Message<'a>> {
-        use oxc_allocator::CloneIn;
         use std::sync::Mutex;
 
+        // Wrap allocator in `MessageCloner` so can clone `Message`s into it
+        let message_cloner = MessageCloner::new(allocator);
+
         let messages = Mutex::new(Vec::<Message<'a>>::new());
         rayon::scope(|scope| {
             self.resolve_modules(scope, check_syntax_errors, tx_error, |me, mut module| {
@@ -773,13 +844,13 @@ impl Runtime {
                                         .map(|err| Message::new(err, PossibleFixes::None))
                                         .collect(),
                                 }
-                                .into_iter()
-                                .map(|mut message| {
+                                .iter_mut()
+                                .map(|message| {
                                     if section.source.start != 0 {
                                         message.move_offset(section.source.start)
                                         .move_fix_offset(section.source.start);
                                     }
-                                    message.clone_in(allocator)
+                                    message_cloner.clone_message(message)
                                 }),
                             );
                         }
diff --git a/crates/oxc_linter/src/tester.rs b/crates/oxc_linter/src/tester.rs
@@ -501,7 +501,7 @@ impl Tester {
         fix_kind: ExpectFixKind,
         fix_index: u8,
     ) -> TestResult {
-        let allocator = Allocator::default();
+        let mut allocator = Allocator::default();
         let rule = self.find_rule().read_json(rule_config.unwrap_or_default());
         let mut external_plugin_store = ExternalPluginStore::default();
         let linter = Linter::new(
@@ -554,7 +554,7 @@ impl Tester {
             .with_paths(paths);
 
         let (sender, _receiver) = mpsc::channel();
-        let result = lint_service.run_test_source(&allocator, false, &sender);
+        let result = lint_service.run_test_source(&mut allocator, false, &sender);
 
         if result.is_empty() {
             return TestResult::Passed;
