On Mon 2020-11-30 14:57:57, Richard Fitzgerald wrote:
> The existing code attempted to handle numbers by doing a strto[u]l(),
> ignoring the field width, and then bitshifting the field out of the
> converted value. If the string contains a run of valid digits longer
> than will fit in a long or long long, this would overflow and no amount
> of bitshifting can recover the correct value.
>
> This patch fixes vsscanf to obey number field widths when parsing
> the number.
>
> A new _parse_integer_limit() is added that takes a limit for the number
> of characters to parse. The number field conversion in vsscanf is changed
> to use this new function.
>
> The cases of a base prefix or leading '-' that is >= the maximum
> field
I have troubles to parse this sentence. It might be because I am
not a native speaker.
> width is handled such that the result of a sccanf is consistent with the
> observed behaviour of userland sscanf.
Anyway, it would be great to explain this on few examples that describe
the corner cases. See also below.
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -39,20 +39,22 @@ const char *_parse_integer_fixup_radix(const char *s,
> unsigned int *base)
>
> /*
> * Convert non-negative integer string representation in explicitly given
> radix
> - * to an integer.
> + * to an integer. A maximum of max_chars characters will be converted.
> + *
> * Return number of characters consumed maybe or-ed with overflow bit.
> * If overflow occurs, result integer (incorrect) is still returned.
> *
> * Don't you dare use this function.
> */
> -unsigned int _parse_integer(const char *s, unsigned int base, unsigned long
> long *p)
> +unsigned int _parse_integer_limit(const char *s, unsigned int base,
> + unsigned long long *p, size_t max_chars)
> {
> unsigned long long res;
> unsigned int rv;
>
> res = 0;
> rv = 0;
> - while (1) {
> + for (; max_chars > 0; max_chars--) {
> unsigned int c = *s;
> unsigned int lc = c | 0x20; /* don't tolower() this line */
> unsigned int val;
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 14c9a6af1b23..21145da468e0 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -53,29 +53,47 @@
> #include <linux/string_helpers.h>
> #include "kstrtox.h"
>
> -/**
> - * simple_strtoull - convert a string to an unsigned long long
> - * @cp: The start of the string
> - * @endp: A pointer to the end of the parsed string will be placed here
> - * @base: The number base to use
> - *
> - * This function has caveats. Please use kstrtoull instead.
> - */
> -unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int
> base)
> +static unsigned long long simple_strntoull(const char *startp, size_t
> max_chars,
> + char **endp, unsigned int base)
> {
> - unsigned long long result;
> + const char *cp;
> + unsigned long long result = 0ULL;
> unsigned int rv;
>
> - cp = _parse_integer_fixup_radix(cp, &base);
> - rv = _parse_integer(cp, base, &result);
> + if (max_chars == 0) {
> + cp = startp;
> + goto out;
> + }
> +
> + cp = _parse_integer_fixup_radix(startp, &base);
> + if ((cp - startp) >= max_chars) {
> + cp = startp + max_chars;
> + goto out;
> + }
> +
> + max_chars -= (cp - startp);
> + rv = _parse_integer_limit(cp, base, &result, max_chars);
> /* FIXME */
> cp += (rv & ~KSTRTOX_OVERFLOW);
> -
> +out:
> if (endp)
> *endp = (char *)cp;
>
> return result;
> }
> +
> +/**
> + * simple_strtoull - convert a string to an unsigned long long
> + * @cp: The start of the string
> + * @endp: A pointer to the end of the parsed string will be placed here
> + * @base: The number base to use
> + *
> + * This function has caveats. Please use kstrtoull instead.
> + */
> +unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int
> base)
> +{
> + return simple_strntoull(cp, UINT_MAX, endp, base);
Please, use INT_MAX everywhere. It is an arbitrary big-enough number.
And INT_MAX is already used in vscnprintf().
> +}
> EXPORT_SYMBOL(simple_strtoull);
>
> /**
> @@ -88,7 +106,7 @@ EXPORT_SYMBOL(simple_strtoull);
> */
> unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base)
> {
> - return simple_strtoull(cp, endp, base);
> + return simple_strntoull(cp, UINT_MAX, endp, base);
Same here.
> }
> EXPORT_SYMBOL(simple_strtoul);
>
> @@ -109,6 +127,19 @@ long simple_strtol(const char *cp, char **endp, unsigned
> int base)
> }
> EXPORT_SYMBOL(simple_strtol);
>
> +static long long simple_strntoll(const char *cp, size_t max_chars, char
> **endp,
> + unsigned int base)
> +{
> + /*
> + * simple_strntoull safely handles receiving max_chars==0 in the
> + * case we start with max_chars==1 and find a '-' prefix.
> + */
> + if (*cp == '-' && max_chars > 0)
> + return -simple_strntoull(cp + 1, max_chars - 1, endp, base);
> +
> + return simple_strntoull(cp, max_chars, endp, base);
> +}
> +
> /**
> * simple_strtoll - convert a string to a signed long long
> * @cp: The start of the string
> @@ -119,10 +150,7 @@ EXPORT_SYMBOL(simple_strtol);
> */
> long long simple_strtoll(const char *cp, char **endp, unsigned int base)
> {
> - if (*cp == '-')
> - return -simple_strtoull(cp + 1, endp, base);
> -
> - return simple_strtoull(cp, endp, base);
> + return simple_strntoll(cp, UINT_MAX, endp, base);
And here.
> }
> EXPORT_SYMBOL(simple_strtoll);
>
> @@ -3433,8 +3461,11 @@ int vsscanf(const char *buf, const char *fmt, va_list
> args)
> str = skip_spaces(str);
>
> digit = *str;
> - if (is_sign && digit == '-')
> + if (is_sign && digit == '-') {
> + if (field_width == 1)
> + break;
This should be handled in a separate patch. It is a subtle change that
is hidden deep inside a big diff.
1. It is quite hard to notice because the above simple_strntoll() was
implemented to return 0 in this case.
2. The commit message describes the situation when all read numbers
overflow unsigned long long. And this is another corner that
was not clearly mentioned.
> digit = *(str + 1);
> + }
>
> if (!digit
> || (base == 16 && !isxdigit(digit))
Otherwise, I really like patch. Thanks a lot for the effort.
I am sorry that it took me so long to look at it. I was pretty busy
last week. I am going to look at the huge 2nd patch tomorrow.
Best Regards,
Petr
