From 37cd73b8ab03142fb560cdce42e0ac47613085ff Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Sat, 29 Jun 2024 12:05:47 -0700 Subject: [PATCH] Fix soundness bug in NotNan::partial_cmp. Ill-behaved FloatCore implementations for user types could have `x.partial_cmp(&x) == None` even when `x.is_nan() == false`. This crate will now panic in those cases, rather than execute undefined behavior. Fixes #150. --- src/lib.rs | 9 ++++----- tests/test.rs | 42 +++++++++++++++++++++--------------------- 2 files changed, 25 insertions(+), 26 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index baa067f..bafa276 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -15,7 +15,6 @@ use core::cmp::Ordering; use core::convert::TryFrom; use core::fmt; use core::hash::{Hash, Hasher}; -use core::hint::unreachable_unchecked; use core::iter::{Product, Sum}; use core::num::FpCategory; use core::ops::{ @@ -1163,10 +1162,10 @@ impl Borrow for NotNan { #[allow(clippy::derive_ord_xor_partial_ord)] impl Ord for NotNan { fn cmp(&self, other: &NotNan) -> Ordering { - match self.partial_cmp(other) { - Some(ord) => ord, - None => unsafe { unreachable_unchecked() }, - } + // Can't use unreachable_unchecked because unsafe code can't depend on FloatCore impl. + // https://github.com/reem/rust-ordered-float/issues/150 + self.partial_cmp(other) + .expect("partial_cmp failed for non-NaN value") } } diff --git a/tests/test.rs b/tests/test.rs index 5626d1c..014acc0 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -576,72 +576,72 @@ fn hash_is_good_for_fractional_numbers() { #[test] #[should_panic] fn test_add_fails_on_nan() { - let a = not_nan(std::f32::INFINITY); - let b = not_nan(std::f32::NEG_INFINITY); + let a = not_nan(f32::INFINITY); + let b = not_nan(f32::NEG_INFINITY); let _c = a + b; } #[test] #[should_panic] fn test_add_fails_on_nan_ref() { - let a = not_nan(std::f32::INFINITY); - let b = not_nan(std::f32::NEG_INFINITY); + let a = not_nan(f32::INFINITY); + let b = not_nan(f32::NEG_INFINITY); let _c = a + &b; } #[test] #[should_panic] fn test_add_fails_on_nan_ref_ref() { - let a = not_nan(std::f32::INFINITY); - let b = not_nan(std::f32::NEG_INFINITY); + let a = not_nan(f32::INFINITY); + let b = not_nan(f32::NEG_INFINITY); let _c = &a + &b; } #[test] #[should_panic] fn test_add_fails_on_nan_t_ref() { - let a = not_nan(std::f32::INFINITY); - let b = std::f32::NEG_INFINITY; + let a = not_nan(f32::INFINITY); + let b = f32::NEG_INFINITY; let _c = a + &b; } #[test] #[should_panic] fn test_add_fails_on_nan_ref_t_ref() { - let a = not_nan(std::f32::INFINITY); - let b = std::f32::NEG_INFINITY; + let a = not_nan(f32::INFINITY); + let b = f32::NEG_INFINITY; let _c = &a + &b; } #[test] #[should_panic] fn test_add_fails_on_nan_ref_t() { - let a = not_nan(std::f32::INFINITY); - let b = std::f32::NEG_INFINITY; + let a = not_nan(f32::INFINITY); + let b = f32::NEG_INFINITY; let _c = &a + b; } #[test] #[should_panic] fn test_add_assign_fails_on_nan_ref() { - let mut a = not_nan(std::f32::INFINITY); - let b = not_nan(std::f32::NEG_INFINITY); + let mut a = not_nan(f32::INFINITY); + let b = not_nan(f32::NEG_INFINITY); a += &b; } #[test] #[should_panic] fn test_add_assign_fails_on_nan_t_ref() { - let mut a = not_nan(std::f32::INFINITY); - let b = std::f32::NEG_INFINITY; + let mut a = not_nan(f32::INFINITY); + let b = f32::NEG_INFINITY; a += &b; } #[test] #[should_panic] fn test_add_assign_fails_on_nan_t() { - let mut a = not_nan(std::f32::INFINITY); - let b = std::f32::NEG_INFINITY; + let mut a = not_nan(f32::INFINITY); + let b = f32::NEG_INFINITY; a += b; } @@ -678,15 +678,15 @@ fn ordered_f64_neg() { #[test] #[should_panic] fn test_sum_fails_on_nan() { - let a = not_nan(std::f32::INFINITY); - let b = not_nan(std::f32::NEG_INFINITY); + let a = not_nan(f32::INFINITY); + let b = not_nan(f32::NEG_INFINITY); let _c: NotNan<_> = [a, b].iter().sum(); } #[test] #[should_panic] fn test_product_fails_on_nan() { - let a = not_nan(std::f32::INFINITY); + let a = not_nan(f32::INFINITY); let b = not_nan(0f32); let _c: NotNan<_> = [a, b].iter().product(); }