search

linebender / glazier

Deprecated Rust Window Creation Library
Apache License 2.0
206 stars 32 forks source link

Soundness issue with `Counter` #91

Closed richard-uk1 closed 1 year ago

richard-uk1 commented 1 year ago

In Glazier there is a counter struct that wraps an atomic number and provides unique IDs. There is a method on it that returns a NonZeroU, and if the inner number overflows then it will get initialized with zero, which is UB. I don't think the doc comment is sufficient here, unless we mark the methods on the Counter as unsafe. Alternatively we should acquare exclusive access while we check that the number is not u::MAX before we increment it and get our ID. This will lead to a little more contention, but the alternative is UB.

jneem commented 1 year ago

Could we just do NonZero::new(..).unwrap() instead of NonZero::new_unchecked(..)?

raphlinus commented 1 year ago

Sigh. I ran across this when adapting the code for xilem_core. Probably best to use the safe method and panic. However, the argument that it will not overflow is a good one. The counter can only be incremented by 1. Assuming you can do 1 billion operations per second, that means you'll hit an overflow in 585 years. I'm personally comfortable taking that risk.

The real question is the audit cost of "unsafe." It's probably not worth it overall.