oxc icon indicating copy to clipboard operation
oxc copied to clipboard

`AstBuilder::copy` is unsound

Open overlookmotel opened this issue 8 months ago • 8 comments

https://github.com/oxc-project/oxc/blob/7f7b5ea9e83f37ff666d6f33fda7a9e27ea2aa07/crates/oxc_ast/src/ast_builder.rs#L71-L79

This method can be used correctly when a node is being removed from AST anyway, but there are multiple ways in which it can lead to unsound situations:

Aliasing violation:

fn do_bad_stuff(stmt: &Statement, ast: &AstBuilder) {
  if let Statement::BlockStatement(block) = stmt {
    let box1 = ast.copy(block);
    let box2 = ast.copy(block);
    // We now have two `Box<BlockStatement>` pointing to same `BlockStatement`
  }
}

Mutation through immutable ref:

fn do_bad_stuff2(stmt: &Statement, ast: &AstBuilder) {
  if let Statement::BlockStatement(block) = stmt {
    let block = ast.copy(block);
    // We can mutate, even though we only got an immutable ref
    block.body.clear();
  }
}

The double-free which #3481 fixes was likely caused by using copy.

We could make the method unsafe and caller will have to ensure it's only used when safe to do so. However, personally I think we should remove the API entirely because (1) it is difficult to reason about when it's safe to use and is therefore a footgun and (2) the performance hit from using safe alternatives is very minimal.

Unfortunately the alternatives are likely take or mem::replace which are not so ergonomic.

overlookmotel avatar May 31 '24 06:05 overlookmotel