linebender / druid

A data-first Rust-native UI design toolkit.
https://linebender.org/druid/
Apache License 2.0
9.58k stars 567 forks source link

`Counter::next_nonzero` can have undefined behavior #2198

Open i509VCB opened 2 years ago

i509VCB commented 2 years ago

Per the documentation of AtomicU64::fetch_add

This operation wraps around on overflow.

This means that the following function will return zero if fetch_add is called U64::MAX times which violates the safety requirements for NonZeroU64::new_unchecked.

    /// Return the next value, as a `NonZeroU64`.
    pub fn next_nonzero(&self) -> NonZeroU64 {
        // safe because our initial value is 1 and can only be incremented.
        unsafe { NonZeroU64::new_unchecked(self.0.fetch_add(1, Ordering::Relaxed)) }
    }
raphlinus commented 2 years ago

This is a fun one, I'm pretty sure we discussed it. Because we only add 1, assuming 1ns per increment, it would take 213503.982335 years for it to wrap. So it wouldn't satisfy a theorem prover, but in this case I think it's reasonable to accept the safety justification.

cmyr commented 2 years ago

Good catch, and I do think that the justification (stated well by raph) could at least be included in the doc comment.