search

rust-lang / hashbrown

Rust port of Google's SwissTable hash map
https://rust-lang.github.io/hashbrown
Apache License 2.0
2.38k stars 275 forks source link

Unsound usages of unsafe implementation from `usize` to `T` #526

Closed llooFlashooll closed 3 months ago

llooFlashooll commented 3 months ago

Hi, I am scanning the hashbrown in the latest version with my own static analyzer tool.

Unsafe conversion found at: src/raw/mod.rs:65:14: 65:40

fn invalid_mut<T>(addr: usize) -> *mut T {
    unsafe { core::mem::transmute(addr) }
}

This unsound implementation would create a misalignment issues. The usize and T are different types.

The problematic value is further manipulated at: src/raw/mod.rs:380:13: 380:35, src/raw/mod.rs:564:13: 564:61

This would potentially cause undefined behaviors in Rust. If we further manipulate the problematic converted types, it would potentially lead to different consequences. I am reporting this issue for your attention.

Amanieu commented 3 months ago

This function is not unsound: it is converting an integer to a raw pointer, which is not a problem. Raw pointers are allowed to hold any address, even misaligned ones.

llooFlashooll commented 3 months ago

Ok, thanks! My tool would just output some potentially unsafe issues. It mainly depends on the case.

llooFlashooll commented 2 weeks ago

Hi! Maybe I'll apply for the RustSec advisory ID for instructions. People may further know that through this conversion, the worst case would be creating a misaligned and meaningless value. This will help me validate the effectiveness of my tool. Thanks again!

<-This is a wrong attempt. Sorry for any inconvenience I caused. 😢

taiki-e commented 2 weeks ago

The usize and T are different types.

The conversion here is usize to *mut T (pointer to T), not usize to T. It is always sound as long as the resulting pointer is not dereferenced.

llooFlashooll commented 2 weeks ago

I understand. I do not intend to bring false positive. However, what if the user uses this function and then dereference the value?

taiki-e commented 2 weeks ago

Dereferencing pointers require unsafe code, and the user must guarantee the safety of the dereference at that point.

And since invalid_mut is a private function, there is no possibility for users to use it.

See also std::ptr::without_provenance_mut doing the same as invalid_mut.

llooFlashooll commented 2 weeks ago

Thank you. I think it is still an unsafe operation to convert the types. But it depends on the purpose. I will remove the pr issue I created. Sorry for any inconvenience I caused.

taiki-e commented 2 weeks ago

I think it is still an unsafe operation to convert the types.

"unsafe" and "unsound" are not equivalent. A code that misuses an unsafe operation is unsound, but a code that uses it correctly is sound.

That transmute is an unsafe operation has already been explained by the fact that it is an unsafe function, and invalid_mut is sound because it uses that unsafe operation in the correct way. (And since its soundness is independent of its input, it can be declared as a safe function.)