search

rust-lang / rust-analyzer

A Rust compiler front-end for IDEs
https://rust-analyzer.github.io/
Apache License 2.0
14.31k stars 1.62k forks source link

Safety: intern::symbol::TaggedArcPtr::try_as_arc_owned is unsafe #18499

Open sam-mccall opened 1 week ago

sam-mccall commented 1 week ago

An audit of unsafe in ra_ap_intern found this possible issue:

https://github.com/rust-lang/rust-analyzer/blob/dd9cd22514cb1001a0a2374b36a85eb75245f27b/crates/intern/src/symbol.rs#L81-L92

This must be unsafe, because the safety invariants of ManuallyDrop permit patterns that would be unsound, like calling try_as_arc_owned twice on the same TaggedArcPtr and invoking ManuallyDrop::drop or ::take on both of those two separate ManuallyDrop values, easily producing a use-after-free.

This is a non-blocking issue because it is an internal-only interface which is used correctly.

This doesn't prevent our use of rust-analyzer, but I'm sharing it here in case you agree and want to clarify the safety in the code.

ChayimFriedman2 commented 1 week ago

As said, this method is pub(crate), so it doesn't have to be unsafe. Still this would be an improvement, and I think a PR will be fine.