AstBuilder::copy is unsound

[overlookmotel]

oxc/crates/oxc_ast/src/ast_builder.rs

Lines 71 to 79 in 7f7b5ea

	pub fn copy<T>(&self, src: &T) -> T {
	// SAFETY:
	// This should be safe as long as `src` is an reference from the allocator.
	// But honestly, I'm not really sure if this is safe.
	#[allow(unsafe_code)]
	unsafe {
	std::mem::transmute_copy(src)
	}
	}

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.

[Boshen]

@Dunqing Please don't use this API from now on 😁 .

Use move_xxx APIs instead.

[overlookmotel]

I don't think it's necessarily high priority to fix this. I just wanted to request that we don't introduce more uses of it now, as it will make it more work to remove it later.

[rzvxa]

@overlookmotel Can we use the CloneIn trait over this?

[overlookmotel]

I think CloneIn is something different. Usually ast.copy is used when we want to extract a node from AST, and insert it somewhere else. Cloning would be very expensive if the node is the tip of a deep tree, and isn't necessary.

[rzvxa]

I think CloneIn is something different. Usually ast.copy is used when we want to extract a node from AST, and insert it somewhere else. Cloning would be very expensive if the node is the tip of a deep tree, and isn't necessary.

If we just want to take the sub-tree out and put it somewhere else, We should be able to use the move method instead. For all other cases if we don't clone the sub-tree and actually copy it in 2 places how can we prevent aliasing?

[overlookmotel]

Yes, that's true, if you want to duplicate the sub-tree and put it in AST in 2 places, it must be cloned or it's UB. But all the places I saw ast.copy being used, it was to take it out of one place, and put it back in another.

[rzvxa]

So what is stopping us from replacing all of our usages with the move method? Wouldn't it be better if we deprecate this unsound method?

[overlookmotel]

So what is stopping us from replacing all of our usages with the move method?

Nothing! We just haven't done it yet.

Unfortunately, using move methods is quite unergonomic - but such is the ways of Rust.

I would be really keen to see the back of this unsound method. I remain quite anxious about the soundness of Traverse, and would like to run Miri over the transformer to test it. But right now there's no point doing that because Miri is likely to go ape over all the ast.copys, and that'll obscure any other problem.