Implementation of NotNan::cmp is unsound

[Earthcomputer]

The current implementation of Ord for NotNan<T> is as follows:

impl<T: FloatCore> Ord for NotNan<T> {
    fn cmp(&self, other: &NotNan<T>) -> Ordering {
        match self.partial_cmp(other) {
            Some(ord) => ord,
            None => unsafe { unreachable_unchecked() },
        }
    }
}

and the implementation of NotNan::<T>::new is:

pub fn new(val: T) -> Result<Self, FloatIsNan> {
    match val {
        ref val if val.is_nan() => Err(FloatIsNan),
        val => Ok(NotNan(val)),
    }
}

this makes it possible to invoke undefined behaviour without using unsafe with the following code:

#[derive(PartialOrd, PartialEq)]
struct EvilFloat(f32);
impl FloatCore for EvilFloat {
    fn is_nan(self) -> bool {
        false
    }
    // ...
}

fn main() {
    let evil_value = NotNan::new(EvilFloat(f32::NAN)).unwrap();
    let x = NotNan::new(EvilFloat(0.0)).unwrap();
    evil_value.cmp(&x);
}

Full test case: https://gist.github.com/Earthcomputer/27d11c9c70c1335ae29cdf187362c4dd

Miri run:

error: Undefined Behavior: entering unreachable code
    --> /home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ordered-float-4.2.0/src/lib.rs:1168:30
     |
1168 |             None => unsafe { unreachable_unchecked() },
     |                              ^^^^^^^^^^^^^^^^^^^^^^^ entering unreachable code
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: BACKTRACE:
     = note: inside `<ordered_float::NotNan<EvilFloat> as std::cmp::Ord>::cmp` at /home/joe/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ordered-float-4.2.0/src/lib.rs:1168:30: 1168:53
note: inside `main`
    --> src/main.rs:128:5
     |
128  |     evil_value.cmp(&x);
     |     ^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

[mbrubeck]

Thanks! This is fixed in version 4.2.1.