-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ConstantTimeEq implementation for [T; N]. #114
base: main
Are you sure you want to change the base?
Conversation
Only when the const-generics feature is enabled. I had to modify a few tests: they were relying on [u8; N] being automatically dereferenced into &[u8] to get the blanket ConstantTimeEq implementation for [T]. But once a blanket implementation is also available for [T; N], Rust attempts to use it instead & then complains that it can't compare arrays of different lengths. I suppose this technically counts as a backwards-compatibility break.
A few notes:
|
src/lib.rs
Outdated
// This loop shouldn't be shortcircuitable, since the compiler | ||
// shouldn't be able to reason about the value of the `u8` | ||
// unwrapped from the `ct_eq` result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the compiler were sufficiently smart, if it noticed x
became 0 it could short circuit the loop because anything & 0
is 0
.
Why not just use a Choice
? (or use the slice impl)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering the same thing; I switched to falling back to the slice implementation. But the slice implementation uses literally the same code for its element-by-element comparison loop:
Lines 299 to 342 in 6b6a81a
impl<T: ConstantTimeEq> ConstantTimeEq for [T] { | |
/// Check whether two slices of `ConstantTimeEq` types are equal. | |
/// | |
/// # Note | |
/// | |
/// This function short-circuits if the lengths of the input slices | |
/// are different. Otherwise, it should execute in time independent | |
/// of the slice contents. | |
/// | |
/// Since arrays coerce to slices, this function works with fixed-size arrays: | |
/// | |
/// ``` | |
/// # use subtle::ConstantTimeEq; | |
/// # | |
/// let a: [u8; 8] = [0,1,2,3,4,5,6,7]; | |
/// let b: [u8; 8] = [0,1,2,3,0,1,2,3]; | |
/// | |
/// let a_eq_a = a.ct_eq(&a); | |
/// let a_eq_b = a.ct_eq(&b); | |
/// | |
/// assert_eq!(a_eq_a.unwrap_u8(), 1); | |
/// assert_eq!(a_eq_b.unwrap_u8(), 0); | |
/// ``` | |
#[inline] | |
fn ct_eq(&self, _rhs: &[T]) -> Choice { | |
let len = self.len(); | |
// Short-circuit on the *lengths* of the slices, not their | |
// contents. | |
if len != _rhs.len() { | |
return Choice::from(0); | |
} | |
// This loop shouldn't be shortcircuitable, since the compiler | |
// shouldn't be able to reason about the value of the `u8` | |
// unwrapped from the `ct_eq` result. | |
let mut x = 1u8; | |
for (ai, bi) in self.iter().zip(_rhs.iter()) { | |
x &= ai.ct_eq(bi).unwrap_u8(); | |
} | |
x.into() | |
} | |
} |
If there's a problem with this code, there may be a pre-existing problem with the implementation for [T]
.
FWIW, I'm pretty comfortable falling back on the [T]
implementation for the [T; N]
implementation: a little experimentation showed that the Rust compiler can reason about slice lengths to optimize away length checks in similar code (but that's no guarantee that it will do so here, of course).
It's kind of crazy how it's able to use a different impl for I'm not sure where I stand on this being a breaking change or not. It definitely seems a lot more correct/type-safe for the impl for |
Only when the const-generics feature is enabled.
I had to modify a few tests: they were relying on [u8; N] being automatically dereferenced into &[u8] to get the blanket ConstantTimeEq implementation for [T]. But once a blanket implementation is also available for [T; N], Rust attempts to use it instead & then complains that it can't compare arrays of different lengths. I suppose this technically counts as a backwards-compatibility break.