On Thu Jun 19, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
> I'm having a difficult time parsing. Are you suggesting that we guard
> against implementations of `TryInto<u64>` that misbehave?
Let me try a different explanation:
The safety requirement for implementing the `FromStrRadix`:
/// The member functions of this trait must be implemented according to
/// their documentation.
Together with the functions of the trait:
/// Parse `src` to [`Self`] using radix `radix`.
fn from_str_radix(src: &BStr, radix: u32) -> Result<Self,
crate::error::Error>;
/// Return the absolute value of [`Self::MIN`].
fn abs_min() -> u64;
/// Perform bitwise 2's complement on `self`.
///
/// Note: This function does not make sense for unsigned integers.
fn complement(self) -> Self;
Doesn't make sense. What does it mean to return the "absolute value of
[`Self::MIN`]"? We don't have "absolute value" defined for an arbitrary
type. Similarly for `complement` and `from_str_radix`, what does "Parse
`src` to [`Self`] using radex `radix`" mean? It's not well-defined.
You use this safety requirement in the parsing branch for negative
numbers (the `unsafe` call at the bottom):
[b'-', rest @ ..] => {
let (radix, digits) = strip_radix(rest.as_ref());
// 2's complement values range from -2^(b-1) to 2^(b-1)-1.
// So if we want to parse negative numbers as positive and
// later multiply by -1, we have to parse into a larger
// integer. We choose `u64` as sufficiently large.
//
// NOTE: 128 bit integers are not available on all
// platforms, hence the choice of 64 bits.
let val =
u64::from_str_radix(core::str::from_utf8(digits).map_err(|_|
EINVAL)?, radix)
.map_err(|_| EINVAL)?;
if val > Self::abs_min() {
return Err(EINVAL);
}
if val == Self::abs_min() {
return Ok(Self::MIN);
}
// SAFETY: We checked that `val` will fit in `Self` above.
let val: Self = unsafe { val.try_into().unwrap_unchecked() };
Ok(val.complement())
}
But you don't mention that the check is valid due to the safety
requirements of implementing `FromStrRadix`. But even if you did, that
wouldn't mean anything as I explained above.
So let's instead move all of this negation & u64 conversion logic into
the `FromStrRadix` trait. Then it can be safe & the `ParseInt::from_str`
function doesn't use `unsafe` (there still will be `unsafe` in the
macro, but that is fine, as it's more local and knows the concrete
types).
---
Cheers,
Benno
Here is what I have in mind:
diff --git a/rust/kernel/str/parse_int.rs b/rust/kernel/str/parse_int.rs
index 0754490aec4b..9d6e146c5ea7 100644
--- a/rust/kernel/str/parse_int.rs
+++ b/rust/kernel/str/parse_int.rs
@@ -13,32 +13,16 @@
// `ParseInt`, that is, prevents downstream users from implementing the
// trait.
mod private {
+ use crate::prelude::*;
use crate::str::BStr;
/// Trait that allows parsing a [`&BStr`] to an integer with a radix.
- ///
- /// # Safety
- ///
- /// The member functions of this trait must be implemented according to
- /// their documentation.
- ///
- /// [`&BStr`]: kernel::str::BStr
- // This is required because the `from_str_radix` function on the primitive
- // integer types is not part of any trait.
- pub unsafe trait FromStrRadix: Sized {
- /// The minimum value this integer type can assume.
- const MIN: Self;
-
+ pub trait FromStrRadix: Sized {
/// Parse `src` to [`Self`] using radix `radix`.
- fn from_str_radix(src: &BStr, radix: u32) -> Result<Self,
crate::error::Error>;
-
- /// Return the absolute value of [`Self::MIN`].
- fn abs_min() -> u64;
+ fn from_str_radix(src: &BStr, radix: u32) -> Result<Self>;
- /// Perform bitwise 2's complement on `self`.
- ///
- /// Note: This function does not make sense for unsigned integers.
- fn complement(self) -> Self;
+ /// Tries to convert `value` into [`Self`] and negates the resulting
value.
+ fn from_u64_negated(value: u64) -> Result<Self>;
}
}
@@ -108,19 +92,7 @@ fn from_str(src: &BStr) -> Result<Self> {
let val =
u64::from_str_radix(core::str::from_utf8(digits).map_err(|_| EINVAL)?, radix)
.map_err(|_| EINVAL)?;
-
- if val > Self::abs_min() {
- return Err(EINVAL);
- }
-
- if val == Self::abs_min() {
- return Ok(Self::MIN);
- }
-
- // SAFETY: We checked that `val` will fit in `Self` above.
- let val: Self = unsafe { val.try_into().unwrap_unchecked() };
-
- Ok(val.complement())
+ Self::from_u64_negated(val)
}
_ => {
let (radix, digits) = strip_radix(src);
@@ -131,41 +103,49 @@ fn from_str(src: &BStr) -> Result<Self> {
}
macro_rules! impl_parse_int {
- ($ty:ty) => {
- // SAFETY: We implement the trait according to the documentation.
- unsafe impl private::FromStrRadix for $ty {
- const MIN: Self = <$ty>::MIN;
-
- fn from_str_radix(src: &BStr, radix: u32) -> Result<Self,
crate::error::Error> {
- <$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_|
EINVAL)?, radix)
- .map_err(|_| EINVAL)
- }
-
- fn abs_min() -> u64 {
- #[allow(unused_comparisons)]
- if Self::MIN < 0 {
- 1u64 << (Self::BITS - 1)
- } else {
- 0
+ ($($ty:ty),*) => {
+ $(
+ impl private::FromStrRadix for $ty {
+ fn from_str_radix(src: &BStr, radix: u32) -> Result<Self> {
+
<$ty>::from_str_radix(core::str::from_utf8(src).map_err(|_| EINVAL)?, radix)
+ .map_err(|_| EINVAL)
}
- }
- fn complement(self) -> Self {
- (!self).wrapping_add((1 as $ty))
+ fn from_u64_negated(value: u64) -> Result<Self> {
+ const ABS_MIN: u64 = {
+ #[allow(unused_comparisons)]
+ if <$ty>::MIN < 0 {
+ 1u64 << (Self::BITS - 1)
+ } else {
+ 0
+ }
+ };
+
+ fn complement(self) -> Self {
+ (!self).wrapping_add((1 as $ty))
+ }
+ if val > ABS_MIN {
+ return Err(EINVAL);
+ }
+
+ if val == ABS_MIN {
+ return Ok(<$ty>::MIN);
+ }
+
+ // SAFETY: The above checks guarantee that `val` fits into
`Self`:
+ // - if `Self` is unsigned, then `ABS_MIN == 0` and thus
we have returned above
+ // (either `EINVAL` or `MIN`).
+ // - if `Self` is signed, then we have that `0 <= val <
ABS_MIN`. And since
+ // `ABS_MIN - 1` fits into `Self` by construction, `val`
also does.
+ let val: Self = unsafe { val.try_into().unwrap_unchecked()
};
+
+ Ok((!val).wrapping_add(1))
+ }
}
- }
- impl ParseInt for $ty {}
+ impl ParseInt for $ty {}
+ )*
};
}
-impl_parse_int!(i8);
-impl_parse_int!(u8);
-impl_parse_int!(i16);
-impl_parse_int!(u16);
-impl_parse_int!(i32);
-impl_parse_int!(u32);
-impl_parse_int!(i64);
-impl_parse_int!(u64);
-impl_parse_int!(isize);
-impl_parse_int!(usize);
+impl_parse_int![i8, u8, i16, u16, i32, u32, i64, u64, isize, usize];
