| Message ID | 20201130145800.19960-1-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] lib: vsprintf: Fix handling of number field widths in vsscanf | expand |
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
diff --git a/lib/kstrtox.c b/lib/kstrtox.c index a14ccf905055..0ac2ee8bd9d0 100644 --- 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; @@ -82,6 +84,11 @@ unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long return rv; } +unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long *p) +{ + return _parse_integer_limit(s, base, p, INT_MAX); +} + static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res) { unsigned long long _res; diff --git a/lib/kstrtox.h b/lib/kstrtox.h index 3b4637bcd254..4c6536f85cac 100644 --- a/lib/kstrtox.h +++ b/lib/kstrtox.h @@ -4,6 +4,8 @@ #define KSTRTOX_OVERFLOW (1U << 31) const char *_parse_integer_fixup_radix(const char *s, unsigned int *base); +unsigned int _parse_integer_limit(const char *s, unsigned int base, + unsigned long long *res, size_t max_chars); unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long *res); #endif 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); +} 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); } 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); } 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; digit = *(str + 1); + } if (!digit || (base == 16 && !isxdigit(digit)) @@ -3444,25 +3475,13 @@ int vsscanf(const char *buf, const char *fmt, va_list args) break; if (is_sign) - val.s = qualifier != 'L' ? - simple_strtol(str, &next, base) : - simple_strtoll(str, &next, base); + val.s = simple_strntoll(str, + field_width > 0 ? field_width : UINT_MAX, + &next, base); else - val.u = qualifier != 'L' ? - simple_strtoul(str, &next, base) : - simple_strtoull(str, &next, base); - - if (field_width > 0 && next - str > field_width) { - if (base == 0) - _parse_integer_fixup_radix(str, &base); - while (next - str > field_width) { - if (is_sign) - val.s = div_s64(val.s, base); - else - val.u = div_u64(val.u, base); - --next; - } - } + val.u = simple_strntoull(str, + field_width > 0 ? field_width : UINT_MAX, + &next, base); switch (qualifier) { case 'H': /* that's 'hh' in format */
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 width is handled such that the result of a sccanf is consistent with the observed behaviour of userland sscanf. Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> --- lib/kstrtox.c | 13 +++++-- lib/kstrtox.h | 2 ++ lib/vsprintf.c | 93 ++++++++++++++++++++++++++++++-------------------- 3 files changed, 68 insertions(+), 40 deletions(-)