[PATCH v2 0/2] rust: str: parse_int: match kstrtol() sign and newline handling

[PATCH v2 0/2] rust: str: parse_int: match kstrtol() sign and newline handling

[Younes Akhouayri via B4 Relay]

The documentation of `ParseInt` says that any string parsed by
`kstrtol()` or `kstrtoul()` is also parsed by `from_str()`. That does
not hold today, and `from_str()` also accepts strings that `kstrtol()`
rejects:

    input     kstrtol()     i32::from_str()
    "0x-5"    -EINVAL       Ok(-5)
    "0x+5"    -EINVAL       Ok(5)
    "-+5"     -EINVAL       Ok(-5)
    "+0x5"    5             Err(EINVAL)
    "5\n"     5             Err(EINVAL)

Patch 1 accepts a sign only before the radix prefix. Patch 2 accepts a
single trailing newline, as `kstrtol()` does. The `0o` and `0b`
prefixes are Rust extensions and are left as they are.

Nothing in-tree depends on the old behaviour except the `rust_minimal`
sample, so this is meant for rust-next. The `Fixes:` tags record where
the behaviour comes from; I am not asking for a stable backport.

Tested on UML with the Rust KUnit suite, plus a temporary KUnit test
(not part of this series) that compares `i32::from_str()` with
`kstrtoint()`, a thin wrapper around `kstrtoll()`, on 13 inputs; all of
them agree. Also booted the `rust_minimal` sample: `test_parameter=0x-5`
is now rejected and `test_parameter=+0x5` gives 5.

Signed-off-by: Younes Akhouayri <git@younes.io>
---
Changes in v2:
- Add a doctest for "-0x+5" to patch 1 (Miguel).
- Refer to kstrtol() instead of kstrtoint() in the commit messages and
  the cover letter (Miguel).
- Say in the strip_radix() doc comment which from_str_radix() is meant
  and why a sign is rejected there (Miguel).
- Note the rust-next target and that no stable backport is requested.
- Link to v1: https://patch.msgid.link/20261027-b4-parse-int-sign-v1-0-7f386318a4b7@younes.io

---
Younes Akhouayri (2):
      rust: str: parse_int: only accept a sign before the radix prefix
      rust: str: parse_int: accept a trailing newline like kstrtol()

 rust/kernel/str/parse_int.rs | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)
---
base-commit: 7d109178cd644268326cd31702ed46c7ac8bb300
change-id: 20261027-b4-parse-int-sign-36e899fc4f43

Best regards,
--  
Younes Akhouayri <git@younes.io>

[PATCH v2 1/2] rust: str: parse_int: only accept a sign before the radix prefix

[Younes Akhouayri via B4 Relay]

From: Younes Akhouayri <git@younes.io>

`ParseInt::from_str()` strips an optional `-` and a radix prefix, then
hands the remaining digits to `from_str_radix()` from `core`. That
function accepts a leading `+`, and also `-` for signed types, so a
sign placed after the prefix, or a second sign, is parsed too:

    i32::from_str("0x-5")  == Ok(-5)
    i32::from_str("0x+5")  == Ok(5)
    i32::from_str("-+5")   == Ok(-5)
    i32::from_str("-0x+5") == Ok(-5)

`kstrtol()` rejects all of these with -EINVAL. On the other hand,
`kstrtol()` accepts "+0x5" as 5, but `from_str()` rejects it, because
it does not strip a leading `+` before looking for the prefix. Since
the documentation says that any string parsed by `kstrtol()` is also
parsed by `from_str()`, this is a bug.

Integer module parameters are parsed with `from_str()`, so this is
visible from the kernel command line. Booting with
`rust_minimal.test_parameter=0x-5` makes the `rust_minimal` sample
print `test_parameter: -5`.

Fix it by accepting a single `+` or `-` before the radix prefix only,
and by making `strip_radix()` reject digits that start with a sign.
Add doctests for these cases.

Fixes: 51d9ee90ea90 ("rust: str: add radix prefixed integer parsing functions")
Signed-off-by: Younes Akhouayri <git@younes.io>
---
 rust/kernel/str/parse_int.rs | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/str/parse_int.rs b/rust/kernel/str/parse_int.rs
