Occurrences of unsafe can be replaced with safe versions

[e00E]

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

  	/// Checks if the high bit 0x80000000 is not set on a float.
  	#[inline(always)]
  	pub fn is_positive(value: f32) -> bool {
  	unsafe { transmute::<f32, u32>(value) < 0x80000000 }
  	}

  
  https://godbolt.org/z/z4x3hEPa3

• fontdue/src/font.rs

  Lines 611 to 613 in c361446

  	pub fn lookup_glyph_index(&self, character: char) -> u16 {
  	unsafe { mem::transmute::<Option<NonZeroU16>, u16>(self.char_to_glyph.get(&character).copied()) }
  	}

  
  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.

[mooman219]

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 NonZeroU16's layout is the same as a u16, it's in the official docs. I'll add a comment.

[e00E]

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.
What I am worried more about is the transmute from Option to u16. Even if it was guaranteed that the NonZeroU16 layout matched, it is still not guaranteed that None has the same bit pattern as 0u16.
I agree that this should hold in practice because of niche optimization but it is still wrong to rely on it in a way that can cause UB. If the safe version compiles to the same assembly then why not use it?

[mooman219]

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
https://doc.rust-lang.org/std/option/#representation

It is guaranteed that NonZeroU16 has the same layout as u16.
NonZero### are also transparent over the inner value, meaning None can't ever not be 0.
All nonzero_integers follow this behavior and this is the intended use case.
If this behavior changes in the future it will require a new rust edition.

[e00E]

The second link explicitly states

It is further guaranteed that, for the cases above, one can mem::transmute from all valid values of T to Option and from Some::(_) to T (but transmuting None:: to T is undefined behaviour).

[mooman219]

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.

[e00E]

I am unhappy about this. To summarize the situation from my pov:

• This library uses unsafe code that is explicitly documented by the standard library to be undefined behavior.
• There exists safe code that optimizes to the same assembly as the unsafe code.
• This library is a font renderer making it likely to take untrusted input.

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.

[mooman219]

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.

[e00E]

Logically it has to be 0 otherwise it invalidates their other guarantees, at which point I have other issues.

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.
If the compiler knows that this transmute is UB if the Option is None, and it can prove that an invocation of this function runs this transmute with None as the input then the compiler can make this invocation do anything. It is merely luck that in the None case the transmute works correctly at the moment.

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.
For this reason I might ask other people's opinion on this issue so they hopefully either convince me that the transmute is fine or you that it should be replaced. I'm saying in advance to explain why a third party might suddenly comment here.

[e00E]

I just realized I did not read

but transmuting None:: to T is undefined behaviour

carefully enough. We are transmuting None::<NonZeroU16> to u16 not to NonZeroU16 so I no longer believe that the documentation clearly states that this is UB.

[e00E]

My conclusion after asking around:
This not UB based on current compiler internals but also not guaranteed not UB based on current documentation. It is something that is intended you can do with these types and likely to be guaranteed not UB at some point in the future.

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.

[mooman219]

ok