Skip to content

Commit

Permalink
fix(traverse)!: remove unsound APIs (#7514)
Browse files Browse the repository at this point in the history
It's essential to `oxc_traverse`'s safety scheme that the user cannot create a `TraverseAncestry`, because they could then substitute it for the one stored in `TraverseCtx`, and cause a buffer underrun when an ancestor gets popped off stack which should never be empty - but it is because user has sneakily swapped it for another one.

Not being able to create a `TraverseAncestry` also requires that user cannot obtain an owned `TraverseCtx` either, because you can obtain an owned `TraverseAncestry` from an owned `TraverseCtx`.

Therefore, it's unsound for `TraverseCtx::new` to be public.

However, it is useful in minifier to be able to re-use the same `TraverseCtx` over and over, which requires having an owned `TraverseCtx`.

To support this use case, introduce `ReusableTraverseCtx`. It is an opaque wrapper around `TraverseCtx`, which prevents accessing the `TraverseCtx` inside it. It's safe for user to own a `ReusableTraverseCtx`, because there's nothing they can do with it except for using it to traverse via `traverse_mut_with_ctx`, which ensures the safety invariants are upheld.

At some point, we'll hopefully be able to reduce the number of passes in the minifier, and so remove the need for `ReusableTraverseCtx`.But in the meantime, this keeps `Traverse`'s API safe from unsound abuse.

Note: Strictly speaking, there is still room to abuse the API and produce UB by initiating a 2nd traversal of a different AST in an `Traverse` visitor, and then `mem::swap` the 2 x `&mut TraverseCtx`s. But this is 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 real risk in practice. We'd need branded lifetimes to close that hole too.

So this PR doesn't 100% ensure safety in a formal sense, but it at least makes it very hard to trigger UB *by accident*, which was the risk before.
  • Loading branch information
overlookmotel committed Nov 28, 2024
1 parent 8acab7f commit 84e1bf6
Show file tree
Hide file tree
Showing 18 changed files with 181 additions and 106 deletions.
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use oxc_allocator::Vec;
use oxc_ast::ast::*;
use oxc_traverse::{Traverse, TraverseCtx};
use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx};

use crate::CompressorPass;

Expand All @@ -17,9 +17,9 @@ impl<'a> CompressorPass<'a> for CollapseVariableDeclarations {
self.changed
}

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
self.changed = false;
oxc_traverse::walk_program(self, program, ctx);
traverse_mut_with_ctx(self, program, ctx);
}
}

Expand Down
6 changes: 3 additions & 3 deletions crates/oxc_minifier/src/ast_passes/exploit_assigns.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use oxc_ast::ast::*;
use oxc_traverse::{Traverse, TraverseCtx};
use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse};

use crate::CompressorPass;

Expand All @@ -15,9 +15,9 @@ impl<'a> CompressorPass<'a> for ExploitAssigns {
self.changed
}

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
self.changed = false;
oxc_traverse::walk_program(self, program, ctx);
traverse_mut_with_ctx(self, program, ctx);
}
}

Expand Down
12 changes: 6 additions & 6 deletions crates/oxc_minifier/src/ast_passes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ pub use statement_fusion::StatementFusion;

use oxc_allocator::Vec;
use oxc_ast::ast::*;
use oxc_traverse::{Traverse, TraverseCtx};
use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx};

pub trait CompressorPass<'a>: Traverse<'a> {
fn changed(&self) -> bool;

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>);
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>);
}

// See `latePeepholeOptimizations`
Expand Down Expand Up @@ -61,9 +61,9 @@ impl<'a> CompressorPass<'a> for EarlyPass {
self.changed
}

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
self.changed = false;
oxc_traverse::walk_program(self, program, ctx);
traverse_mut_with_ctx(self, program, ctx);
self.changed = self.x0_statement_fusion.changed()
|| self.x1_peephole_remove_dead_code.changed()
|| self.x2_peephole_minimize_conditions.changed()
Expand Down Expand Up @@ -167,9 +167,9 @@ impl<'a> CompressorPass<'a> for LatePass {
self.changed
}

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
self.changed = false;
oxc_traverse::walk_program(self, program, ctx);
traverse_mut_with_ctx(self, program, ctx);
self.changed = self.x0_exploit_assigns.changed()
|| self.x0_exploit_assigns.changed()
|| self.x1_collapse_variable_declarations.changed()
Expand Down
6 changes: 3 additions & 3 deletions crates/oxc_minifier/src/ast_passes/peephole_fold_constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use oxc_syntax::{
number::{NumberBase, ToJsString},
operator::{BinaryOperator, LogicalOperator},
};
use oxc_traverse::{Ancestor, Traverse, TraverseCtx};
use oxc_traverse::{traverse_mut_with_ctx, Ancestor, ReusableTraverseCtx, Traverse, TraverseCtx};

use crate::{node_util::Ctx, CompressorPass};

Expand All @@ -24,9 +24,9 @@ impl<'a> CompressorPass<'a> for PeepholeFoldConstants {
self.changed
}

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
self.changed = false;
oxc_traverse::walk_program(self, program, ctx);
traverse_mut_with_ctx(self, program, ctx);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use oxc_ast::ast::*;
use oxc_ecmascript::ToBoolean;
use oxc_span::SPAN;
use oxc_traverse::{Traverse, TraverseCtx};
use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx};

use crate::CompressorPass;

Expand All @@ -21,9 +21,9 @@ impl<'a> CompressorPass<'a> for PeepholeMinimizeConditions {
self.changed
}

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
self.changed = false;
oxc_traverse::walk_program(self, program, ctx);
traverse_mut_with_ctx(self, program, ctx);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use oxc_allocator::Vec;
use oxc_ast::{ast::*, Visit};
use oxc_ecmascript::constant_evaluation::{ConstantEvaluation, IsLiteralValue};
use oxc_span::SPAN;
use oxc_traverse::{Ancestor, Traverse, TraverseCtx};
use oxc_traverse::{traverse_mut_with_ctx, Ancestor, ReusableTraverseCtx, Traverse, TraverseCtx};

use crate::node_util::Ctx;
use crate::{keep_var::KeepVar, CompressorPass};
Expand All @@ -22,9 +22,9 @@ impl<'a> CompressorPass<'a> for PeepholeRemoveDeadCode {
self.changed
}

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
self.changed = false;
oxc_traverse::walk_program(self, program, ctx);
traverse_mut_with_ctx(self, program, ctx);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use oxc_ecmascript::{
constant_evaluation::ConstantEvaluation, StringCharAt, StringCharCodeAt, StringIndexOf,
StringLastIndexOf, StringSubstring,
};
use oxc_traverse::{Traverse, TraverseCtx};
use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx};

use crate::{node_util::Ctx, CompressorPass};

Expand All @@ -19,9 +19,9 @@ impl<'a> CompressorPass<'a> for PeepholeReplaceKnownMethods {
self.changed
}

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
self.changed = false;
oxc_traverse::walk_program(self, program, ctx);
traverse_mut_with_ctx(self, program, ctx);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use oxc_syntax::{
number::NumberBase,
operator::{BinaryOperator, UnaryOperator},
};
use oxc_traverse::{Ancestor, Traverse, TraverseCtx};
use oxc_traverse::{traverse_mut_with_ctx, Ancestor, ReusableTraverseCtx, Traverse, TraverseCtx};

use crate::{node_util::Ctx, CompressorPass};

Expand All @@ -32,9 +32,9 @@ impl<'a> CompressorPass<'a> for PeepholeSubstituteAlternateSyntax {
self.changed
}

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
self.changed = false;
oxc_traverse::walk_program(self, program, ctx);
traverse_mut_with_ctx(self, program, ctx);
}
}

Expand Down
6 changes: 3 additions & 3 deletions crates/oxc_minifier/src/ast_passes/remove_syntax.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use oxc_allocator::Vec;
use oxc_ast::ast::*;
use oxc_span::GetSpan;
use oxc_traverse::{Traverse, TraverseCtx};
use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx};

use crate::{CompressOptions, CompressorPass};

Expand All @@ -19,8 +19,8 @@ impl<'a> CompressorPass<'a> for RemoveSyntax {
false
}

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
oxc_traverse::walk_program(self, program, ctx);
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
traverse_mut_with_ctx(self, program, ctx);
}
}

Expand Down
6 changes: 3 additions & 3 deletions crates/oxc_minifier/src/ast_passes/statement_fusion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use oxc_allocator::Vec;
use oxc_ast::ast::*;
use oxc_ecmascript::side_effects::MayHaveSideEffects;
use oxc_span::SPAN;
use oxc_traverse::{Traverse, TraverseCtx};
use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx};

use crate::CompressorPass;

Expand All @@ -20,9 +20,9 @@ impl<'a> CompressorPass<'a> for StatementFusion {
self.changed
}

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
self.changed = false;
oxc_traverse::walk_program(self, program, ctx);
traverse_mut_with_ctx(self, program, ctx);
}
}

Expand Down
6 changes: 3 additions & 3 deletions crates/oxc_minifier/src/compressor.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use oxc_allocator::Allocator;
use oxc_ast::ast::*;
use oxc_semantic::{ScopeTree, SemanticBuilder, SymbolTable};
use oxc_traverse::TraverseCtx;
use oxc_traverse::ReusableTraverseCtx;

use crate::{
ast_passes::{
Expand Down Expand Up @@ -33,7 +33,7 @@ impl<'a> Compressor<'a> {
scopes: ScopeTree,
program: &mut Program<'a>,
) {
let mut ctx = TraverseCtx::new(scopes, symbols, self.allocator);
let mut ctx = ReusableTraverseCtx::new(scopes, symbols, self.allocator);
RemoveSyntax::new(self.options).build(program, &mut ctx);

if self.options.dead_code_elimination {
Expand Down Expand Up @@ -62,7 +62,7 @@ impl<'a> Compressor<'a> {
LatePass::new().build(program, &mut ctx);
}

fn dead_code_elimination(program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) {
fn dead_code_elimination(program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) {
PeepholeFoldConstants::new().build(program, ctx);
PeepholeMinimizeConditions::new().build(program, ctx);
PeepholeRemoveDeadCode::new().build(program, ctx);
Expand Down
4 changes: 2 additions & 2 deletions crates/oxc_minifier/src/tester.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use oxc_codegen::{CodeGenerator, CodegenOptions};
use oxc_parser::Parser;
use oxc_semantic::SemanticBuilder;
use oxc_span::SourceType;
use oxc_traverse::TraverseCtx;
use oxc_traverse::ReusableTraverseCtx;

use crate::{ast_passes::CompressorPass, ast_passes::RemoveSyntax, CompressOptions};

Expand Down Expand Up @@ -40,7 +40,7 @@ fn run<'a, P: CompressorPass<'a>>(
if let Some(pass) = pass {
let (symbols, scopes) =
SemanticBuilder::new().build(&program).semantic.into_symbol_table_and_scope_tree();
let mut ctx = TraverseCtx::new(scopes, symbols, allocator);
let mut ctx = ReusableTraverseCtx::new(scopes, symbols, allocator);
RemoveSyntax::new(CompressOptions::all_false()).build(&mut program, &mut ctx);
pass.build(&mut program, &mut ctx);
}
Expand Down
Loading

0 comments on commit 84e1bf6

Please sign in to comment.