index 48eb4c202984..66f590251ffc 100644
--- a/rust/kernel/str/parse_int.rs
+++ b/rust/kernel/str/parse_int.rs
@@ -28,8 +28,13 @@ pub trait FromStrRadix: Sized {
 
 /// Extract the radix from an integer literal optionally prefixed with
 /// one of `0x`, `0X`, `0o`, `0O`, `0b`, `0B`, `0`.
-fn strip_radix(src: &BStr) -> (u32, &BStr) {
-    match src.deref() {
+///
+/// Returns [`EINVAL`] if the remaining digits start with a sign. The
+/// caller passes them to the `from_str_radix()` functions of the
+/// primitive integer types in `core`, which accept a leading `+` or `-`
+/// of their own, so a sign here would make `0x-5` or `-+5` parse.
+fn strip_radix(src: &BStr) -> Result<(u32, &BStr)> {
+    let (radix, digits) = match src.deref() {
         [b'0', b'x' | b'X', rest @ ..] => (16, rest.as_ref()),
         [b'0', b'o' | b'O', rest @ ..] => (8, rest.as_ref()),
         [b'0', b'b' | b'B', rest @ ..] => (2, rest.as_ref()),
@@ -38,6 +43,11 @@ fn strip_radix(src: &BStr) -> (u32, &BStr) {
         // not be able to parse `0`.
         [b'0', ..] => (8, src),
         _ => (10, src),
+    };
+
+    match digits.deref() {
+        [b'+' | b'-', ..] => Err(EINVAL),
+        _ => Ok((radix, digits)),
     }
 }
 
@@ -75,13 +85,19 @@ fn strip_radix(src: &BStr) -> (u32, &BStr) {
 /// assert!(i8::from_str(b_str!("-129")).is_err());
 /// assert_eq!(Ok(255u8), u8::from_str(b_str!("255")));
 /// assert!(u8::from_str(b_str!("256")).is_err());
+///
+/// assert_eq!(Ok(0x5i32), i32::from_str(b_str!("+0x5")));
+/// assert!(i32::from_str(b_str!("0x-5")).is_err());
+/// assert!(i32::from_str(b_str!("0x+5")).is_err());
+/// assert!(i32::from_str(b_str!("-+5")).is_err());
+/// assert!(i32::from_str(b_str!("-0x+5")).is_err());
 /// ```
 pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
     /// Parse a string according to the description in [`Self`].
     fn from_str(src: &BStr) -> Result<Self> {
         match src.deref() {
             [b'-', rest @ ..] => {
-                let (radix, digits) = strip_radix(rest.as_ref());
+                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
@@ -94,8 +110,8 @@ fn from_str(src: &BStr) -> Result<Self> {
                         .map_err(|_| EINVAL)?;
                 Self::from_u64_negated(val)
             }
-            _ => {
-                let (radix, digits) = strip_radix(src);
+            [b'+', rest @ ..] | rest => {
+                let (radix, digits) = strip_radix(rest.as_ref())?;
                 Self::from_str_radix(digits, radix).map_err(|_| EINVAL)
             }
         }

-- 
2.56.0

[PATCH v2 2/2] rust: str: parse_int: accept a trailing newline like kstrtol()

[Younes Akhouayri via B4 Relay]

From: Younes Akhouayri <git@younes.io>

The documentation of `ParseInt` says that any string parsed by
`kstrtol()` or `kstrtoul()` is also parsed by `from_str()`. However,
those functions accept a single trailing newline, and `from_str()` does
not:

    kstrtol("5\n", 0, &v)   == 0, v == 5
    i32::from_str("5\n")    == Err(EINVAL)

A trailing newline is common when the string comes from user space, for
example when a value is written to a sysfs file with `echo`.

Fix it by removing a single trailing newline before parsing, and add
doctests for it.

Fixes: 51d9ee90ea90 ("rust: str: add radix prefixed integer parsing functions")
Signed-off-by: Younes Akhouayri <git@younes.io>
---
 rust/kernel/str/parse_int.rs | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/str/parse_int.rs b/rust/kernel/str/parse_int.rs
index 66f590251ffc..5b13925c328e 100644
--- a/rust/kernel/str/parse_int.rs
+++ b/rust/kernel/str/parse_int.rs
@@ -56,8 +56,8 @@ fn strip_radix(src: &BStr) -> Result<(u32, &BStr)> {
 /// Strings beginning with `0x`, `0o`, or `0b` are parsed as hex, octal, or
 /// binary respectively. Strings beginning with `0` otherwise are parsed as
 /// octal. Anything else is parsed as decimal. A leading `+` or `-` is also
-/// permitted. Any string parsed by [`kstrtol()`] or [`kstrtoul()`] will be
-/// successfully parsed.
+/// permitted, as is a single trailing newline. Any string parsed by
+/// [`kstrtol()`] or [`kstrtoul()`] will be successfully parsed.
 ///
 /// [`kstrtol()`]: https://docs.kernel.org/core-api/kernel-api.html#c.kstrtol
 /// [`kstrtoul()`]: https://docs.kernel.org/core-api/kernel-api.html#c.kstrtoul
@@ -91,11 +91,15 @@ fn strip_radix(src: &BStr) -> Result<(u32, &BStr)> {
 /// assert!(i32::from_str(b_str!("0x+5")).is_err());
 /// assert!(i32::from_str(b_str!("-+5")).is_err());
 /// assert!(i32::from_str(b_str!("-0x+5")).is_err());
+///
+/// assert_eq!(Ok(5u8), u8::from_str(b_str!("5\n")));
+/// assert!(u8::from_str(b_str!("5\n\n")).is_err());
 /// ```
 pub trait ParseInt: private::FromStrRadix + TryFrom<u64> {
     /// Parse a string according to the description in [`Self`].
     fn from_str(src: &BStr) -> Result<Self> {
-        match src.deref() {
+        let src = src.strip_suffix(b"\n").unwrap_or(src);
+        match src {
             [b'-', rest @ ..] => {
                 let (radix, digits) = strip_radix(rest.as_ref())?;
                 // 2's complement values range from -2^(b-1) to 2^(b-1)-1.

-- 
2.56.0

Re: [PATCH v2 0/2] rust: str: parse_int: match kstrtol() sign and newline handling

[Andreas Hindborg]

"Younes Akhouayri via B4 Relay" <devnull+git.younes.io@kernel.org>
writes:

> The documentation of `ParseInt` says that any string parsed by
> `kstrtol()` or `kstrtoul()` is also parsed by `from_str()`. That does
> not hold today, and `from_str()` also accepts strings that `kstrtol()`
> rejects:
>
>     input     kstrtol()     i32::from_str()
>     "0x-5"    -EINVAL       Ok(-5)
>     "0x+5"    -EINVAL       Ok(5)
>     "-+5"     -EINVAL       Ok(-5)
>     "+0x5"    5             Err(EINVAL)
>     "5\n"     5             Err(EINVAL)

The documentation says that `from_str` will parse at least the set
parsed by `kstrtol`, so the first three cases are in line with the
documentation.

For the last two lines, we can strip leading "+" and trailing newline
before invoking the existing implementation.

Best regards,
Andreas Hindborg