Skip to content

Commit

Permalink
Fix soundness bug in NotNan::partial_cmp.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mbrubeck committed Jun 29, 2024
1 parent 8eb3455 commit 9593cb8
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 30 deletions.
17 changes: 8 additions & 9 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -1163,10 +1162,10 @@ impl Borrow<f64> for NotNan<f64> {
#[allow(clippy::derive_ord_xor_partial_ord)]
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() },
}
// 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")
}
}

Expand Down Expand Up @@ -2535,7 +2534,7 @@ mod impl_rand {
fn uniform_sampling_panic_on_infinity_notnan() {
let (low, high) = (
NotNan::new(0f64).unwrap(),
NotNan::new(core::f64::INFINITY).unwrap(),
NotNan::new(f64::INFINITY).unwrap(),
);
let uniform = Uniform::new(low, high);
let _ = uniform.sample(&mut rand::thread_rng());
Expand All @@ -2544,15 +2543,15 @@ mod impl_rand {
#[test]
#[should_panic]
fn uniform_sampling_panic_on_infinity_ordered() {
let (low, high) = (OrderedFloat(0f64), OrderedFloat(core::f64::INFINITY));
let (low, high) = (OrderedFloat(0f64), OrderedFloat(f64::INFINITY));
let uniform = Uniform::new(low, high);
let _ = uniform.sample(&mut rand::thread_rng());
}

#[test]
#[should_panic]
fn uniform_sampling_panic_on_nan_ordered() {
let (low, high) = (OrderedFloat(0f64), OrderedFloat(core::f64::NAN));
let (low, high) = (OrderedFloat(0f64), OrderedFloat(f64::NAN));
let uniform = Uniform::new(low, high);
let _ = uniform.sample(&mut rand::thread_rng());
}
Expand Down Expand Up @@ -2689,7 +2688,7 @@ mod impl_bytemuck {
Err(CheckedCastError::InvalidBitPattern),
);

let pi = core::f64::consts::PI;
let pi = f64::consts::PI;

Check failure on line 2691 in src/lib.rs

View workflow job for this annotation

GitHub Actions / Tests (stable)

ambiguous associated type

Check failure on line 2691 in src/lib.rs

View workflow job for this annotation

GitHub Actions / Tests (stable)

ambiguous associated type
assert!(try_cast::<f64, NotNan<f64>>(pi).is_ok());
}
}
42 changes: 21 additions & 21 deletions tests/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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();
}
Expand Down

0 comments on commit 9593cb8

Please sign in to comment.