rust-analyzer / rowan

Apache License 2.0
689 stars 57 forks source link

Fixed unsound Clone::clone() implementation of ThinArc #94

Closed ruabmbua closed 3 years ago

ruabmbua commented 3 years ago

In the custom ThinArc type in arc.rs the Clone implementation causes undefined behaviour. ThinArc is passed as an immutable reference, meaning no mutable references are allowed to be constructed from its data. (There is no UnsafeCell in sight).

In arc.rs line 297, a *mut T pointer obtained from the ThinArc contents is passed to slice::from_raw_parts_mut(), which casts the mutable pointer to a mutable reference internally, which is undefined behaviour.

https://github.com/rust-analyzer/rowan/blob/20c0564531763f4bac9e584670ae6e331a15eb21/src/arc.rs#L297

I replaced the call to ptr::slice_from_raw_parts_mut, which does not suffer from this problem. This also makes the wrapping unsafe {} block, and casting back to *mut [T] unnecessary.

Testing: Miri agrees

CAD97 commented 3 years ago

@matklad this is adapted from triomphe, which was written before ptr::slice_from_raw_parts, not from my pointer-utils, right? I'm 90% sure I fixed all cases of me using the wrong one.

Probably all uses of slice::from_raw_parts should be using ptr::slice_from_raw_parts instead such that they don't change pointer provenance at all.

There's at least one more in the file from me scanning it.

CAD97 commented 3 years ago

Specifically, L262. It creates a &[T] from &self so isn't immediate UB, but it is still creating an incorrect reference (it's actually to HeaderSlice<H, [T]>) and is used illegally under the current Stacked Borrows model (as it's then extended to the actual &HeaderSlice).

Even if it weren't required, the raw version better expresses intent there.

lnicola commented 3 years ago

this is adapted from triomphe, which was written before ptr::slice_from_raw_parts, not from my pointer-utils, right?

Yeah, that's from https://github.com/rust-analyzer/rowan/pull/89/files#diff-165c024876cc38d3c8254e7c741e0f9eafb0c38169e65922bfd41c53e97ac20d.

lnicola commented 3 years ago

Can you also bump the version?

matklad commented 3 years ago

bors r+

bors[bot] commented 3 years ago

Build succeeded: