-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Occurrences of unsafe can be replaced with safe versions #107
Comments
Oh both of these are fine. For the first, yeahhhh I'll swap that to to/from bits and drop the unsafe. For the second, specifically rust guarantees |
I am not sure that NonZeroU16 is guaranteed to have the same layout as u16. I couldn't find this statement in the standard library's documentation. |
https://doc.rust-lang.org/beta/std/num/struct.NonZeroU16.html It is guaranteed that NonZeroU16 has the same layout as u16. |
The second link explicitly states
|
There was a proposal for f32s as well which that would apply to, but otherwise it can't settle on any other value for the existing NonZeroU8/16/32/etc. They didn't want to commit to the contract in case they added future types with different restrictions. If they ever add more bits to u16 though I'll keep an eye out. |
I am unhappy about this. To summarize the situation from my pov:
This is irresponsible because it can lead to security vulnerabilities. It does not make me trust this library. What is the argument against using the safe code here? Back to this specific instance of unsafe: Even if in memory the values of None and 0 are the same (whether this is guaranteed is still unclear to me) the transmute is UB. I'm not very familiar with Rust's guarantees around UB but in other languages like c++ doing things that you know would do the correct thing executing their assembly on hardware even though technically UB is not ok. For example, you might know that adding 1 to the maximum int wraps around on hardware. But this is still UB. If the compiler proves you're executing UB then it is allowed to do anything. Even if the assembly that same compiler generates would do what you intended. This has lead to many real bugs and vulnerabilities. |
I'm fine with the assumption. Logically it has to be 0 otherwise it invalidates their other guarantees, at which point I have other issues. If we assume the current guarantees are silently removed within the same rust edition, and consider the value random over the entire space, it also doesn't matter since this is consumed by a bounds check, which CI would catch. I had some unfavorable optimization with this function in past micro-benchmarks and as a result I'm keeping the cast. |
This is the fallacy I tried to explain with the c++ example. The value of the memory at runtime is not a sufficient condition for this unsafe block being correct. This a common misunderstanding about UB. If the value was different then the unsafe would definitely be wrong and we wouldn't be arguing about this. But if the value is the same, the unsafe might still be wrong. Here is another reference to strengthen my point https://rust-lang.github.io/unsafe-code-guidelines/glossary.html#undefined-behavior and there are some blog posts by Ralf Jung that touch on this: https://www.ralfj.de/blog/2020/07/15/unused-data.html and https://www.ralfj.de/blog/2021/11/18/ub-good-idea.html . If we truly believe that the cast should not be UB then this should be brought up in an issue in the rust repository. I feel they made it absolutely clear that this code is UB so it is probably not a bug and code should not rely on this not being UB. I don't want this discussion to feel like a you vs me thing and I do want to figure what is correct in this situation. I care about this issue because fontdue is good library that seems like it has the goal of being widely used. I recognize that this your library and you are free to do with it as you wish but if the goal is to be used by other people then I find not having UB to be the responsible thing to do. |
I just realized I did not read
carefully enough. We are transmuting |
My conclusion after asking around: I no longer believe there is a serious defect. I still believe it would be better to use the same assembly producing safe version but either choice is no big deal. |
ok |
I checked a couple of uses of unsafe in this crate and found that they can be replaced with safe versions that generate the same assembly with optimizations. There are probably more instances of this.
fontdue/src/platform/float/mod.rs
Lines 35 to 39 in c361446
https://godbolt.org/z/z4x3hEPa3
fontdue/src/font.rs
Lines 611 to 613 in c361446
https://godbolt.org/z/hbz5sjhPa
On that note it is worrying that many uses of unsafe are not documented. I feel their impact should be benchmarked versus safe versions and their correctness should be explained. For example the second link above, with the transmute, seems like undefined behavior to me because I am not sure that Rust guarantees that the layout matches this way.
The text was updated successfully, but these errors were encountered: