Skip to content

Commit

Permalink
docs(traverse): document soundness hole (#7515)
Browse files Browse the repository at this point in the history
Document the remaining (small) soundness hole in `Traverse`.
  • Loading branch information
overlookmotel committed Nov 28, 2024
1 parent 5143bb8 commit d351b3e
Showing 1 changed file with 20 additions and 0 deletions.
20 changes: 20 additions & 0 deletions crates/oxc_traverse/src/context/ancestry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,26 @@ const INITIAL_STACK_CAPACITY: usize = 64; // 64 entries = 1 KiB
/// - `TraverseAncestry::new` and `TraverseCtx::new` are private.
/// b. cannot obtain an owned `TraverseAncestry` from a `&TraverseAncestry`
/// - `TraverseAncestry` is not `Clone`.
///
/// ## Soundness hole
///
/// Strictly speaking, there is still room to abuse the API and cause UB as follows:
///
/// * Initiate a 2nd traversal of a different AST inside a `Traverse` visitor method.
/// * `mem::swap` the 2 x `&mut TraverseCtx`s from the 2 different traversals.
///
/// The 2 ASTs would have to be different, but borrowed for same lifetime, so I (@overlookmotel) don't
/// think it's possible by this method to produce aliasing violations, or to over-extend AST node
/// lifetimes to cause a use-after-free.
/// But it *could* produce buffer underrun in `pop_stack`, when it tries to pop from a stack which
/// is already empty.
///
/// In practice, this would be a completely bizarre thing to do, and would basically require you to
/// write malicious code specifically designed to cause UB. So it's not a particularly real risk.
///
/// To close this hole and make the API 100% sound, we'd need branded lifetimes so that all
/// `TraverseCtx`s have unique lifetimes, and so cannot be swapped for any other without
/// the borrow-checker complaining.
pub struct TraverseAncestry<'a> {
stack: NonEmptyStack<Ancestor<'a, 'static>>,
}
Expand Down

0 comments on commit d351b3e

Please sign in to comment.