From 94f8a30f7b85fb48952c5b251bbf1c6cfc4ebd46 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Sun, 18 Jan 2026 21:26:26 -0700 Subject: [PATCH] ctutils: use new `cmov` trait impls; remove `unsafe` In #1404 impls of `Cmov` and `CmovEq` were added to the `NonZero*` and `Ordering` types and slices thereof, which is what the unsafe code in `ctutils` existed to handle. Now they can be handled like the other types `cmov` already has trait impls for. And with that, we can remove all of the unsafe code in this crate, with all of it having been factored into the `cmov` crate, which makes that the one place to look for `unsafe` auditing. With all the `unsafe` removed, this also adds `forbid(unsafe_code)`. --- Cargo.lock | 8 ++++- ctutils/Cargo.toml | 2 +- ctutils/src/lib.rs | 2 +- ctutils/src/traits/ct_assign.rs | 47 ++++++++++++------------- ctutils/src/traits/ct_neg.rs | 62 ++++++++++++++------------------- ctutils/src/traits/ct_select.rs | 59 ++++++++----------------------- 6 files changed, 72 insertions(+), 108 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b3b48d1b..f44d15ae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -62,6 +62,12 @@ dependencies = [ "proptest", ] +[[package]] +name = "cmov" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df1888fb431ee159b513b5c2f249e03f1c9d788f7bd842927619dbeb88764039" + [[package]] name = "collectable" version = "0.1.0" @@ -81,7 +87,7 @@ dependencies = [ name = "ctutils" version = "0.4.0-pre.4" dependencies = [ - "cmov", + "cmov 0.5.1", "proptest", "subtle", ] diff --git a/ctutils/Cargo.toml b/ctutils/Cargo.toml index c34efb08..371ce875 100644 --- a/ctutils/Cargo.toml +++ b/ctutils/Cargo.toml @@ -17,7 +17,7 @@ edition = "2024" rust-version = "1.85" [dependencies] -cmov = "0.5" +cmov = "0.5.1" # optional dependencies subtle = { version = "2", optional = true, default-features = false } diff --git a/ctutils/src/lib.rs b/ctutils/src/lib.rs index 3c11a82a..42417442 100644 --- a/ctutils/src/lib.rs +++ b/ctutils/src/lib.rs @@ -4,7 +4,7 @@ html_logo_url = "https://raw.githubusercontent.com/RustCrypto/meta/master/logo.svg", html_favicon_url = "https://raw.githubusercontent.com/RustCrypto/meta/master/logo.svg" )] -#![deny(unsafe_code)] +#![forbid(unsafe_code)] // `unsafe` should go in `cmov` #![warn( clippy::borrow_as_ptr, clippy::cast_lossless, diff --git a/ctutils/src/traits/ct_assign.rs b/ctutils/src/traits/ct_assign.rs index 7f3a3274..3f558621 100644 --- a/ctutils/src/traits/ct_assign.rs +++ b/ctutils/src/traits/ct_assign.rs @@ -1,4 +1,4 @@ -use crate::{Choice, CtSelect}; +use crate::Choice; use cmov::Cmov; use core::{ cmp, @@ -8,6 +8,9 @@ use core::{ }, }; +#[cfg(feature = "subtle")] +use crate::CtSelect; + #[cfg(doc)] use core::num::NonZero; @@ -86,29 +89,18 @@ macro_rules! impl_ct_assign_slice_with_cmov { }; } -impl_ct_assign_slice_with_cmov!(i8, i16, i32, i64, i128, u8, u16, u32, u64, u128); -impl_ct_assign_with_cmov!(isize, usize); -impl CtAssignSlice for isize {} -impl CtAssignSlice for usize {} - -/// Impl `CtAssign` using the `CtSelect` trait. -macro_rules! impl_ct_assign_with_ct_select { - ( $($ty:ty),+ ) => { - $( - impl CtAssign for $ty { - #[inline] - fn ct_assign(&mut self, rhs: &Self, choice: Choice) { - *self = Self::ct_select(self, rhs, choice); - } - } - - impl CtAssignSlice for $ty {} - )+ - }; -} - -impl_ct_assign_with_ct_select!( - cmp::Ordering, +// NOTE: impls `CtAssign` and `CtAssignSlice` +impl_ct_assign_slice_with_cmov!( + i8, + i16, + i32, + i64, + i128, + u8, + u16, + u32, + u64, + u128, NonZeroI8, NonZeroI16, NonZeroI32, @@ -118,9 +110,14 @@ impl_ct_assign_with_ct_select!( NonZeroU16, NonZeroU32, NonZeroU64, - NonZeroU128 + NonZeroU128, + cmp::Ordering ); +impl_ct_assign_with_cmov!(isize, usize); +impl CtAssignSlice for isize {} +impl CtAssignSlice for usize {} + impl CtAssign for [T; N] where T: CtAssignSlice, diff --git a/ctutils/src/traits/ct_neg.rs b/ctutils/src/traits/ct_neg.rs index 51b9ffc0..76d5d2bc 100644 --- a/ctutils/src/traits/ct_neg.rs +++ b/ctutils/src/traits/ct_neg.rs @@ -25,14 +25,12 @@ macro_rules! impl_signed_ct_neg { impl CtNeg for $int { #[inline] fn ct_neg(&self, choice: Choice) -> Self { - let neg = -*self; - self.ct_select(&neg, choice) + self.ct_select(&-*self, choice) } #[inline] fn ct_neg_assign(&mut self, choice: Choice) { - let neg = -*self; - self.ct_assign(&neg, choice) + self.ct_assign(&-*self, choice) } } )+ @@ -46,58 +44,52 @@ macro_rules! impl_unsigned_ct_neg { impl CtNeg for $uint { #[inline] fn ct_neg(&self, choice: Choice) -> Self { - let neg = self.wrapping_neg(); - self.ct_select(&neg, choice) + self.ct_select(&self.wrapping_neg(), choice) } #[inline] fn ct_neg_assign(&mut self, choice: Choice) { - let neg = self.wrapping_neg(); - self.ct_assign(&neg, choice) + self.ct_assign(&self.wrapping_neg(), choice) } } )+ }; } -impl_signed_ct_neg!(i8, i16, i32, i64, i128); +impl_signed_ct_neg!( + i8, + i16, + i32, + i64, + i128, + NonZeroI8, + NonZeroI16, + NonZeroI32, + NonZeroI64, + NonZeroI128 +); impl_unsigned_ct_neg!(u8, u16, u32, u64, u128); -/// Impl `CtNeg` for `NonZero` by calling the `CtNeg` impl for `T`. -macro_rules! impl_ct_neg_for_nonzero_integer { - ( $($nzint:ident),+ ) => { +/// Unfortunately `NonZeroU*` doesn't support `wrapping_neg` for some reason (but `NonZeroI*` does), +/// even though the wrapping negation of any non-zero integer should also be non-zero. +/// +/// So we need a special case just for `NonZeroU*`, at least for now. +macro_rules! impl_ct_neg_for_unsigned_nonzero { + ( $($nzuint:ident),+ ) => { $( - impl CtNeg for $nzint { + impl CtNeg for $nzuint { #[inline] fn ct_neg(&self, choice: Choice) -> Self { - let n = self.get().ct_neg(choice); - - // SAFETY: we are constructing `NonZero` from a value we obtained from - // `NonZero::get`, which ensures it's non-zero, and the negation of a non-zero - // integer will always be non-zero: - // - // - signed: `{i*}::MIN` and `{i*}::MAX` are each other's negations - // - unsigned: `1` and `{u*}::MAX` are each other's negations - #[allow(unsafe_code)] - unsafe { $nzint::new_unchecked(n) } + // TODO(tarcieri): use `NonZero::wrapping_neg` if it becomes available + let n = self.get().ct_select(&self.get().wrapping_neg(), choice); + $nzuint::new(n).expect("should be non-zero") } } )+ }; } -impl_ct_neg_for_nonzero_integer!( - NonZeroI8, - NonZeroI16, - NonZeroI32, - NonZeroI64, - NonZeroI128, - NonZeroU8, - NonZeroU16, - NonZeroU32, - NonZeroU64, - NonZeroU128 -); +impl_ct_neg_for_unsigned_nonzero!(NonZeroU8, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU128); #[cfg(test)] mod tests { diff --git a/ctutils/src/traits/ct_select.rs b/ctutils/src/traits/ct_select.rs index 0e5df749..5a7fe3d9 100644 --- a/ctutils/src/traits/ct_select.rs +++ b/ctutils/src/traits/ct_select.rs @@ -92,29 +92,18 @@ macro_rules! impl_ct_select_with_ct_assign { } impl_ct_select_with_ct_assign!( - i8, i16, i32, i64, i128, isize, u8, u16, u32, u64, u128, usize -); - -/// Impl `CtSelect` for `NonZero` by calling the `CtSelect` impl for `T`. -macro_rules! impl_ct_select_for_nonzero_integer { - ( $($nzint:ident),+ ) => { - $( - impl CtSelect for $nzint { - #[inline] - fn ct_select(&self, rhs: &Self, choice: Choice) -> Self { - let n = self.get().ct_select(&rhs.get(), choice); - - // SAFETY: we are constructing `NonZero` from a value we obtained from - // `NonZero::get`, which ensures it's non-zero. - #[allow(unsafe_code)] - unsafe { $nzint::new_unchecked(n) } - } - } - )+ - }; -} - -impl_ct_select_for_nonzero_integer!( + i8, + i16, + i32, + i64, + i128, + isize, + u8, + u16, + u32, + u64, + u128, + usize, NonZeroI8, NonZeroI16, NonZeroI32, @@ -124,30 +113,10 @@ impl_ct_select_for_nonzero_integer!( NonZeroU16, NonZeroU32, NonZeroU64, - NonZeroU128 + NonZeroU128, + cmp::Ordering ); -impl CtSelect for cmp::Ordering { - fn ct_select(&self, other: &Self, choice: Choice) -> Self { - // `Ordering` is `#[repr(i8)]` where: - // - // - `Less` => -1 - // - `Equal` => 0 - // - `Greater` => 1 - // - // Given this, it's possible to operate on orderings as if they're `i8`, which allows us to - // use the `CtSelect` impl on `i8` to select between them. - let ret = (*self as i8).ct_select(&(*other as i8), choice); - - // SAFETY: `Ordering` is `#[repr(i8)]` and `ret` has been assigned to - // a value which was originally a valid `Ordering` then cast to `i8` - #[allow(trivial_casts, unsafe_code)] - unsafe { - *(&raw const ret).cast::() - } - } -} - #[cfg(feature = "subtle")] impl CtSelect for subtle::Choice { #[inline]