| Message ID | 20201130145800.19960-1-rf@opensource.cirrus.com |
|---|---|
| State | Superseded |
| 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
On Mon 2020-11-30 14:57:58, Richard Fitzgerald wrote: > Adds test_sscanf to test various number conversion cases, as > number conversion was previously broken. > > This also tests the simple_strtoxxx() functions exported from > vsprintf.c. It is impressive. Honestly, I do not feel to be expert on testing and mathematics. I am not sure how comprehensive the test is. Also I am not sure what experts would say about the tricks with random numbers. Anyway, this is much more than what I have expected. And it checks great number of variants and corner cases. I suggest only one small change, see below. > --- /dev/null > +++ b/lib/test_scanf.c > @@ -0,0 +1,747 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Test cases for sscanf facility. > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/bitops.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/overflow.h> > +#include <linux/printk.h> > +#include <linux/random.h> > +#include <linux/slab.h> > +#include <linux/string.h> > + > +#include "../tools/testing/selftests/kselftest_module.h" > + > +#define BUF_SIZE 1024 > + > +static unsigned total_tests __initdata; > +static unsigned failed_tests __initdata; > +static char *test_buffer __initdata; > +static char *fmt_buffer __initdata; > +static struct rnd_state rnd_state __initdata; > + > +typedef int (*check_fn)(const void *check_data, const char *string, > + const char *fmt, int n_args, va_list ap); > + > +static void __scanf(4, 6) __init > +_test(check_fn fn, const void *check_data, const char *string, const char *fmt, > + int n_args, ...) > +{ > + va_list ap, ap_copy; > + int ret; > + > + total_tests++; > + > + va_start(ap, n_args); > + va_copy(ap_copy, ap); > + ret = vsscanf(string, fmt, ap_copy); > + va_end(ap_copy); > + > + if (ret != n_args) { > + pr_warn("vsscanf(\"%s\", \"%s\", ...) returned %d expected %d\n", > + string, fmt, ret, n_args); > + goto fail; > + } > + > + ret = (*fn)(check_data, string, fmt, n_args, ap); > + if (ret) > + goto fail; > + > + va_end(ap); > + > + return; > + > +fail: > + failed_tests++; > + va_end(ap); > +} > + > +#define test_one_number(T, gen_fmt, scan_fmt, val, fn) \ > +do { \ > + const T expect_val = (T)(val); \ > + T result = ~expect_val; /* should be overwritten */ \ If I get it correctly, this is supposed to initialize the temporary variable with a value that is different from the expected value. It will cause test failure when it is not updated by vsscanf(). It does not work for zero value. A better solution might be to add a constant, for example: T result = expect_val + 3; /* do not match when not overwritten */ \ I did not use "+ 1" intentionally because it might hide some overflow issues. > + \ > + snprintf(test_buffer, BUF_SIZE, gen_fmt, expect_val); \ > + _test(fn, &expect_val, test_buffer, "%" scan_fmt, 1, &result); \ > +} while (0) Otherwise, it looks good to me. Best Regards, Petr
On Mon 2020-11-30 14:57:59, Richard Fitzgerald wrote: > Adds a wrapper shell script for the test_scanf module. > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr
On 09/12/2020 14:15, Petr Mladek wrote: > On Mon 2020-11-30 14:57:58, Richard Fitzgerald wrote: >> Adds test_sscanf to test various number conversion cases, as >> number conversion was previously broken. >> >> This also tests the simple_strtoxxx() functions exported from >> vsprintf.c. > > It is impressive. > > Honestly, I do not feel to be expert on testing and mathematics. > I am not sure how comprehensive the test is. Also I am not > sure what experts would say about the tricks with random > numbers. > > Anyway, this is much more than what I have expected. And it checks > great number of variants and corner cases. > > I suggest only one small change, see below. > >> --- /dev/null >> +++ b/lib/test_scanf.c >> @@ -0,0 +1,747 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Test cases for sscanf facility. >> + */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> + >> +#include <linux/bitops.h> >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/overflow.h> >> +#include <linux/printk.h> >> +#include <linux/random.h> >> +#include <linux/slab.h> >> +#include <linux/string.h> >> + >> +#include "../tools/testing/selftests/kselftest_module.h" >> + >> +#define BUF_SIZE 1024 >> + >> +static unsigned total_tests __initdata; >> +static unsigned failed_tests __initdata; >> +static char *test_buffer __initdata; >> +static char *fmt_buffer __initdata; >> +static struct rnd_state rnd_state __initdata; >> + >> +typedef int (*check_fn)(const void *check_data, const char *string, >> + const char *fmt, int n_args, va_list ap); >> + >> +static void __scanf(4, 6) __init >> +_test(check_fn fn, const void *check_data, const char *string, const char *fmt, >> + int n_args, ...) >> +{ >> + va_list ap, ap_copy; >> + int ret; >> + >> + total_tests++; >> + >> + va_start(ap, n_args); >> + va_copy(ap_copy, ap); >> + ret = vsscanf(string, fmt, ap_copy); >> + va_end(ap_copy); >> + >> + if (ret != n_args) { >> + pr_warn("vsscanf(\"%s\", \"%s\", ...) returned %d expected %d\n", >> + string, fmt, ret, n_args); >> + goto fail; >> + } >> + >> + ret = (*fn)(check_data, string, fmt, n_args, ap); >> + if (ret) >> + goto fail; >> + >> + va_end(ap); >> + >> + return; >> + >> +fail: >> + failed_tests++; >> + va_end(ap); >> +} >> + >> +#define test_one_number(T, gen_fmt, scan_fmt, val, fn) \ >> +do { \ >> + const T expect_val = (T)(val); \ >> + T result = ~expect_val; /* should be overwritten */ \ > > If I get it correctly, this is supposed to initialize the temporary > variable with a value that is different from the expected value. > It will cause test failure when it is not updated by vsscanf(). > > It does not work for zero value. A better solution might be to add That's a ~, not a - ~0 = 0xFFFFFFFF ~-1 = 0 > a constant, for example: > > T result = expect_val + 3; /* do not match when not overwritten */ \ > > I did not use "+ 1" intentionally because it might hide some overflow > issues. > >> + \ >> + snprintf(test_buffer, BUF_SIZE, gen_fmt, expect_val); \ >> + _test(fn, &expect_val, test_buffer, "%" scan_fmt, 1, &result); \ >> +} while (0) > > Otherwise, it looks good to me. > > Best Regards, > Petr >
On Tue 2020-12-15 14:26:53, Richard Fitzgerald wrote: > > > On 09/12/2020 14:15, Petr Mladek wrote: > > On Mon 2020-11-30 14:57:58, Richard Fitzgerald wrote: > > > Adds test_sscanf to test various number conversion cases, as > > > number conversion was previously broken. > > > > > > This also tests the simple_strtoxxx() functions exported from > > > vsprintf.c. > > > > It is impressive. > > > > Honestly, I do not feel to be expert on testing and mathematics. > > I am not sure how comprehensive the test is. Also I am not > > sure what experts would say about the tricks with random > > numbers. > > > > Anyway, this is much more than what I have expected. And it checks > > great number of variants and corner cases. > > > > I suggest only one small change, see below. > > > > > --- /dev/null > > > +++ b/lib/test_scanf.c > > > +#define test_one_number(T, gen_fmt, scan_fmt, val, fn) \ > > > +do { \ > > > + const T expect_val = (T)(val); \ > > > + T result = ~expect_val; /* should be overwritten */ \ > > > > If I get it correctly, this is supposed to initialize the temporary > > variable with a value that is different from the expected value. > > It will cause test failure when it is not updated by vsscanf(). > > > > It does not work for zero value. A better solution might be to add > > That's a ~, not a - > ~0 = 0xFFFFFFFF > ~-1 = 0 I see. This works well. I am sorry for the noise. Sigh, I think that I need stronger glasses or use a monitor. I am not able to distinguish the two characeters without staring closely on the laptop screen. I wish I was able to work from the office again. 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(-)