mirror of
https://github.com/aphrody-labs/oxc.git
synced 2026-10-15 06:39:17 +00:00
fix(linter/plugins): alter method for obtaining mutable Program when sending AST to JS plugins (#26077)
### What's needed for JS plugins When running JS plugins, we need to obtain a mutable copy of the AST (`&mut Program`) in order to alter it (update spans to UTF-16 offsets). But `Semantic` only contains an immutable `&Program` reference, and also its `AstNodes` contains immutable references to _all_ nodes in the AST. So we need to do 2 things: 1. Convert the `&Program` from `Semantic` to `&mut Program`. 2. Get rid of all the active references to AST nodes stored in `AstNodes` so that the `&mut Program` doesn't illegally alias with those references. ### Previous solution and its problems Previously we achieved this with a dodgy hack where we discarded all of `Semantic` (to get rid of the AST node references) and then "laundered" the pointer to the `Program`, upgrading it to write permission, by taking the provenance of the `Allocator`'s cursor pointer (which does have write permission). However, this relied on the `Program` being in the `Allocator`'s current chunk, so that the `Allocator`'s current cursor and the `Program` are within the same allocation (a requirement for the pointer returned from `NonNull::with_addr` being legal to dereference). This assumption was always a bit shaky, but when we enabled React Compiler rules by default in last release, it became clear it was completely untenable. React Compiler rules allocate a lot, and the debug assertions which checked that the `Program` was in current chunk started failing - so many allocations had occured that the `Allocator` had grown a fresh chunk to accomodate them. ### How this PR fixes the problem This PR alters the method by which we obtain the `&mut Program`. It still discards all the references to AST nodes held in `Semantic`, but now performs a bitwise copy of the original `Program` and allocates that copy back into the arena, yielding a `&mut Program`. This avoids the need for pointer "laudering" and removes the requirement for `Program` to be in the current `Allocator` chunk. As noted in the comments, this is still not sound. We have no way to prove that no other references to AST nodes exist, stashed somewhere, which would be an aliasing violation (UB). However, a thorough review of the code shows that we currently do not do this, so there is no actual UB at present. So, in short, this change does not achieve soundness, but it does at least swap definite proven UB for a _potential risk_ of UB which isn't currently triggered. Not great, but better. ### Side effects Previously we nuked the whole of `Semantic`, replacing it all with a new empty copy. Instead this PR operates more "surgically", just removing the AST node and `Comment` references that we _need_ to get rid of to avoid aliasing violations, and leaving the rest of `Semantic` alone. This is much cheaper.
This commit is contained in:
1 parent
36ec0ef2ba
commit
8531b9b2a4
2 files changed
+46
-33
No files matched your search
@@ -7,7 +7,7 @@
|
||||
#![expect(clippy::missing_errors_doc)]
|
||||
|
||||
use std::{
|
||||
iter, mem,
|
||||
iter,
|
||||
path::Path,
|
||||
ptr::{self, NonNull},
|
||||
rc::Rc,
|
||||
@@ -513,39 +513,43 @@ impl Linter {
|
||||
return;
|
||||
}
|
||||
|
||||
// Extract `Semantic` from `ContextHost`, and get a mutable reference to `Program`.
|
||||
//
|
||||
// It's not possible to obtain a `&mut Program` while `Semantic` exists, because `Semantic`
|
||||
// contains `AstNodes`, which contains `AstKind`s for every AST nodes, each of which contains
|
||||
// an immutable `&` ref to an AST node.
|
||||
// Obtaining a `&mut Program` while `Semantic` exists would be illegal aliasing.
|
||||
//
|
||||
// So instead we get a pointer to `Program`.
|
||||
// The pointer is obtained initially from `&Program` in `Semantic`, but that pointer
|
||||
// has no provenance for mutation, so can't be converted to `&mut Program`.
|
||||
// So create a new pointer to `Program` which inherits `cursor_ptr`'s provenance, which does allow mutation.
|
||||
//
|
||||
// We then drop `Semantic`, after which no references to any AST nodes remain.
|
||||
// We can then safely convert the pointer to `&mut Program`.
|
||||
//
|
||||
// `Program` was created in `allocator`, and `Program` is the last thing to be allocated, so is in current chunk.
|
||||
// So `cursor_ptr` and `Program` are within the same allocation.
|
||||
// All callers of `Linter::run` obtain `allocator` and `Semantic` from `ModuleContent`,
|
||||
// which ensure they are in same allocation.
|
||||
// However, we have no static guarantee of this, so strictly speaking it's unsound.
|
||||
// TODO: It would be better to avoid the need for a `&mut Program` here, and so avoid this
|
||||
// sketchy behavior.
|
||||
let ctx_host = Rc::get_mut(ctx_host).unwrap();
|
||||
let semantic = mem::take(ctx_host.semantic_mut());
|
||||
let program_addr = NonNull::from(semantic.nodes().program()).addr();
|
||||
// Check `Program` is in `Allocator`'s current chunk
|
||||
debug_assert!(program_addr >= allocator.cursor_ptr().addr());
|
||||
debug_assert!(program_addr < allocator.data_end_ptr().addr());
|
||||
let mut program_ptr = allocator.cursor_ptr().cast::<Program<'a>>().with_addr(program_addr);
|
||||
drop(semantic);
|
||||
// SAFETY: Now that we've dropped `Semantic`, no references to any AST nodes remain,
|
||||
// so can get a mutable reference to `Program` without aliasing violations
|
||||
let program = unsafe { program_ptr.as_mut() };
|
||||
|
||||
// We need a `&mut Program` here, but `Semantic` only contains a `&Program`, along with `&` references
|
||||
// to all other nodes in the AST.
|
||||
//
|
||||
// Use a very dodgy hack to achieve this.
|
||||
//
|
||||
// 1. Get an immutable reference to `&Program` from the `AstNodes` contained in `Semantic`.
|
||||
// 2. Call `Semantic::clear_ast_references` to discard all references it holds to AST nodes and comments.
|
||||
// 3. Make a bitwise copy of the `Program`.
|
||||
// 4. Allocate it back into arena, yielding a `&mut Program`.
|
||||
//
|
||||
// Within this function, this is sound.
|
||||
// The dangerous part is copying the `ArenaVec`s (`program.body` etc), but `ArenaVec` only contains a pointer
|
||||
// which has no ownership semantics, and they cannot contain `Drop` types. So as long as we don't hold
|
||||
// references to any of the *contents* of the original `ArenaVec`, and don't use it after its been copied,
|
||||
// we can't violate aliasing rules.
|
||||
//
|
||||
// Here we create the `&Program` in a block from which it doesn't escape, ensuring the reference doesn't live
|
||||
// while the copy lives, and we empty all AST node/comment references from `Semantic`, ensuring there's no way
|
||||
// to access references to AST nodes after this point, which could alias the `&mut Program` we've created.
|
||||
//
|
||||
// What we CAN'T protect against is if some other references to AST nodes have been already stashed somewhere,
|
||||
// and that would be UB. At present, we don't do that, but there is nothing statically preventing that,
|
||||
// so it'd be easy for someone add code which inadvertently causes UB, without any idea they were doing that.
|
||||
//
|
||||
// TODO: We need to fix this. We should avoid the need for a mutable AST here by converting AST node spans
|
||||
// to UTF-16 during deserialization on JS side - but that is not easy to achieve without a heavy perf penalty.
|
||||
// This current hack *is* unsound, but is unlikely to bite in practice, so we can live with it for now.
|
||||
let program = {
|
||||
let semantic = ctx_host.semantic_mut();
|
||||
let program = semantic.nodes().program();
|
||||
semantic.clear_ast_references();
|
||||
// SAFETY: Only somewhat safe! See above.
|
||||
let program = unsafe { NonNull::from(program).read() };
|
||||
allocator.alloc(program)
|
||||
};
|
||||
|
||||
// If `js_allocator_pool` is provided, use clone-into-fixed-allocator approach
|
||||
if let Some(js_allocator_pool) = js_allocator_pool {
|
||||
|
||||
@@ -118,6 +118,15 @@ impl<'a> Semantic<'a> {
|
||||
(self.scoping, self.nodes)
|
||||
}
|
||||
|
||||
/// Discard all references to AST nodes and comments held by this `Semantic`.
|
||||
///
|
||||
/// This should not ordinarily be used. It leaves [`Semantic`] in an inconsistent state.
|
||||
/// This method is only present to support some unsafe code in `oxc_linter`.
|
||||
pub fn clear_ast_references(&mut self) {
|
||||
self.nodes = AstNodes::default();
|
||||
self.comments = &[];
|
||||
}
|
||||
|
||||
/// Source code of the JavaScript/TypeScript program being analyzed.
|
||||
pub fn source_text(&self) -> &'a str {
|
||||
self.source_text
|
||||
|
||||
Reference in new issue
Block a user