* [PATCH v4 1/4] lib: vsprintf: scanf: Negative number must have field width > 1
@ 2021-02-03 16:50 Richard Fitzgerald
2021-02-03 16:50 ` [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf Richard Fitzgerald
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Richard Fitzgerald @ 2021-02-03 16:50 UTC (permalink / raw)
To: pmladek, rostedt, sergey.senozhatsky, andriy.shevchenko, linux, shuah
Cc: linux-kernel, linux-kselftest, patches, Richard Fitzgerald
If a signed number field starts with a '-' the field width must be > 1,
or unlimited, to allow at least one digit after the '-'.
This patch adds a check for this. If a signed field starts with '-'
and field_width == 1 the scanf will quit.
It is ok for a signed number field to have a field width of 1 if it
starts with a digit. In that case the single digit can be converted.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
lib/vsprintf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..28bb26cd1f67 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3434,8 +3434,12 @@ 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))
--
2.20.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf
2021-02-03 16:50 [PATCH v4 1/4] lib: vsprintf: scanf: Negative number must have field width > 1 Richard Fitzgerald
@ 2021-02-03 16:50 ` Richard Fitzgerald
2021-02-03 19:45 ` Andy Shevchenko
2021-02-03 16:50 ` [PATCH v4 3/4] lib: test_scanf: Add tests for sscanf number conversion Richard Fitzgerald
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Richard Fitzgerald @ 2021-02-03 16:50 UTC (permalink / raw)
To: pmladek, rostedt, sergey.senozhatsky, andriy.shevchenko, linux, shuah
Cc: linux-kernel, linux-kselftest, patches, Richard Fitzgerald
The existing code attempted to handle numbers by doing a strto[u]l(),
ignoring the field width, and then repeatedly dividing to extract the
field out of the full 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 dividing 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.
If a number starts with a radix prefix, the field width must be long
enough for at last one digit after the prefix. If not, it will be handled
like this:
sscanf("0x4", "%1i", &i): i=0, scanning continues with the 'x'
sscanf("0x4", "%2i", &i): i=0, scanning continues with the '4'
This is consistent with the observed behaviour of userland sscanf.
Note that this patch does NOT fix the problem of a single field value
overflowing the target type. So for example:
sscanf("123456789abcdef", "%x", &i);
Will not produce the correct result because the value obviously overflows
INT_MAX. But sscanf will report a successful conversion.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
lib/kstrtox.c | 13 ++++++--
lib/kstrtox.h | 2 ++
lib/vsprintf.c | 88 +++++++++++++++++++++++++++++---------------------
3 files changed, 64 insertions(+), 39 deletions(-)
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index a118b0b1e9b2..e6d14945df4f 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 28bb26cd1f67..6afc47575331 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);
@@ -3449,25 +3477,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 */
--
2.20.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 3/4] lib: test_scanf: Add tests for sscanf number conversion
2021-02-03 16:50 [PATCH v4 1/4] lib: vsprintf: scanf: Negative number must have field width > 1 Richard Fitzgerald
2021-02-03 16:50 ` [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf Richard Fitzgerald
@ 2021-02-03 16:50 ` Richard Fitzgerald
2021-02-03 19:47 ` Andy Shevchenko
2021-02-03 16:50 ` [PATCH v4 4/4] selftests: lib: Add wrapper script for test_scanf Richard Fitzgerald
2021-02-03 19:48 ` [PATCH v4 1/4] lib: vsprintf: scanf: Negative number must have field width > 1 Andy Shevchenko
3 siblings, 1 reply; 15+ messages in thread
From: Richard Fitzgerald @ 2021-02-03 16:50 UTC (permalink / raw)
To: pmladek, rostedt, sergey.senozhatsky, andriy.shevchenko, linux, shuah
Cc: linux-kernel, linux-kselftest, patches, Richard Fitzgerald
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.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
MAINTAINERS | 1 +
lib/Kconfig.debug | 3 +
lib/Makefile | 1 +
lib/test_scanf.c | 752 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 757 insertions(+)
create mode 100644 lib/test_scanf.c
diff --git a/MAINTAINERS b/MAINTAINERS
index d3e847f7f3dc..d29257f9bb18 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19090,6 +19090,7 @@ S: Maintained
T: git git://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git
F: Documentation/core-api/printk-formats.rst
F: lib/test_printf.c
+F: lib/test_scanf.c
F: lib/vsprintf.c
VT1211 HARDWARE MONITOR DRIVER
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 7937265ef879..a799414d48a5 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2077,6 +2077,9 @@ config TEST_KSTRTOX
config TEST_PRINTF
tristate "Test printf() family of functions at runtime"
+config TEST_SCANF
+ tristate "Test scanf() family of functions at runtime"
+
config TEST_BITMAP
tristate "Test bitmap_*() family of functions at runtime"
help
diff --git a/lib/Makefile b/lib/Makefile
index afeff05fa8c5..e42c929eba2e 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
obj-$(CONFIG_TEST_PRINTF) += test_printf.o
+obj-$(CONFIG_TEST_SCANF) += test_scanf.o
obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
obj-$(CONFIG_TEST_STRSCPY) += test_strscpy.o
obj-$(CONFIG_TEST_UUID) += test_uuid.o
diff --git a/lib/test_scanf.c b/lib/test_scanf.c
new file mode 100644
index 000000000000..d83de378ff59
--- /dev/null
+++ b/lib/test_scanf.c
@@ -0,0 +1,752 @@
+// 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 _check_numbers_template(arg_fmt, expect, str, fmt, n_args, ap) \
+do { \
+ pr_debug("\"%s\", \"%s\" ->\n", str, fmt); \
+ for (; n_args > 0; n_args--, expect++) { \
+ typeof(*expect) got = *va_arg(ap, typeof(expect)); \
+ pr_debug("\t" arg_fmt "\n", got); \
+ if (got != *expect) { \
+ pr_warn("vsscanf(\"%s\", \"%s\", ...) expected " arg_fmt " got " arg_fmt "\n", \
+ str, fmt, *expect, got); \
+ return 1; \
+ } \
+ } \
+ return 0; \
+} while (0)
+
+static int __init check_ull(const void *check_data, const char *string,
+ const char *fmt, int n_args, va_list ap)
+{
+ const unsigned long long *pval = check_data;
+
+ _check_numbers_template("%llu", pval, string, fmt, n_args, ap);
+}
+
+static int __init check_ll(const void *check_data, const char *string,
+ const char *fmt, int n_args, va_list ap)
+{
+ const long long *pval = check_data;
+
+ _check_numbers_template("%lld", pval, string, fmt, n_args, ap);
+}
+
+static int __init check_ulong(const void *check_data, const char *string,
+ const char *fmt, int n_args, va_list ap)
+{
+ const unsigned long *pval = check_data;
+
+ _check_numbers_template("%lu", pval, string, fmt, n_args, ap);
+}
+
+static int __init check_long(const void *check_data, const char *string,
+ const char *fmt, int n_args, va_list ap)
+{
+ const long *pval = check_data;
+
+ _check_numbers_template("%ld", pval, string, fmt, n_args, ap);
+}
+
+static int __init check_uint(const void *check_data, const char *string,
+ const char *fmt, int n_args, va_list ap)
+{
+ const unsigned int *pval = check_data;
+
+ _check_numbers_template("%u", pval, string, fmt, n_args, ap);
+}
+
+static int __init check_int(const void *check_data, const char *string,
+ const char *fmt, int n_args, va_list ap)
+{
+ const int *pval = check_data;
+
+ _check_numbers_template("%d", pval, string, fmt, n_args, ap);
+}
+
+static int __init check_ushort(const void *check_data, const char *string,
+ const char *fmt, int n_args, va_list ap)
+{
+ const unsigned short *pval = check_data;
+
+ _check_numbers_template("%hu", pval, string, fmt, n_args, ap);
+}
+
+static int __init check_short(const void *check_data, const char *string,
+ const char *fmt, int n_args, va_list ap)
+{
+ const short *pval = check_data;
+
+ _check_numbers_template("%hd", pval, string, fmt, n_args, ap);
+}
+
+static int __init check_uchar(const void *check_data, const char *string,
+ const char *fmt, int n_args, va_list ap)
+{
+ const unsigned char *pval = check_data;
+
+ _check_numbers_template("%hhu", pval, string, fmt, n_args, ap);
+}
+
+static int __init check_char(const void *check_data, const char *string,
+ const char *fmt, int n_args, va_list ap)
+{
+ const signed char *pval = check_data;
+
+ _check_numbers_template("%hhd", pval, string, fmt, n_args, ap);
+}
+
+/* Selection of interesting numbers to test, copied from test-kstrtox.c */
+static const unsigned long long numbers[] __initconst = {
+ 0x0ULL,
+ 0x1ULL,
+ 0x7fULL,
+ 0x80ULL,
+ 0x81ULL,
+ 0xffULL,
+ 0x100ULL,
+ 0x101ULL,
+ 0x7fffULL,
+ 0x8000ULL,
+ 0x8001ULL,
+ 0xffffULL,
+ 0x10000ULL,
+ 0x10001ULL,
+ 0x7fffffffULL,
+ 0x80000000ULL,
+ 0x80000001ULL,
+ 0xffffffffULL,
+ 0x100000000ULL,
+ 0x100000001ULL,
+ 0x7fffffffffffffffULL,
+ 0x8000000000000000ULL,
+ 0x8000000000000001ULL,
+ 0xfffffffffffffffeULL,
+ 0xffffffffffffffffULL,
+};
+
+#define value_representable_in_type(T, val) \
+(is_signed_type(T) \
+ ? ((long long)(val) >= type_min(T)) && ((long long)(val) <= type_max(T)) \
+ : ((unsigned long long)(val) >= type_min(T)) && \
+ ((unsigned long long)(val) <= type_max(T)))
+
+#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 */ \
+ \
+ snprintf(test_buffer, BUF_SIZE, gen_fmt, expect_val); \
+ _test(fn, &expect_val, test_buffer, "%" scan_fmt, 1, &result); \
+} while (0)
+
+#define simple_numbers_loop(T, gen_fmt, scan_fmt, fn) \
+do { \
+ int i; \
+ \
+ for (i = 0; i < ARRAY_SIZE(numbers); i++) { \
+ if (!value_representable_in_type(T, numbers[i])) \
+ continue; \
+ \
+ test_one_number(T, gen_fmt, scan_fmt, numbers[i], fn); \
+ \
+ if (is_signed_type(T)) \
+ test_one_number(T, gen_fmt, scan_fmt, \
+ -numbers[i], fn); \
+ } \
+} while (0)
+
+static void __init numbers_simple(void)
+{
+ simple_numbers_loop(unsigned long long, "%llu", "llu", check_ull);
+ simple_numbers_loop(long long, "%lld", "lld", check_ll);
+ simple_numbers_loop(long long, "%lld", "lli", check_ll);
+ simple_numbers_loop(unsigned long long, "%llx", "llx", check_ull);
+ simple_numbers_loop(long long, "%llx", "llx", check_ll);
+ simple_numbers_loop(long long, "0x%llx", "lli", check_ll);
+ simple_numbers_loop(unsigned long long, "0x%llx", "llx", check_ull);
+ simple_numbers_loop(long long, "0x%llx", "llx", check_ll);
+
+ simple_numbers_loop(unsigned long, "%lu", "lu", check_ulong);
+ simple_numbers_loop(long, "%ld", "ld", check_long);
+ simple_numbers_loop(long, "%ld", "li", check_long);
+ simple_numbers_loop(unsigned long, "%lx", "lx", check_ulong);
+ simple_numbers_loop(long, "%lx", "lx", check_long);
+ simple_numbers_loop(long, "0x%lx", "li", check_long);
+ simple_numbers_loop(unsigned long, "0x%lx", "lx", check_ulong);
+ simple_numbers_loop(long, "0x%lx", "lx", check_long);
+
+ simple_numbers_loop(unsigned int, "%u", "u", check_uint);
+ simple_numbers_loop(int, "%d", "d", check_int);
+ simple_numbers_loop(int, "%d", "i", check_int);
+ simple_numbers_loop(unsigned int, "%x", "x", check_uint);
+ simple_numbers_loop(int, "%x", "x", check_int);
+ simple_numbers_loop(int, "0x%x", "i", check_int);
+ simple_numbers_loop(unsigned int, "0x%x", "x", check_uint);
+ simple_numbers_loop(int, "0x%x", "x", check_int);
+
+ simple_numbers_loop(unsigned short, "%hu", "hu", check_ushort);
+ simple_numbers_loop(short, "%hd", "hd", check_short);
+ simple_numbers_loop(short, "%hd", "hi", check_short);
+ simple_numbers_loop(unsigned short, "%hx", "hx", check_ushort);
+ simple_numbers_loop(short, "%hx", "hx", check_short);
+ simple_numbers_loop(short, "0x%hx", "hi", check_short);
+ simple_numbers_loop(unsigned short, "0x%hx", "hx", check_ushort);
+ simple_numbers_loop(short, "0x%hx", "hx", check_short);
+
+ simple_numbers_loop(unsigned char, "%hhu", "hhu", check_uchar);
+ simple_numbers_loop(signed char, "%hhd", "hhd", check_char);
+ simple_numbers_loop(signed char, "%hhd", "hhi", check_char);
+ simple_numbers_loop(unsigned char, "%hhx", "hhx", check_uchar);
+ simple_numbers_loop(signed char, "%hhx", "hhx", check_char);
+ simple_numbers_loop(signed char, "0x%hhx", "hhi", check_char);
+ simple_numbers_loop(unsigned char, "0x%hhx", "hhx", check_uchar);
+ simple_numbers_loop(signed char, "0x%hhx", "hhx", check_char);
+}
+
+/*
+ * This gives a better variety of number "lengths" in a small sample than
+ * the raw prandom*() functions (Not mathematically rigorous!!).
+ * Variabilty of length and value is more important than perfect randomness.
+ */
+static u32 __init next_test_random(u32 max_bits)
+{
+ u32 n_bits = hweight32(prandom_u32_state(&rnd_state)) % (max_bits + 1);
+
+ return prandom_u32_state(&rnd_state) & (UINT_MAX >> (32 - n_bits));
+}
+
+static unsigned long long __init next_test_random_ull(void)
+{
+ u32 rand1 = prandom_u32_state(&rnd_state);
+ u32 n_bits = (hweight32(rand1) * 3) % 64;
+ u64 val = (u64)prandom_u32_state(&rnd_state) * rand1;
+
+ return val & (ULLONG_MAX >> (64 - n_bits));
+}
+
+#define random_for_type(T) \
+ ((T)(sizeof(T) <= sizeof(u32) \
+ ? next_test_random(BITS_PER_TYPE(T)) \
+ : next_test_random_ull()))
+
+/*
+ * Define a pattern of negative and positive numbers to ensure we get
+ * some of both within the small number of samples in a test string.
+ */
+#define NEGATIVES_PATTERN 0x3246 /* 00110010 01000110 */
+
+#define fill_random_array(arr) \
+do { \
+ unsigned int neg_pattern = NEGATIVES_PATTERN; \
+ int i; \
+ \
+ for (i = 0; i < ARRAY_SIZE(arr); i++, neg_pattern >>= 1) { \
+ (arr)[i] = random_for_type(typeof((arr)[0])); \
+ if (is_signed_type(typeof((arr)[0])) && (neg_pattern & 1)) \
+ (arr)[i] = -(arr)[i]; \
+ } \
+} while (0)
+
+/*
+ * Convenience wrapper around snprintf() to append at buf_pos in buf,
+ * updating buf_pos and returning the number of characters appended.
+ * On error buf_pos is not changed and return value is 0.
+ */
+static int __init __printf(4, 5)
+append_fmt(char *buf, int *buf_pos, int buf_len, const char *val_fmt, ...)
+{
+ va_list ap;
+ int field_len;
+
+ va_start(ap, val_fmt);
+ field_len = vsnprintf(buf + *buf_pos, buf_len - *buf_pos, val_fmt, ap);
+ va_end(ap);
+
+ if (field_len < 0)
+ field_len = 0;
+
+ *buf_pos += field_len;
+
+ return field_len;
+}
+
+/*
+ * Convenience function to append the field delimiter string
+ * to both the value string and format string buffers.
+ */
+static void __init append_delim(char *str_buf, int *str_buf_pos, int str_buf_len,
+ char *fmt_buf, int *fmt_buf_pos, int fmt_buf_len,
+ const char *delim_str)
+{
+ append_fmt(str_buf, str_buf_pos, str_buf_len, delim_str);
+ append_fmt(fmt_buf, fmt_buf_pos, fmt_buf_len, delim_str);
+}
+
+#define test_array_8(fn, check_data, string, fmt, arr) \
+do { \
+ BUILD_BUG_ON(ARRAY_SIZE(arr) != 8); \
+ _test(fn, check_data, string, fmt, 8, \
+ &(arr)[0], &(arr)[1], &(arr)[2], &(arr)[3], \
+ &(arr)[4], &(arr)[5], &(arr)[6], &(arr)[7]); \
+} while (0)
+
+#define numbers_list_8(T, gen_fmt, field_sep, scan_fmt, fn) \
+do { \
+ int i, pos = 0, fmt_pos = 0; \
+ T expect[8], result[8]; \
+ \
+ fill_random_array(expect); \
+ \
+ for (i = 0; i < ARRAY_SIZE(expect); i++) { \
+ if (i != 0) \
+ append_delim(test_buffer, &pos, BUF_SIZE, \
+ fmt_buffer, &fmt_pos, BUF_SIZE, \
+ field_sep); \
+ \
+ append_fmt(test_buffer, &pos, BUF_SIZE, gen_fmt, expect[i]); \
+ append_fmt(fmt_buffer, &fmt_pos, BUF_SIZE, "%%%s", scan_fmt); \
+ } \
+ \
+ test_array_8(fn, expect, test_buffer, fmt_buffer, result); \
+} while (0)
+
+#define numbers_list_fix_width(T, gen_fmt, field_sep, width, scan_fmt, fn) \
+do { \
+ char full_fmt[16]; \
+ \
+ snprintf(full_fmt, sizeof(full_fmt), "%u%s", width, scan_fmt); \
+ numbers_list_8(T, gen_fmt, field_sep, full_fmt, fn); \
+} while (0)
+
+#define numbers_list_val_width(T, gen_fmt, field_sep, scan_fmt, fn) \
+do { \
+ int i, val_len, pos = 0, fmt_pos = 0; \
+ T expect[8], result[8]; \
+ \
+ fill_random_array(expect); \
+ \
+ for (i = 0; i < ARRAY_SIZE(expect); i++) { \
+ if (i != 0) \
+ append_delim(test_buffer, &pos, BUF_SIZE, \
+ fmt_buffer, &fmt_pos, BUF_SIZE, field_sep);\
+ \
+ val_len = append_fmt(test_buffer, &pos, BUF_SIZE, gen_fmt, \
+ expect[i]); \
+ append_fmt(fmt_buffer, &fmt_pos, BUF_SIZE, \
+ "%%%u%s", val_len, scan_fmt); \
+ } \
+ \
+ test_array_8(fn, expect, test_buffer, fmt_buffer, result); \
+} while (0)
+
+static void __init numbers_list(const char *delim)
+{
+ numbers_list_8(unsigned long long, "%llu", delim, "llu", check_ull);
+ numbers_list_8(long long, "%lld", delim, "lld", check_ll);
+ numbers_list_8(long long, "%lld", delim, "lli", check_ll);
+ numbers_list_8(unsigned long long, "%llx", delim, "llx", check_ull);
+ numbers_list_8(unsigned long long, "0x%llx", delim, "llx", check_ull);
+ numbers_list_8(long long, "0x%llx", delim, "lli", check_ll);
+
+ numbers_list_8(unsigned long, "%lu", delim, "lu", check_ulong);
+ numbers_list_8(long, "%ld", delim, "ld", check_long);
+ numbers_list_8(long, "%ld", delim, "li", check_long);
+ numbers_list_8(unsigned long, "%lx", delim, "lx", check_ulong);
+ numbers_list_8(unsigned long, "0x%lx", delim, "lx", check_ulong);
+ numbers_list_8(long, "0x%lx", delim, "li", check_long);
+
+ numbers_list_8(unsigned int, "%u", delim, "u", check_uint);
+ numbers_list_8(int, "%d", delim, "d", check_int);
+ numbers_list_8(int, "%d", delim, "i", check_int);
+ numbers_list_8(unsigned int, "%x", delim, "x", check_uint);
+ numbers_list_8(unsigned int, "0x%x", delim, "x", check_uint);
+ numbers_list_8(int, "0x%x", delim, "i", check_int);
+
+ numbers_list_8(unsigned short, "%hu", delim, "hu", check_ushort);
+ numbers_list_8(short, "%hd", delim, "hd", check_short);
+ numbers_list_8(short, "%hd", delim, "hi", check_short);
+ numbers_list_8(unsigned short, "%hx", delim, "hx", check_ushort);
+ numbers_list_8(unsigned short, "0x%hx", delim, "hx", check_ushort);
+ numbers_list_8(short, "0x%hx", delim, "hi", check_short);
+
+ numbers_list_8(unsigned char, "%hhu", delim, "hhu", check_uchar);
+ numbers_list_8(signed char, "%hhd", delim, "hhd", check_char);
+ numbers_list_8(signed char, "%hhd", delim, "hhi", check_char);
+ numbers_list_8(unsigned char, "%hhx", delim, "hhx", check_uchar);
+ numbers_list_8(unsigned char, "0x%hhx", delim, "hhx", check_uchar);
+ numbers_list_8(signed char, "0x%hhx", delim, "hhi", check_char);
+}
+
+/*
+ * List of numbers separated by delim. Each field width specifier is the
+ * maximum possible digits for the given type and base.
+ */
+static void __init numbers_list_field_width_typemax(const char *delim)
+{
+ numbers_list_fix_width(unsigned long long, "%llu", delim, 20, "llu", check_ull);
+ numbers_list_fix_width(long long, "%lld", delim, 20, "lld", check_ll);
+ numbers_list_fix_width(long long, "%lld", delim, 20, "lli", check_ll);
+ numbers_list_fix_width(unsigned long long, "%llx", delim, 16, "llx", check_ull);
+ numbers_list_fix_width(unsigned long long, "0x%llx", delim, 18, "llx", check_ull);
+ numbers_list_fix_width(long long, "0x%llx", delim, 18, "lli", check_ll);
+
+#if BITS_PER_LONG == 64
+ numbers_list_fix_width(unsigned long, "%lu", delim, 20, "lu", check_ulong);
+ numbers_list_fix_width(long, "%ld", delim, 20, "ld", check_long);
+ numbers_list_fix_width(long, "%ld", delim, 20, "li", check_long);
+ numbers_list_fix_width(unsigned long, "%lx", delim, 16, "lx", check_ulong);
+ numbers_list_fix_width(unsigned long, "0x%lx", delim, 18, "lx", check_ulong);
+ numbers_list_fix_width(long, "0x%lx", delim, 18, "li", check_long);
+#else
+ numbers_list_fix_width(unsigned long, "%lu", delim, 10, "lu", check_ulong);
+ numbers_list_fix_width(long, "%ld", delim, 11, "ld", check_long);
+ numbers_list_fix_width(long, "%ld", delim, 11, "li", check_long);
+ numbers_list_fix_width(unsigned long, "%lx", delim, 8, "lx", check_ulong);
+ numbers_list_fix_width(unsigned long, "0x%lx", delim, 10, "lx", check_ulong);
+ numbers_list_fix_width(long, "0x%lx", delim, 10, "li", check_long);
+#endif
+
+ numbers_list_fix_width(unsigned int, "%u", delim, 10, "u", check_uint);
+ numbers_list_fix_width(int, "%d", delim, 11, "d", check_int);
+ numbers_list_fix_width(int, "%d", delim, 11, "i", check_int);
+ numbers_list_fix_width(unsigned int, "%x", delim, 8, "x", check_uint);
+ numbers_list_fix_width(unsigned int, "0x%x", delim, 10, "x", check_uint);
+ numbers_list_fix_width(int, "0x%x", delim, 10, "i", check_int);
+
+ numbers_list_fix_width(unsigned short, "%hu", delim, 5, "hu", check_ushort);
+ numbers_list_fix_width(short, "%hd", delim, 6, "hd", check_short);
+ numbers_list_fix_width(short, "%hd", delim, 6, "hi", check_short);
+ numbers_list_fix_width(unsigned short, "%hx", delim, 4, "hx", check_ushort);
+ numbers_list_fix_width(unsigned short, "0x%hx", delim, 6, "hx", check_ushort);
+ numbers_list_fix_width(short, "0x%hx", delim, 6, "hi", check_short);
+
+ numbers_list_fix_width(unsigned char, "%hhu", delim, 3, "hhu", check_uchar);
+ numbers_list_fix_width(signed char, "%hhd", delim, 4, "hhd", check_char);
+ numbers_list_fix_width(signed char, "%hhd", delim, 4, "hhi", check_char);
+ numbers_list_fix_width(unsigned char, "%hhx", delim, 2, "hhx", check_uchar);
+ numbers_list_fix_width(unsigned char, "0x%hhx", delim, 4, "hhx", check_uchar);
+ numbers_list_fix_width(signed char, "0x%hhx", delim, 4, "hhi", check_char);
+}
+
+/*
+ * List of numbers separated by delim. Each field width specifier is the
+ * exact length of the corresponding value digits in the string being scanned.
+ */
+static void __init numbers_list_field_width_val_width(const char *delim)
+{
+ numbers_list_val_width(unsigned long long, "%llu", delim, "llu", check_ull);
+ numbers_list_val_width(long long, "%lld", delim, "lld", check_ll);
+ numbers_list_val_width(long long, "%lld", delim, "lli", check_ll);
+ numbers_list_val_width(unsigned long long, "%llx", delim, "llx", check_ull);
+ numbers_list_val_width(unsigned long long, "0x%llx", delim, "llx", check_ull);
+ numbers_list_val_width(long long, "0x%llx", delim, "lli", check_ll);
+
+ numbers_list_val_width(unsigned long, "%lu", delim, "lu", check_ulong);
+ numbers_list_val_width(long, "%ld", delim, "ld", check_long);
+ numbers_list_val_width(long, "%ld", delim, "li", check_long);
+ numbers_list_val_width(unsigned long, "%lx", delim, "lx", check_ulong);
+ numbers_list_val_width(unsigned long, "0x%lx", delim, "lx", check_ulong);
+ numbers_list_val_width(long, "0x%lx", delim, "li", check_long);
+
+ numbers_list_val_width(unsigned int, "%u", delim, "u", check_uint);
+ numbers_list_val_width(int, "%d", delim, "d", check_int);
+ numbers_list_val_width(int, "%d", delim, "i", check_int);
+ numbers_list_val_width(unsigned int, "%x", delim, "x", check_uint);
+ numbers_list_val_width(unsigned int, "0x%x", delim, "x", check_uint);
+ numbers_list_val_width(int, "0x%x", delim, "i", check_int);
+
+ numbers_list_val_width(unsigned short, "%hu", delim, "hu", check_ushort);
+ numbers_list_val_width(short, "%hd", delim, "hd", check_short);
+ numbers_list_val_width(short, "%hd", delim, "hi", check_short);
+ numbers_list_val_width(unsigned short, "%hx", delim, "hx", check_ushort);
+ numbers_list_val_width(unsigned short, "0x%hx", delim, "hx", check_ushort);
+ numbers_list_val_width(short, "0x%hx", delim, "hi", check_short);
+
+ numbers_list_val_width(unsigned char, "%hhu", delim, "hhu", check_uchar);
+ numbers_list_val_width(signed char, "%hhd", delim, "hhd", check_char);
+ numbers_list_val_width(signed char, "%hhd", delim, "hhi", check_char);
+ numbers_list_val_width(unsigned char, "%hhx", delim, "hhx", check_uchar);
+ numbers_list_val_width(unsigned char, "0x%hhx", delim, "hhx", check_uchar);
+ numbers_list_val_width(signed char, "0x%hhx", delim, "hhi", check_char);
+}
+
+/*
+ * Slice a continuous string of digits without field delimiters, containing
+ * numbers of varying length, using the field width to extract each group
+ * of digits. For example the hex values c0,3,bf01,303 would have a
+ * string representation of "c03bf01303" and extracted with "%2x%1x%4x%3x".
+ */
+static void __init numbers_slice(void)
+{
+ numbers_list_field_width_val_width("");
+}
+
+#define test_number_prefix(T, str, scan_fmt, expect0, expect1, n_args, fn) \
+do { \
+ const T expect[2] = { expect0, expect1 }; \
+ T result[2] = {~expect[0], ~expect[1]}; \
+ \
+ _test(fn, &expect, str, scan_fmt, n_args, &result[0], &result[1]); \
+} while (0)
+
+/*
+ * Number prefix is >= field width.
+ * Expected behaviour is derived from testing userland sscanf.
+ */
+static void __init numbers_prefix_overflow(void)
+{
+ /*
+ * Negative decimal with a field of width 1, should quit scanning
+ * and return 0.
+ */
+ test_number_prefix(long long, "-1 1", "%1lld %lld", 0, 0, 0, check_ll);
+ test_number_prefix(long, "-1 1", "%1ld %ld", 0, 0, 0, check_long);
+ test_number_prefix(int, "-1 1", "%1d %d", 0, 0, 0, check_int);
+ test_number_prefix(short, "-1 1", "%1hd %hd", 0, 0, 0, check_short);
+ test_number_prefix(signed char, "-1 1", "%1hhd %hhd", 0, 0, 0, check_char);
+
+ test_number_prefix(long long, "-1 1", "%1lli %lli", 0, 0, 0, check_ll);
+ test_number_prefix(long, "-1 1", "%1li %li", 0, 0, 0, check_long);
+ test_number_prefix(int, "-1 1", "%1i %i", 0, 0, 0, check_int);
+ test_number_prefix(short, "-1 1", "%1hi %hi", 0, 0, 0, check_short);
+ test_number_prefix(signed char, "-1 1", "%1hhi %hhi", 0, 0, 0, check_char);
+
+ /*
+ * 0x prefix in a field of width 1: 0 is a valid digit so should
+ * convert. Next field scan starts at the 'x' which isn't a digit so
+ * scan quits with one field converted.
+ */
+ test_number_prefix(unsigned long long, "0xA7", "%1llx%llx", 0, 0, 1, check_ull);
+ test_number_prefix(unsigned long, "0xA7", "%1lx%lx", 0, 0, 1, check_ulong);
+ test_number_prefix(unsigned int, "0xA7", "%1x%x", 0, 0, 1, check_uint);
+ test_number_prefix(unsigned short, "0xA7", "%1hx%hx", 0, 0, 1, check_ushort);
+ test_number_prefix(unsigned char, "0xA7", "%1hhx%hhx", 0, 0, 1, check_uchar);
+ test_number_prefix(long long, "0xA7", "%1lli%llx", 0, 0, 1, check_ll);
+ test_number_prefix(long, "0xA7", "%1li%lx", 0, 0, 1, check_long);
+ test_number_prefix(int, "0xA7", "%1i%x", 0, 0, 1, check_int);
+ test_number_prefix(short, "0xA7", "%1hi%hx", 0, 0, 1, check_short);
+ test_number_prefix(char, "0xA7", "%1hhi%hhx", 0, 0, 1, check_char);
+
+ /*
+ * 0x prefix in a field of width 2 using %x conversion: first field
+ * converts to 0. Next field scan starts at the character after "0x".
+ * Both fields will convert.
+ */
+ test_number_prefix(unsigned long long, "0xA7", "%2llx%llx", 0, 0xa7, 2, check_ull);
+ test_number_prefix(unsigned long, "0xA7", "%2lx%lx", 0, 0xa7, 2, check_ulong);
+ test_number_prefix(unsigned int, "0xA7", "%2x%x", 0, 0xa7, 2, check_uint);
+ test_number_prefix(unsigned short, "0xA7", "%2hx%hx", 0, 0xa7, 2, check_ushort);
+ test_number_prefix(unsigned char, "0xA7", "%2hhx%hhx", 0, 0xa7, 2, check_uchar);
+
+ /*
+ * 0x prefix in a field of width 2 using %i conversion: first field
+ * converts to 0. Next field scan starts at the character after "0x",
+ * which will convert if can be intepreted as decimal but will fail
+ * if it contains any hex digits (since no 0x prefix).
+ */
+ test_number_prefix(long long, "0x67", "%2lli%lli", 0, 67, 2, check_ll);
+ test_number_prefix(long, "0x67", "%2li%li", 0, 67, 2, check_long);
+ test_number_prefix(int, "0x67", "%2i%i", 0, 67, 2, check_int);
+ test_number_prefix(short, "0x67", "%2hi%hi", 0, 67, 2, check_short);
+ test_number_prefix(char, "0x67", "%2hhi%hhi", 0, 67, 2, check_char);
+
+ test_number_prefix(long long, "0xA7", "%2lli%lli", 0, 0, 1, check_ll);
+ test_number_prefix(long, "0xA7", "%2li%li", 0, 0, 1, check_long);
+ test_number_prefix(int, "0xA7", "%2i%i", 0, 0, 1, check_int);
+ test_number_prefix(short, "0xA7", "%2hi%hi", 0, 0, 1, check_short);
+ test_number_prefix(char, "0xA7", "%2hhi%hhi", 0, 0, 1, check_char);
+}
+
+#define _test_simple_strtoxx(T, fn, gen_fmt, expect, base) \
+do { \
+ T got; \
+ char *endp; \
+ int len; \
+ bool fail = false; \
+ \
+ total_tests++; \
+ len = snprintf(test_buffer, BUF_SIZE, gen_fmt, expect); \
+ got = (fn)(test_buffer, &endp, base); \
+ pr_debug(#fn "(\"%s\", %d) -> " gen_fmt "\n", test_buffer, base, got); \
+ if (got != (expect)) { \
+ fail = true; \
+ pr_warn(#fn "(\"%s\", %d): got " gen_fmt " expected " gen_fmt "\n", \
+ test_buffer, base, got, expect); \
+ } else if (endp != test_buffer + len) { \
+ fail = true; \
+ pr_warn(#fn "(\"%s\", %d) startp=0x%px got endp=0x%px expected 0x%px\n", \
+ test_buffer, base, test_buffer, \
+ test_buffer + len, endp); \
+ } \
+ \
+ if (fail) \
+ failed_tests++; \
+} while (0)
+
+#define test_simple_strtoxx(T, fn, gen_fmt, base) \
+do { \
+ int i; \
+ \
+ for (i = 0; i < ARRAY_SIZE(numbers); i++) { \
+ _test_simple_strtoxx(T, fn, gen_fmt, (T)numbers[i], base); \
+ \
+ if (is_signed_type(T)) \
+ _test_simple_strtoxx(T, fn, gen_fmt, \
+ -(T)numbers[i], base); \
+ } \
+} while (0)
+
+static void __init test_simple_strtoull(void)
+{
+ test_simple_strtoxx(unsigned long long, simple_strtoull, "%llu", 10);
+ test_simple_strtoxx(unsigned long long, simple_strtoull, "%llu", 0);
+ test_simple_strtoxx(unsigned long long, simple_strtoull, "%llx", 16);
+ test_simple_strtoxx(unsigned long long, simple_strtoull, "0x%llx", 16);
+ test_simple_strtoxx(unsigned long long, simple_strtoull, "0x%llx", 0);
+}
+
+static void __init test_simple_strtoll(void)
+{
+ test_simple_strtoxx(long long, simple_strtoll, "%lld", 10);
+ test_simple_strtoxx(long long, simple_strtoll, "%lld", 0);
+ test_simple_strtoxx(long long, simple_strtoll, "%llx", 16);
+ test_simple_strtoxx(long long, simple_strtoll, "0x%llx", 16);
+ test_simple_strtoxx(long long, simple_strtoll, "0x%llx", 0);
+}
+
+static void __init test_simple_strtoul(void)
+{
+ test_simple_strtoxx(unsigned long, simple_strtoul, "%lu", 10);
+ test_simple_strtoxx(unsigned long, simple_strtoul, "%lu", 0);
+ test_simple_strtoxx(unsigned long, simple_strtoul, "%lx", 16);
+ test_simple_strtoxx(unsigned long, simple_strtoul, "0x%lx", 16);
+ test_simple_strtoxx(unsigned long, simple_strtoul, "0x%lx", 0);
+}
+
+static void __init test_simple_strtol(void)
+{
+ test_simple_strtoxx(long, simple_strtol, "%ld", 10);
+ test_simple_strtoxx(long, simple_strtol, "%ld", 0);
+ test_simple_strtoxx(long, simple_strtol, "%lx", 16);
+ test_simple_strtoxx(long, simple_strtol, "0x%lx", 16);
+ test_simple_strtoxx(long, simple_strtol, "0x%lx", 0);
+}
+
+/* Selection of common delimiters/separators between numbers in a string. */
+static const char * const number_delimiters[] __initconst = {
+ " ", ":", ",", "-", "/",
+};
+
+static void __init test_numbers(void)
+{
+ int i;
+
+ /* String containing only one number. */
+ numbers_simple();
+
+ /* String with multiple numbers separated by delimiter. */
+ for (i = 0; i < ARRAY_SIZE(number_delimiters); i++) {
+ numbers_list(number_delimiters[i]);
+
+ /* Field width may be longer than actual field digits. */
+ numbers_list_field_width_typemax(number_delimiters[i]);
+
+ /* Each field width exactly length of actual field digits. */
+ numbers_list_field_width_val_width(number_delimiters[i]);
+ }
+
+ /* Slice continuous sequence of digits using field widths. */
+ numbers_slice();
+
+ numbers_prefix_overflow();
+}
+
+static void __init selftest(void)
+{
+ test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
+ if (!test_buffer)
+ return;
+
+ fmt_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
+ if (!fmt_buffer) {
+ kfree(test_buffer);
+ return;
+ }
+
+ prandom_seed_state(&rnd_state, 3141592653589793238ULL);
+
+ test_numbers();
+
+ test_simple_strtoull();
+ test_simple_strtoll();
+ test_simple_strtoul();
+ test_simple_strtol();
+
+ kfree(fmt_buffer);
+ kfree(test_buffer);
+}
+
+KSTM_MODULE_LOADERS(test_scanf);
+MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.cirrus.com>");
+MODULE_LICENSE("GPL v2");
--
2.20.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 4/4] selftests: lib: Add wrapper script for test_scanf
2021-02-03 16:50 [PATCH v4 1/4] lib: vsprintf: scanf: Negative number must have field width > 1 Richard Fitzgerald
2021-02-03 16:50 ` [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf Richard Fitzgerald
2021-02-03 16:50 ` [PATCH v4 3/4] lib: test_scanf: Add tests for sscanf number conversion Richard Fitzgerald
@ 2021-02-03 16:50 ` Richard Fitzgerald
2021-02-03 19:48 ` Andy Shevchenko
2021-02-03 19:48 ` [PATCH v4 1/4] lib: vsprintf: scanf: Negative number must have field width > 1 Andy Shevchenko
3 siblings, 1 reply; 15+ messages in thread
From: Richard Fitzgerald @ 2021-02-03 16:50 UTC (permalink / raw)
To: pmladek, rostedt, sergey.senozhatsky, andriy.shevchenko, linux, shuah
Cc: linux-kernel, linux-kselftest, patches, Richard Fitzgerald
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>
---
tools/testing/selftests/lib/Makefile | 2 +-
tools/testing/selftests/lib/config | 1 +
tools/testing/selftests/lib/scanf.sh | 4 ++++
3 files changed, 6 insertions(+), 1 deletion(-)
create mode 100755 tools/testing/selftests/lib/scanf.sh
diff --git a/tools/testing/selftests/lib/Makefile b/tools/testing/selftests/lib/Makefile
index a105f094676e..ee71fc99d5b5 100644
--- a/tools/testing/selftests/lib/Makefile
+++ b/tools/testing/selftests/lib/Makefile
@@ -4,6 +4,6 @@
# No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
all:
-TEST_PROGS := printf.sh bitmap.sh prime_numbers.sh strscpy.sh
+TEST_PROGS := printf.sh bitmap.sh prime_numbers.sh scanf.sh strscpy.sh
include ../lib.mk
diff --git a/tools/testing/selftests/lib/config b/tools/testing/selftests/lib/config
index b80ee3f6e265..776c8c42e78d 100644
--- a/tools/testing/selftests/lib/config
+++ b/tools/testing/selftests/lib/config
@@ -1,4 +1,5 @@
CONFIG_TEST_PRINTF=m
+CONFIG_TEST_SCANTF=m
CONFIG_TEST_BITMAP=m
CONFIG_PRIME_NUMBERS=m
CONFIG_TEST_STRSCPY=m
diff --git a/tools/testing/selftests/lib/scanf.sh b/tools/testing/selftests/lib/scanf.sh
new file mode 100755
index 000000000000..b59b8ba561c3
--- /dev/null
+++ b/tools/testing/selftests/lib/scanf.sh
@@ -0,0 +1,4 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Tests the scanf infrastructure using test_scanf kernel module.
+$(dirname $0)/../kselftest/module.sh "scanf" test_scanf
--
2.20.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf
2021-02-03 16:50 ` [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf Richard Fitzgerald
@ 2021-02-03 19:45 ` Andy Shevchenko
2021-02-04 16:35 ` Petr Mladek
0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2021-02-03 19:45 UTC (permalink / raw)
To: Richard Fitzgerald
Cc: pmladek, rostedt, sergey.senozhatsky, linux, shuah, linux-kernel,
linux-kselftest, patches
On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:
> The existing code attempted to handle numbers by doing a strto[u]l(),
> ignoring the field width, and then repeatedly dividing to extract the
> field out of the full 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 dividing can recover the correct value.
>
> This patch fixes vsscanf to obey number field widths when parsing
vsscanf()
> 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.
>
> If a number starts with a radix prefix, the field width must be long
> enough for at last one digit after the prefix. If not, it will be handled
> like this:
>
> sscanf("0x4", "%1i", &i): i=0, scanning continues with the 'x'
> sscanf("0x4", "%2i", &i): i=0, scanning continues with the '4'
>
> This is consistent with the observed behaviour of userland sscanf.
>
> Note that this patch does NOT fix the problem of a single field value
> overflowing the target type. So for example:
>
> sscanf("123456789abcdef", "%x", &i);
>
> Will not produce the correct result because the value obviously overflows
> INT_MAX. But sscanf will report a successful conversion.
...
> + for (; max_chars > 0; max_chars--) {
Less fragile is to write
while (max_chars--)
This allows max_char to be an unsigned type.
Moreover...
> + return _parse_integer_limit(s, base, p, INT_MAX);
You have inconsistency with INT_MAX vs, size_t above.
...
> +unsigned int _parse_integer_limit(const char *s, unsigned int base,
> + unsigned long long *res, size_t max_chars);
Also, can you leave res on previous line, so it will be easier to see what's
the difference with the original one?
> unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long *res);
...
> - 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;
> + }
It's redundant if I'm not mistaken.
> + cp = _parse_integer_fixup_radix(startp, &base);
> + if ((cp - startp) >= max_chars) {
> + cp = startp + max_chars;
> + goto out;
> + }
This will be exactly the same, no?
Moreover you will have while (max_chars--) in the _limit() variant which is
also a no-op.
...
> -
Unrelated change.
> +out:
> if (endp)
> *endp = (char *)cp;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/4] lib: test_scanf: Add tests for sscanf number conversion
2021-02-03 16:50 ` [PATCH v4 3/4] lib: test_scanf: Add tests for sscanf number conversion Richard Fitzgerald
@ 2021-02-03 19:47 ` Andy Shevchenko
0 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2021-02-03 19:47 UTC (permalink / raw)
To: Richard Fitzgerald
Cc: pmladek, rostedt, sergey.senozhatsky, linux, shuah, linux-kernel,
linux-kselftest, patches
On Wed, Feb 03, 2021 at 04:50:08PM +0000, 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.
Cool!
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> MAINTAINERS | 1 +
> lib/Kconfig.debug | 3 +
> lib/Makefile | 1 +
> lib/test_scanf.c | 752 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 757 insertions(+)
> create mode 100644 lib/test_scanf.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d3e847f7f3dc..d29257f9bb18 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19090,6 +19090,7 @@ S: Maintained
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/pmladek/printk.git
> F: Documentation/core-api/printk-formats.rst
> F: lib/test_printf.c
> +F: lib/test_scanf.c
> F: lib/vsprintf.c
>
> VT1211 HARDWARE MONITOR DRIVER
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 7937265ef879..a799414d48a5 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2077,6 +2077,9 @@ config TEST_KSTRTOX
> config TEST_PRINTF
> tristate "Test printf() family of functions at runtime"
>
> +config TEST_SCANF
> + tristate "Test scanf() family of functions at runtime"
> +
> config TEST_BITMAP
> tristate "Test bitmap_*() family of functions at runtime"
> help
> diff --git a/lib/Makefile b/lib/Makefile
> index afeff05fa8c5..e42c929eba2e 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> obj-$(CONFIG_TEST_PRINTF) += test_printf.o
> +obj-$(CONFIG_TEST_SCANF) += test_scanf.o
> obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
> obj-$(CONFIG_TEST_STRSCPY) += test_strscpy.o
> obj-$(CONFIG_TEST_UUID) += test_uuid.o
> diff --git a/lib/test_scanf.c b/lib/test_scanf.c
> new file mode 100644
> index 000000000000..d83de378ff59
> --- /dev/null
> +++ b/lib/test_scanf.c
> @@ -0,0 +1,752 @@
> +// 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 _check_numbers_template(arg_fmt, expect, str, fmt, n_args, ap) \
> +do { \
> + pr_debug("\"%s\", \"%s\" ->\n", str, fmt); \
> + for (; n_args > 0; n_args--, expect++) { \
> + typeof(*expect) got = *va_arg(ap, typeof(expect)); \
> + pr_debug("\t" arg_fmt "\n", got); \
> + if (got != *expect) { \
> + pr_warn("vsscanf(\"%s\", \"%s\", ...) expected " arg_fmt " got " arg_fmt "\n", \
> + str, fmt, *expect, got); \
> + return 1; \
> + } \
> + } \
> + return 0; \
> +} while (0)
> +
> +static int __init check_ull(const void *check_data, const char *string,
> + const char *fmt, int n_args, va_list ap)
> +{
> + const unsigned long long *pval = check_data;
> +
> + _check_numbers_template("%llu", pval, string, fmt, n_args, ap);
> +}
> +
> +static int __init check_ll(const void *check_data, const char *string,
> + const char *fmt, int n_args, va_list ap)
> +{
> + const long long *pval = check_data;
> +
> + _check_numbers_template("%lld", pval, string, fmt, n_args, ap);
> +}
> +
> +static int __init check_ulong(const void *check_data, const char *string,
> + const char *fmt, int n_args, va_list ap)
> +{
> + const unsigned long *pval = check_data;
> +
> + _check_numbers_template("%lu", pval, string, fmt, n_args, ap);
> +}
> +
> +static int __init check_long(const void *check_data, const char *string,
> + const char *fmt, int n_args, va_list ap)
> +{
> + const long *pval = check_data;
> +
> + _check_numbers_template("%ld", pval, string, fmt, n_args, ap);
> +}
> +
> +static int __init check_uint(const void *check_data, const char *string,
> + const char *fmt, int n_args, va_list ap)
> +{
> + const unsigned int *pval = check_data;
> +
> + _check_numbers_template("%u", pval, string, fmt, n_args, ap);
> +}
> +
> +static int __init check_int(const void *check_data, const char *string,
> + const char *fmt, int n_args, va_list ap)
> +{
> + const int *pval = check_data;
> +
> + _check_numbers_template("%d", pval, string, fmt, n_args, ap);
> +}
> +
> +static int __init check_ushort(const void *check_data, const char *string,
> + const char *fmt, int n_args, va_list ap)
> +{
> + const unsigned short *pval = check_data;
> +
> + _check_numbers_template("%hu", pval, string, fmt, n_args, ap);
> +}
> +
> +static int __init check_short(const void *check_data, const char *string,
> + const char *fmt, int n_args, va_list ap)
> +{
> + const short *pval = check_data;
> +
> + _check_numbers_template("%hd", pval, string, fmt, n_args, ap);
> +}
> +
> +static int __init check_uchar(const void *check_data, const char *string,
> + const char *fmt, int n_args, va_list ap)
> +{
> + const unsigned char *pval = check_data;
> +
> + _check_numbers_template("%hhu", pval, string, fmt, n_args, ap);
> +}
> +
> +static int __init check_char(const void *check_data, const char *string,
> + const char *fmt, int n_args, va_list ap)
> +{
> + const signed char *pval = check_data;
> +
> + _check_numbers_template("%hhd", pval, string, fmt, n_args, ap);
> +}
> +
> +/* Selection of interesting numbers to test, copied from test-kstrtox.c */
> +static const unsigned long long numbers[] __initconst = {
> + 0x0ULL,
> + 0x1ULL,
> + 0x7fULL,
> + 0x80ULL,
> + 0x81ULL,
> + 0xffULL,
> + 0x100ULL,
> + 0x101ULL,
> + 0x7fffULL,
> + 0x8000ULL,
> + 0x8001ULL,
> + 0xffffULL,
> + 0x10000ULL,
> + 0x10001ULL,
> + 0x7fffffffULL,
> + 0x80000000ULL,
> + 0x80000001ULL,
> + 0xffffffffULL,
> + 0x100000000ULL,
> + 0x100000001ULL,
> + 0x7fffffffffffffffULL,
> + 0x8000000000000000ULL,
> + 0x8000000000000001ULL,
> + 0xfffffffffffffffeULL,
> + 0xffffffffffffffffULL,
> +};
> +
> +#define value_representable_in_type(T, val) \
> +(is_signed_type(T) \
> + ? ((long long)(val) >= type_min(T)) && ((long long)(val) <= type_max(T)) \
> + : ((unsigned long long)(val) >= type_min(T)) && \
> + ((unsigned long long)(val) <= type_max(T)))
> +
> +#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 */ \
> + \
> + snprintf(test_buffer, BUF_SIZE, gen_fmt, expect_val); \
> + _test(fn, &expect_val, test_buffer, "%" scan_fmt, 1, &result); \
> +} while (0)
> +
> +#define simple_numbers_loop(T, gen_fmt, scan_fmt, fn) \
> +do { \
> + int i; \
> + \
> + for (i = 0; i < ARRAY_SIZE(numbers); i++) { \
> + if (!value_representable_in_type(T, numbers[i])) \
> + continue; \
> + \
> + test_one_number(T, gen_fmt, scan_fmt, numbers[i], fn); \
> + \
> + if (is_signed_type(T)) \
> + test_one_number(T, gen_fmt, scan_fmt, \
> + -numbers[i], fn); \
> + } \
> +} while (0)
> +
> +static void __init numbers_simple(void)
> +{
> + simple_numbers_loop(unsigned long long, "%llu", "llu", check_ull);
> + simple_numbers_loop(long long, "%lld", "lld", check_ll);
> + simple_numbers_loop(long long, "%lld", "lli", check_ll);
> + simple_numbers_loop(unsigned long long, "%llx", "llx", check_ull);
> + simple_numbers_loop(long long, "%llx", "llx", check_ll);
> + simple_numbers_loop(long long, "0x%llx", "lli", check_ll);
> + simple_numbers_loop(unsigned long long, "0x%llx", "llx", check_ull);
> + simple_numbers_loop(long long, "0x%llx", "llx", check_ll);
> +
> + simple_numbers_loop(unsigned long, "%lu", "lu", check_ulong);
> + simple_numbers_loop(long, "%ld", "ld", check_long);
> + simple_numbers_loop(long, "%ld", "li", check_long);
> + simple_numbers_loop(unsigned long, "%lx", "lx", check_ulong);
> + simple_numbers_loop(long, "%lx", "lx", check_long);
> + simple_numbers_loop(long, "0x%lx", "li", check_long);
> + simple_numbers_loop(unsigned long, "0x%lx", "lx", check_ulong);
> + simple_numbers_loop(long, "0x%lx", "lx", check_long);
> +
> + simple_numbers_loop(unsigned int, "%u", "u", check_uint);
> + simple_numbers_loop(int, "%d", "d", check_int);
> + simple_numbers_loop(int, "%d", "i", check_int);
> + simple_numbers_loop(unsigned int, "%x", "x", check_uint);
> + simple_numbers_loop(int, "%x", "x", check_int);
> + simple_numbers_loop(int, "0x%x", "i", check_int);
> + simple_numbers_loop(unsigned int, "0x%x", "x", check_uint);
> + simple_numbers_loop(int, "0x%x", "x", check_int);
> +
> + simple_numbers_loop(unsigned short, "%hu", "hu", check_ushort);
> + simple_numbers_loop(short, "%hd", "hd", check_short);
> + simple_numbers_loop(short, "%hd", "hi", check_short);
> + simple_numbers_loop(unsigned short, "%hx", "hx", check_ushort);
> + simple_numbers_loop(short, "%hx", "hx", check_short);
> + simple_numbers_loop(short, "0x%hx", "hi", check_short);
> + simple_numbers_loop(unsigned short, "0x%hx", "hx", check_ushort);
> + simple_numbers_loop(short, "0x%hx", "hx", check_short);
> +
> + simple_numbers_loop(unsigned char, "%hhu", "hhu", check_uchar);
> + simple_numbers_loop(signed char, "%hhd", "hhd", check_char);
> + simple_numbers_loop(signed char, "%hhd", "hhi", check_char);
> + simple_numbers_loop(unsigned char, "%hhx", "hhx", check_uchar);
> + simple_numbers_loop(signed char, "%hhx", "hhx", check_char);
> + simple_numbers_loop(signed char, "0x%hhx", "hhi", check_char);
> + simple_numbers_loop(unsigned char, "0x%hhx", "hhx", check_uchar);
> + simple_numbers_loop(signed char, "0x%hhx", "hhx", check_char);
> +}
> +
> +/*
> + * This gives a better variety of number "lengths" in a small sample than
> + * the raw prandom*() functions (Not mathematically rigorous!!).
> + * Variabilty of length and value is more important than perfect randomness.
> + */
> +static u32 __init next_test_random(u32 max_bits)
> +{
> + u32 n_bits = hweight32(prandom_u32_state(&rnd_state)) % (max_bits + 1);
> +
> + return prandom_u32_state(&rnd_state) & (UINT_MAX >> (32 - n_bits));
> +}
> +
> +static unsigned long long __init next_test_random_ull(void)
> +{
> + u32 rand1 = prandom_u32_state(&rnd_state);
> + u32 n_bits = (hweight32(rand1) * 3) % 64;
> + u64 val = (u64)prandom_u32_state(&rnd_state) * rand1;
> +
> + return val & (ULLONG_MAX >> (64 - n_bits));
> +}
> +
> +#define random_for_type(T) \
> + ((T)(sizeof(T) <= sizeof(u32) \
> + ? next_test_random(BITS_PER_TYPE(T)) \
> + : next_test_random_ull()))
> +
> +/*
> + * Define a pattern of negative and positive numbers to ensure we get
> + * some of both within the small number of samples in a test string.
> + */
> +#define NEGATIVES_PATTERN 0x3246 /* 00110010 01000110 */
> +
> +#define fill_random_array(arr) \
> +do { \
> + unsigned int neg_pattern = NEGATIVES_PATTERN; \
> + int i; \
> + \
> + for (i = 0; i < ARRAY_SIZE(arr); i++, neg_pattern >>= 1) { \
> + (arr)[i] = random_for_type(typeof((arr)[0])); \
> + if (is_signed_type(typeof((arr)[0])) && (neg_pattern & 1)) \
> + (arr)[i] = -(arr)[i]; \
> + } \
> +} while (0)
> +
> +/*
> + * Convenience wrapper around snprintf() to append at buf_pos in buf,
> + * updating buf_pos and returning the number of characters appended.
> + * On error buf_pos is not changed and return value is 0.
> + */
> +static int __init __printf(4, 5)
> +append_fmt(char *buf, int *buf_pos, int buf_len, const char *val_fmt, ...)
> +{
> + va_list ap;
> + int field_len;
> +
> + va_start(ap, val_fmt);
> + field_len = vsnprintf(buf + *buf_pos, buf_len - *buf_pos, val_fmt, ap);
> + va_end(ap);
> +
> + if (field_len < 0)
> + field_len = 0;
> +
> + *buf_pos += field_len;
> +
> + return field_len;
> +}
> +
> +/*
> + * Convenience function to append the field delimiter string
> + * to both the value string and format string buffers.
> + */
> +static void __init append_delim(char *str_buf, int *str_buf_pos, int str_buf_len,
> + char *fmt_buf, int *fmt_buf_pos, int fmt_buf_len,
> + const char *delim_str)
> +{
> + append_fmt(str_buf, str_buf_pos, str_buf_len, delim_str);
> + append_fmt(fmt_buf, fmt_buf_pos, fmt_buf_len, delim_str);
> +}
> +
> +#define test_array_8(fn, check_data, string, fmt, arr) \
> +do { \
> + BUILD_BUG_ON(ARRAY_SIZE(arr) != 8); \
> + _test(fn, check_data, string, fmt, 8, \
> + &(arr)[0], &(arr)[1], &(arr)[2], &(arr)[3], \
> + &(arr)[4], &(arr)[5], &(arr)[6], &(arr)[7]); \
> +} while (0)
> +
> +#define numbers_list_8(T, gen_fmt, field_sep, scan_fmt, fn) \
> +do { \
> + int i, pos = 0, fmt_pos = 0; \
> + T expect[8], result[8]; \
> + \
> + fill_random_array(expect); \
> + \
> + for (i = 0; i < ARRAY_SIZE(expect); i++) { \
> + if (i != 0) \
> + append_delim(test_buffer, &pos, BUF_SIZE, \
> + fmt_buffer, &fmt_pos, BUF_SIZE, \
> + field_sep); \
> + \
> + append_fmt(test_buffer, &pos, BUF_SIZE, gen_fmt, expect[i]); \
> + append_fmt(fmt_buffer, &fmt_pos, BUF_SIZE, "%%%s", scan_fmt); \
> + } \
> + \
> + test_array_8(fn, expect, test_buffer, fmt_buffer, result); \
> +} while (0)
> +
> +#define numbers_list_fix_width(T, gen_fmt, field_sep, width, scan_fmt, fn) \
> +do { \
> + char full_fmt[16]; \
> + \
> + snprintf(full_fmt, sizeof(full_fmt), "%u%s", width, scan_fmt); \
> + numbers_list_8(T, gen_fmt, field_sep, full_fmt, fn); \
> +} while (0)
> +
> +#define numbers_list_val_width(T, gen_fmt, field_sep, scan_fmt, fn) \
> +do { \
> + int i, val_len, pos = 0, fmt_pos = 0; \
> + T expect[8], result[8]; \
> + \
> + fill_random_array(expect); \
> + \
> + for (i = 0; i < ARRAY_SIZE(expect); i++) { \
> + if (i != 0) \
> + append_delim(test_buffer, &pos, BUF_SIZE, \
> + fmt_buffer, &fmt_pos, BUF_SIZE, field_sep);\
> + \
> + val_len = append_fmt(test_buffer, &pos, BUF_SIZE, gen_fmt, \
> + expect[i]); \
> + append_fmt(fmt_buffer, &fmt_pos, BUF_SIZE, \
> + "%%%u%s", val_len, scan_fmt); \
> + } \
> + \
> + test_array_8(fn, expect, test_buffer, fmt_buffer, result); \
> +} while (0)
> +
> +static void __init numbers_list(const char *delim)
> +{
> + numbers_list_8(unsigned long long, "%llu", delim, "llu", check_ull);
> + numbers_list_8(long long, "%lld", delim, "lld", check_ll);
> + numbers_list_8(long long, "%lld", delim, "lli", check_ll);
> + numbers_list_8(unsigned long long, "%llx", delim, "llx", check_ull);
> + numbers_list_8(unsigned long long, "0x%llx", delim, "llx", check_ull);
> + numbers_list_8(long long, "0x%llx", delim, "lli", check_ll);
> +
> + numbers_list_8(unsigned long, "%lu", delim, "lu", check_ulong);
> + numbers_list_8(long, "%ld", delim, "ld", check_long);
> + numbers_list_8(long, "%ld", delim, "li", check_long);
> + numbers_list_8(unsigned long, "%lx", delim, "lx", check_ulong);
> + numbers_list_8(unsigned long, "0x%lx", delim, "lx", check_ulong);
> + numbers_list_8(long, "0x%lx", delim, "li", check_long);
> +
> + numbers_list_8(unsigned int, "%u", delim, "u", check_uint);
> + numbers_list_8(int, "%d", delim, "d", check_int);
> + numbers_list_8(int, "%d", delim, "i", check_int);
> + numbers_list_8(unsigned int, "%x", delim, "x", check_uint);
> + numbers_list_8(unsigned int, "0x%x", delim, "x", check_uint);
> + numbers_list_8(int, "0x%x", delim, "i", check_int);
> +
> + numbers_list_8(unsigned short, "%hu", delim, "hu", check_ushort);
> + numbers_list_8(short, "%hd", delim, "hd", check_short);
> + numbers_list_8(short, "%hd", delim, "hi", check_short);
> + numbers_list_8(unsigned short, "%hx", delim, "hx", check_ushort);
> + numbers_list_8(unsigned short, "0x%hx", delim, "hx", check_ushort);
> + numbers_list_8(short, "0x%hx", delim, "hi", check_short);
> +
> + numbers_list_8(unsigned char, "%hhu", delim, "hhu", check_uchar);
> + numbers_list_8(signed char, "%hhd", delim, "hhd", check_char);
> + numbers_list_8(signed char, "%hhd", delim, "hhi", check_char);
> + numbers_list_8(unsigned char, "%hhx", delim, "hhx", check_uchar);
> + numbers_list_8(unsigned char, "0x%hhx", delim, "hhx", check_uchar);
> + numbers_list_8(signed char, "0x%hhx", delim, "hhi", check_char);
> +}
> +
> +/*
> + * List of numbers separated by delim. Each field width specifier is the
> + * maximum possible digits for the given type and base.
> + */
> +static void __init numbers_list_field_width_typemax(const char *delim)
> +{
> + numbers_list_fix_width(unsigned long long, "%llu", delim, 20, "llu", check_ull);
> + numbers_list_fix_width(long long, "%lld", delim, 20, "lld", check_ll);
> + numbers_list_fix_width(long long, "%lld", delim, 20, "lli", check_ll);
> + numbers_list_fix_width(unsigned long long, "%llx", delim, 16, "llx", check_ull);
> + numbers_list_fix_width(unsigned long long, "0x%llx", delim, 18, "llx", check_ull);
> + numbers_list_fix_width(long long, "0x%llx", delim, 18, "lli", check_ll);
> +
> +#if BITS_PER_LONG == 64
> + numbers_list_fix_width(unsigned long, "%lu", delim, 20, "lu", check_ulong);
> + numbers_list_fix_width(long, "%ld", delim, 20, "ld", check_long);
> + numbers_list_fix_width(long, "%ld", delim, 20, "li", check_long);
> + numbers_list_fix_width(unsigned long, "%lx", delim, 16, "lx", check_ulong);
> + numbers_list_fix_width(unsigned long, "0x%lx", delim, 18, "lx", check_ulong);
> + numbers_list_fix_width(long, "0x%lx", delim, 18, "li", check_long);
> +#else
> + numbers_list_fix_width(unsigned long, "%lu", delim, 10, "lu", check_ulong);
> + numbers_list_fix_width(long, "%ld", delim, 11, "ld", check_long);
> + numbers_list_fix_width(long, "%ld", delim, 11, "li", check_long);
> + numbers_list_fix_width(unsigned long, "%lx", delim, 8, "lx", check_ulong);
> + numbers_list_fix_width(unsigned long, "0x%lx", delim, 10, "lx", check_ulong);
> + numbers_list_fix_width(long, "0x%lx", delim, 10, "li", check_long);
> +#endif
> +
> + numbers_list_fix_width(unsigned int, "%u", delim, 10, "u", check_uint);
> + numbers_list_fix_width(int, "%d", delim, 11, "d", check_int);
> + numbers_list_fix_width(int, "%d", delim, 11, "i", check_int);
> + numbers_list_fix_width(unsigned int, "%x", delim, 8, "x", check_uint);
> + numbers_list_fix_width(unsigned int, "0x%x", delim, 10, "x", check_uint);
> + numbers_list_fix_width(int, "0x%x", delim, 10, "i", check_int);
> +
> + numbers_list_fix_width(unsigned short, "%hu", delim, 5, "hu", check_ushort);
> + numbers_list_fix_width(short, "%hd", delim, 6, "hd", check_short);
> + numbers_list_fix_width(short, "%hd", delim, 6, "hi", check_short);
> + numbers_list_fix_width(unsigned short, "%hx", delim, 4, "hx", check_ushort);
> + numbers_list_fix_width(unsigned short, "0x%hx", delim, 6, "hx", check_ushort);
> + numbers_list_fix_width(short, "0x%hx", delim, 6, "hi", check_short);
> +
> + numbers_list_fix_width(unsigned char, "%hhu", delim, 3, "hhu", check_uchar);
> + numbers_list_fix_width(signed char, "%hhd", delim, 4, "hhd", check_char);
> + numbers_list_fix_width(signed char, "%hhd", delim, 4, "hhi", check_char);
> + numbers_list_fix_width(unsigned char, "%hhx", delim, 2, "hhx", check_uchar);
> + numbers_list_fix_width(unsigned char, "0x%hhx", delim, 4, "hhx", check_uchar);
> + numbers_list_fix_width(signed char, "0x%hhx", delim, 4, "hhi", check_char);
> +}
> +
> +/*
> + * List of numbers separated by delim. Each field width specifier is the
> + * exact length of the corresponding value digits in the string being scanned.
> + */
> +static void __init numbers_list_field_width_val_width(const char *delim)
> +{
> + numbers_list_val_width(unsigned long long, "%llu", delim, "llu", check_ull);
> + numbers_list_val_width(long long, "%lld", delim, "lld", check_ll);
> + numbers_list_val_width(long long, "%lld", delim, "lli", check_ll);
> + numbers_list_val_width(unsigned long long, "%llx", delim, "llx", check_ull);
> + numbers_list_val_width(unsigned long long, "0x%llx", delim, "llx", check_ull);
> + numbers_list_val_width(long long, "0x%llx", delim, "lli", check_ll);
> +
> + numbers_list_val_width(unsigned long, "%lu", delim, "lu", check_ulong);
> + numbers_list_val_width(long, "%ld", delim, "ld", check_long);
> + numbers_list_val_width(long, "%ld", delim, "li", check_long);
> + numbers_list_val_width(unsigned long, "%lx", delim, "lx", check_ulong);
> + numbers_list_val_width(unsigned long, "0x%lx", delim, "lx", check_ulong);
> + numbers_list_val_width(long, "0x%lx", delim, "li", check_long);
> +
> + numbers_list_val_width(unsigned int, "%u", delim, "u", check_uint);
> + numbers_list_val_width(int, "%d", delim, "d", check_int);
> + numbers_list_val_width(int, "%d", delim, "i", check_int);
> + numbers_list_val_width(unsigned int, "%x", delim, "x", check_uint);
> + numbers_list_val_width(unsigned int, "0x%x", delim, "x", check_uint);
> + numbers_list_val_width(int, "0x%x", delim, "i", check_int);
> +
> + numbers_list_val_width(unsigned short, "%hu", delim, "hu", check_ushort);
> + numbers_list_val_width(short, "%hd", delim, "hd", check_short);
> + numbers_list_val_width(short, "%hd", delim, "hi", check_short);
> + numbers_list_val_width(unsigned short, "%hx", delim, "hx", check_ushort);
> + numbers_list_val_width(unsigned short, "0x%hx", delim, "hx", check_ushort);
> + numbers_list_val_width(short, "0x%hx", delim, "hi", check_short);
> +
> + numbers_list_val_width(unsigned char, "%hhu", delim, "hhu", check_uchar);
> + numbers_list_val_width(signed char, "%hhd", delim, "hhd", check_char);
> + numbers_list_val_width(signed char, "%hhd", delim, "hhi", check_char);
> + numbers_list_val_width(unsigned char, "%hhx", delim, "hhx", check_uchar);
> + numbers_list_val_width(unsigned char, "0x%hhx", delim, "hhx", check_uchar);
> + numbers_list_val_width(signed char, "0x%hhx", delim, "hhi", check_char);
> +}
> +
> +/*
> + * Slice a continuous string of digits without field delimiters, containing
> + * numbers of varying length, using the field width to extract each group
> + * of digits. For example the hex values c0,3,bf01,303 would have a
> + * string representation of "c03bf01303" and extracted with "%2x%1x%4x%3x".
> + */
> +static void __init numbers_slice(void)
> +{
> + numbers_list_field_width_val_width("");
> +}
> +
> +#define test_number_prefix(T, str, scan_fmt, expect0, expect1, n_args, fn) \
> +do { \
> + const T expect[2] = { expect0, expect1 }; \
> + T result[2] = {~expect[0], ~expect[1]}; \
> + \
> + _test(fn, &expect, str, scan_fmt, n_args, &result[0], &result[1]); \
> +} while (0)
> +
> +/*
> + * Number prefix is >= field width.
> + * Expected behaviour is derived from testing userland sscanf.
> + */
> +static void __init numbers_prefix_overflow(void)
> +{
> + /*
> + * Negative decimal with a field of width 1, should quit scanning
> + * and return 0.
> + */
> + test_number_prefix(long long, "-1 1", "%1lld %lld", 0, 0, 0, check_ll);
> + test_number_prefix(long, "-1 1", "%1ld %ld", 0, 0, 0, check_long);
> + test_number_prefix(int, "-1 1", "%1d %d", 0, 0, 0, check_int);
> + test_number_prefix(short, "-1 1", "%1hd %hd", 0, 0, 0, check_short);
> + test_number_prefix(signed char, "-1 1", "%1hhd %hhd", 0, 0, 0, check_char);
> +
> + test_number_prefix(long long, "-1 1", "%1lli %lli", 0, 0, 0, check_ll);
> + test_number_prefix(long, "-1 1", "%1li %li", 0, 0, 0, check_long);
> + test_number_prefix(int, "-1 1", "%1i %i", 0, 0, 0, check_int);
> + test_number_prefix(short, "-1 1", "%1hi %hi", 0, 0, 0, check_short);
> + test_number_prefix(signed char, "-1 1", "%1hhi %hhi", 0, 0, 0, check_char);
> +
> + /*
> + * 0x prefix in a field of width 1: 0 is a valid digit so should
> + * convert. Next field scan starts at the 'x' which isn't a digit so
> + * scan quits with one field converted.
> + */
> + test_number_prefix(unsigned long long, "0xA7", "%1llx%llx", 0, 0, 1, check_ull);
> + test_number_prefix(unsigned long, "0xA7", "%1lx%lx", 0, 0, 1, check_ulong);
> + test_number_prefix(unsigned int, "0xA7", "%1x%x", 0, 0, 1, check_uint);
> + test_number_prefix(unsigned short, "0xA7", "%1hx%hx", 0, 0, 1, check_ushort);
> + test_number_prefix(unsigned char, "0xA7", "%1hhx%hhx", 0, 0, 1, check_uchar);
> + test_number_prefix(long long, "0xA7", "%1lli%llx", 0, 0, 1, check_ll);
> + test_number_prefix(long, "0xA7", "%1li%lx", 0, 0, 1, check_long);
> + test_number_prefix(int, "0xA7", "%1i%x", 0, 0, 1, check_int);
> + test_number_prefix(short, "0xA7", "%1hi%hx", 0, 0, 1, check_short);
> + test_number_prefix(char, "0xA7", "%1hhi%hhx", 0, 0, 1, check_char);
> +
> + /*
> + * 0x prefix in a field of width 2 using %x conversion: first field
> + * converts to 0. Next field scan starts at the character after "0x".
> + * Both fields will convert.
> + */
> + test_number_prefix(unsigned long long, "0xA7", "%2llx%llx", 0, 0xa7, 2, check_ull);
> + test_number_prefix(unsigned long, "0xA7", "%2lx%lx", 0, 0xa7, 2, check_ulong);
> + test_number_prefix(unsigned int, "0xA7", "%2x%x", 0, 0xa7, 2, check_uint);
> + test_number_prefix(unsigned short, "0xA7", "%2hx%hx", 0, 0xa7, 2, check_ushort);
> + test_number_prefix(unsigned char, "0xA7", "%2hhx%hhx", 0, 0xa7, 2, check_uchar);
> +
> + /*
> + * 0x prefix in a field of width 2 using %i conversion: first field
> + * converts to 0. Next field scan starts at the character after "0x",
> + * which will convert if can be intepreted as decimal but will fail
> + * if it contains any hex digits (since no 0x prefix).
> + */
> + test_number_prefix(long long, "0x67", "%2lli%lli", 0, 67, 2, check_ll);
> + test_number_prefix(long, "0x67", "%2li%li", 0, 67, 2, check_long);
> + test_number_prefix(int, "0x67", "%2i%i", 0, 67, 2, check_int);
> + test_number_prefix(short, "0x67", "%2hi%hi", 0, 67, 2, check_short);
> + test_number_prefix(char, "0x67", "%2hhi%hhi", 0, 67, 2, check_char);
> +
> + test_number_prefix(long long, "0xA7", "%2lli%lli", 0, 0, 1, check_ll);
> + test_number_prefix(long, "0xA7", "%2li%li", 0, 0, 1, check_long);
> + test_number_prefix(int, "0xA7", "%2i%i", 0, 0, 1, check_int);
> + test_number_prefix(short, "0xA7", "%2hi%hi", 0, 0, 1, check_short);
> + test_number_prefix(char, "0xA7", "%2hhi%hhi", 0, 0, 1, check_char);
> +}
> +
> +#define _test_simple_strtoxx(T, fn, gen_fmt, expect, base) \
> +do { \
> + T got; \
> + char *endp; \
> + int len; \
> + bool fail = false; \
> + \
> + total_tests++; \
> + len = snprintf(test_buffer, BUF_SIZE, gen_fmt, expect); \
> + got = (fn)(test_buffer, &endp, base); \
> + pr_debug(#fn "(\"%s\", %d) -> " gen_fmt "\n", test_buffer, base, got); \
> + if (got != (expect)) { \
> + fail = true; \
> + pr_warn(#fn "(\"%s\", %d): got " gen_fmt " expected " gen_fmt "\n", \
> + test_buffer, base, got, expect); \
> + } else if (endp != test_buffer + len) { \
> + fail = true; \
> + pr_warn(#fn "(\"%s\", %d) startp=0x%px got endp=0x%px expected 0x%px\n", \
> + test_buffer, base, test_buffer, \
> + test_buffer + len, endp); \
> + } \
> + \
> + if (fail) \
> + failed_tests++; \
> +} while (0)
> +
> +#define test_simple_strtoxx(T, fn, gen_fmt, base) \
> +do { \
> + int i; \
> + \
> + for (i = 0; i < ARRAY_SIZE(numbers); i++) { \
> + _test_simple_strtoxx(T, fn, gen_fmt, (T)numbers[i], base); \
> + \
> + if (is_signed_type(T)) \
> + _test_simple_strtoxx(T, fn, gen_fmt, \
> + -(T)numbers[i], base); \
> + } \
> +} while (0)
> +
> +static void __init test_simple_strtoull(void)
> +{
> + test_simple_strtoxx(unsigned long long, simple_strtoull, "%llu", 10);
> + test_simple_strtoxx(unsigned long long, simple_strtoull, "%llu", 0);
> + test_simple_strtoxx(unsigned long long, simple_strtoull, "%llx", 16);
> + test_simple_strtoxx(unsigned long long, simple_strtoull, "0x%llx", 16);
> + test_simple_strtoxx(unsigned long long, simple_strtoull, "0x%llx", 0);
> +}
> +
> +static void __init test_simple_strtoll(void)
> +{
> + test_simple_strtoxx(long long, simple_strtoll, "%lld", 10);
> + test_simple_strtoxx(long long, simple_strtoll, "%lld", 0);
> + test_simple_strtoxx(long long, simple_strtoll, "%llx", 16);
> + test_simple_strtoxx(long long, simple_strtoll, "0x%llx", 16);
> + test_simple_strtoxx(long long, simple_strtoll, "0x%llx", 0);
> +}
> +
> +static void __init test_simple_strtoul(void)
> +{
> + test_simple_strtoxx(unsigned long, simple_strtoul, "%lu", 10);
> + test_simple_strtoxx(unsigned long, simple_strtoul, "%lu", 0);
> + test_simple_strtoxx(unsigned long, simple_strtoul, "%lx", 16);
> + test_simple_strtoxx(unsigned long, simple_strtoul, "0x%lx", 16);
> + test_simple_strtoxx(unsigned long, simple_strtoul, "0x%lx", 0);
> +}
> +
> +static void __init test_simple_strtol(void)
> +{
> + test_simple_strtoxx(long, simple_strtol, "%ld", 10);
> + test_simple_strtoxx(long, simple_strtol, "%ld", 0);
> + test_simple_strtoxx(long, simple_strtol, "%lx", 16);
> + test_simple_strtoxx(long, simple_strtol, "0x%lx", 16);
> + test_simple_strtoxx(long, simple_strtol, "0x%lx", 0);
> +}
> +
> +/* Selection of common delimiters/separators between numbers in a string. */
> +static const char * const number_delimiters[] __initconst = {
> + " ", ":", ",", "-", "/",
> +};
> +
> +static void __init test_numbers(void)
> +{
> + int i;
> +
> + /* String containing only one number. */
> + numbers_simple();
> +
> + /* String with multiple numbers separated by delimiter. */
> + for (i = 0; i < ARRAY_SIZE(number_delimiters); i++) {
> + numbers_list(number_delimiters[i]);
> +
> + /* Field width may be longer than actual field digits. */
> + numbers_list_field_width_typemax(number_delimiters[i]);
> +
> + /* Each field width exactly length of actual field digits. */
> + numbers_list_field_width_val_width(number_delimiters[i]);
> + }
> +
> + /* Slice continuous sequence of digits using field widths. */
> + numbers_slice();
> +
> + numbers_prefix_overflow();
> +}
> +
> +static void __init selftest(void)
> +{
> + test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
> + if (!test_buffer)
> + return;
> +
> + fmt_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
> + if (!fmt_buffer) {
> + kfree(test_buffer);
> + return;
> + }
> +
> + prandom_seed_state(&rnd_state, 3141592653589793238ULL);
> +
> + test_numbers();
> +
> + test_simple_strtoull();
> + test_simple_strtoll();
> + test_simple_strtoul();
> + test_simple_strtol();
> +
> + kfree(fmt_buffer);
> + kfree(test_buffer);
> +}
> +
> +KSTM_MODULE_LOADERS(test_scanf);
> +MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.cirrus.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.20.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 4/4] selftests: lib: Add wrapper script for test_scanf
2021-02-03 16:50 ` [PATCH v4 4/4] selftests: lib: Add wrapper script for test_scanf Richard Fitzgerald
@ 2021-02-03 19:48 ` Andy Shevchenko
0 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2021-02-03 19:48 UTC (permalink / raw)
To: Richard Fitzgerald
Cc: pmladek, rostedt, sergey.senozhatsky, linux, shuah, linux-kernel,
linux-kselftest, patches
On Wed, Feb 03, 2021 at 04:50:09PM +0000, Richard Fitzgerald wrote:
> Adds a wrapper shell script for the test_scanf module.
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> ---
> tools/testing/selftests/lib/Makefile | 2 +-
> tools/testing/selftests/lib/config | 1 +
> tools/testing/selftests/lib/scanf.sh | 4 ++++
> 3 files changed, 6 insertions(+), 1 deletion(-)
> create mode 100755 tools/testing/selftests/lib/scanf.sh
>
> diff --git a/tools/testing/selftests/lib/Makefile b/tools/testing/selftests/lib/Makefile
> index a105f094676e..ee71fc99d5b5 100644
> --- a/tools/testing/selftests/lib/Makefile
> +++ b/tools/testing/selftests/lib/Makefile
> @@ -4,6 +4,6 @@
> # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
> all:
>
> -TEST_PROGS := printf.sh bitmap.sh prime_numbers.sh strscpy.sh
> +TEST_PROGS := printf.sh bitmap.sh prime_numbers.sh scanf.sh strscpy.sh
>
> include ../lib.mk
> diff --git a/tools/testing/selftests/lib/config b/tools/testing/selftests/lib/config
> index b80ee3f6e265..776c8c42e78d 100644
> --- a/tools/testing/selftests/lib/config
> +++ b/tools/testing/selftests/lib/config
> @@ -1,4 +1,5 @@
> CONFIG_TEST_PRINTF=m
> +CONFIG_TEST_SCANTF=m
> CONFIG_TEST_BITMAP=m
> CONFIG_PRIME_NUMBERS=m
> CONFIG_TEST_STRSCPY=m
> diff --git a/tools/testing/selftests/lib/scanf.sh b/tools/testing/selftests/lib/scanf.sh
> new file mode 100755
> index 000000000000..b59b8ba561c3
> --- /dev/null
> +++ b/tools/testing/selftests/lib/scanf.sh
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# Tests the scanf infrastructure using test_scanf kernel module.
> +$(dirname $0)/../kselftest/module.sh "scanf" test_scanf
> --
> 2.20.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/4] lib: vsprintf: scanf: Negative number must have field width > 1
2021-02-03 16:50 [PATCH v4 1/4] lib: vsprintf: scanf: Negative number must have field width > 1 Richard Fitzgerald
` (2 preceding siblings ...)
2021-02-03 16:50 ` [PATCH v4 4/4] selftests: lib: Add wrapper script for test_scanf Richard Fitzgerald
@ 2021-02-03 19:48 ` Andy Shevchenko
3 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2021-02-03 19:48 UTC (permalink / raw)
To: Richard Fitzgerald
Cc: pmladek, rostedt, sergey.senozhatsky, linux, shuah, linux-kernel,
linux-kselftest, patches
On Wed, Feb 03, 2021 at 04:50:06PM +0000, Richard Fitzgerald wrote:
> If a signed number field starts with a '-' the field width must be > 1,
> or unlimited, to allow at least one digit after the '-'.
>
> This patch adds a check for this. If a signed field starts with '-'
> and field_width == 1 the scanf will quit.
>
> It is ok for a signed number field to have a field width of 1 if it
> starts with a digit. In that case the single digit can be converted.
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> ---
> lib/vsprintf.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..28bb26cd1f67 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -3434,8 +3434,12 @@ 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))
> --
> 2.20.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf
2021-02-03 19:45 ` Andy Shevchenko
@ 2021-02-04 16:35 ` Petr Mladek
2021-02-05 11:28 ` Richard Fitzgerald
2021-02-08 11:47 ` Richard Fitzgerald
0 siblings, 2 replies; 15+ messages in thread
From: Petr Mladek @ 2021-02-04 16:35 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Richard Fitzgerald, rostedt, sergey.senozhatsky, linux, shuah,
linux-kernel, linux-kselftest, patches
On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
> On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:
> > The existing code attempted to handle numbers by doing a strto[u]l(),
> > ignoring the field width, and then repeatedly dividing to extract the
> > field out of the full 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 dividing can recover the correct value.
> ...
>
> > + for (; max_chars > 0; max_chars--) {
>
> Less fragile is to write
>
> while (max_chars--)
Except that the original was more obvious at least for me.
I always prefer more readable code when the compiler might do
the optimization easily. But this is my personal taste.
I am fine with both variants.
>
> This allows max_char to be an unsigned type.
>
> Moreover...
>
> > + return _parse_integer_limit(s, base, p, INT_MAX);
>
> You have inconsistency with INT_MAX vs, size_t above.
Ah, this was on my request. INT_MAX is already used on many other
locations in vsnprintf() for this purpose.
An alternative is to fix all the other locations. We would need to
check if it is really safe. Well, I do not want to force Richard
to fix this historical mess. He already put a lot lot of effort
into fixing this long term issue.
...
> > - 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;
> > + }
>
> It's redundant if I'm not mistaken.
Also this is more obvious and less error prone from my POV.
But I agree that it is redundant. I am not sure if this
function is used in some fast paths.
Again I am fine with both variants.
> > + cp = _parse_integer_fixup_radix(startp, &base);
> > + if ((cp - startp) >= max_chars) {
> > + cp = startp + max_chars;
> > + goto out;
> > + }
>
> This will be exactly the same, no?
Best Regards,
Petr
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf
2021-02-04 16:35 ` Petr Mladek
@ 2021-02-05 11:28 ` Richard Fitzgerald
2021-02-05 12:50 ` Andy Shevchenko
2021-02-08 11:47 ` Richard Fitzgerald
1 sibling, 1 reply; 15+ messages in thread
From: Richard Fitzgerald @ 2021-02-05 11:28 UTC (permalink / raw)
To: Petr Mladek, Andy Shevchenko
Cc: rostedt, sergey.senozhatsky, linux, shuah, linux-kernel,
linux-kselftest, patches
On 04/02/2021 16:35, Petr Mladek wrote:
> On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
>> On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:
>>> The existing code attempted to handle numbers by doing a strto[u]l(),
>>> ignoring the field width, and then repeatedly dividing to extract the
>>> field out of the full 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 dividing can recover the correct value.
>
>> ...
>>
>>> + for (; max_chars > 0; max_chars--) {
>>
>> Less fragile is to write
>>
>> while (max_chars--)
>
> Except that the original was more obvious at least for me.
> I always prefer more readable code when the compiler might do
> the optimization easily. But this is my personal taste.
> I am fine with both variants.
>
>>
>> This allows max_char to be an unsigned type.
>>
>> Moreover...
>>
>>> + return _parse_integer_limit(s, base, p, INT_MAX);
>>
>> You have inconsistency with INT_MAX vs, size_t above.
>
> Ah, this was on my request. INT_MAX is already used on many other
> locations in vsnprintf() for this purpose.
>
I originally had UINT_MAX and changed on Petr's request to be
consistent with other code. (Sorry Andy - my mistake not including
you on the earlier review versions).
But 0 < INT_MAX < UINT_MAX, so ok to pass to an unsigned. And as Petr
said on his original review, INT_MAX is "big enough".
I don't mind either way.
> An alternative is to fix all the other locations. We would need to
> check if it is really safe. Well, I do not want to force Richard
> to fix this historical mess. He already put a lot lot of effort
> into fixing this long term issue.
>
> ...
>
>>> - 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;
>>> + }
>>
>> It's redundant if I'm not mistaken.
>
> Also this is more obvious and less error prone from my POV.
> But I agree that it is redundant. I am not sure if this
> function is used in some fast paths.
>
> Again I am fine with both variants.
>
>>> + cp = _parse_integer_fixup_radix(startp, &base);
>>> + if ((cp - startp) >= max_chars) {
>>> + cp = startp + max_chars;
>>> + goto out;
>>> + }
>>
>> This will be exactly the same, no?
>
> Best Regards,
> Petr
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf
2021-02-05 11:28 ` Richard Fitzgerald
@ 2021-02-05 12:50 ` Andy Shevchenko
2021-02-05 15:23 ` David Laight
2021-02-08 17:56 ` Petr Mladek
0 siblings, 2 replies; 15+ messages in thread
From: Andy Shevchenko @ 2021-02-05 12:50 UTC (permalink / raw)
To: Richard Fitzgerald
Cc: Petr Mladek, Andy Shevchenko, Steven Rostedt, Sergey Senozhatsky,
Rasmus Villemoes, Shuah Khan, Linux Kernel Mailing List,
open list:KERNEL SELFTEST FRAMEWORK, patches
On Fri, Feb 5, 2021 at 1:35 PM Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
> On 04/02/2021 16:35, Petr Mladek wrote:
> > On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
> >> On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:
...
> >>> + for (; max_chars > 0; max_chars--) {
> >>
> >> Less fragile is to write
> >>
> >> while (max_chars--)
> >
> > Except that the original was more obvious at least for me.
> > I always prefer more readable code when the compiler might do
> > the optimization easily. But this is my personal taste.
> > I am fine with both variants.
I *slightly* prefer while-loop *in this case* due to less characters
to parse to understand the logic.
> >> This allows max_char to be an unsigned type.
> >>
> >> Moreover...
> >>
> >>> + return _parse_integer_limit(s, base, p, INT_MAX);
> >>
> >> You have inconsistency with INT_MAX vs, size_t above.
> >
> > Ah, this was on my request. INT_MAX is already used on many other
> > locations in vsnprintf() for this purpose.
>
> I originally had UINT_MAX and changed on Petr's request to be
> consistent with other code. (Sorry Andy - my mistake not including
> you on the earlier review versions).
>
> But 0 < INT_MAX < UINT_MAX, so ok to pass to an unsigned. And as Petr
> said on his original review, INT_MAX is "big enough".
Some code has INT_MAX, some has UINT_MAX, while the parameter is size_t.
I think all of these inconsistencies should have a comment either in
the code, or in the commit message, or in the cover letter (depending
on the importance).
Or being fixed to be more consistent with existing code. Whichever you
consider better.
> I don't mind either way.
>
> > An alternative is to fix all the other locations. We would need to
> > check if it is really safe. Well, I do not want to force Richard
> > to fix this historical mess. He already put a lot lot of effort
> > into fixing this long term issue.
...
> >>> - 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;
> >>> + }
> >>
> >> It's redundant if I'm not mistaken.
> >
> > Also this is more obvious and less error prone from my POV.
> > But I agree that it is redundant. I am not sure if this
> > function is used in some fast paths.
> >
> > Again I am fine with both variants.
I think we don't need a (redundant) code, but rather a comment.
> >>> + cp = _parse_integer_fixup_radix(startp, &base);
> >>> + if ((cp - startp) >= max_chars) {
> >>> + cp = startp + max_chars;
> >>> + goto out;
> >>> + }
> >>
> >> This will be exactly the same, no?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf
2021-02-05 12:50 ` Andy Shevchenko
@ 2021-02-05 15:23 ` David Laight
2021-02-05 15:53 ` Andy Shevchenko
2021-02-08 17:56 ` Petr Mladek
1 sibling, 1 reply; 15+ messages in thread
From: David Laight @ 2021-02-05 15:23 UTC (permalink / raw)
To: 'Andy Shevchenko', Richard Fitzgerald
Cc: Petr Mladek, Andy Shevchenko, Steven Rostedt, Sergey Senozhatsky,
Rasmus Villemoes, Shuah Khan, Linux Kernel Mailing List,
open list:KERNEL SELFTEST FRAMEWORK, patches
From: Andy Shevchenko
> Sent: 05 February 2021 12:51
>
> On Fri, Feb 5, 2021 at 1:35 PM Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
> > On 04/02/2021 16:35, Petr Mladek wrote:
> > > On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
> > >> On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:
>
> ...
>
> > >>> + for (; max_chars > 0; max_chars--) {
> > >>
> > >> Less fragile is to write
> > >>
> > >> while (max_chars--)
> > >
> > > Except that the original was more obvious at least for me.
> > > I always prefer more readable code when the compiler might do
> > > the optimization easily. But this is my personal taste.
> > > I am fine with both variants.
>
> I *slightly* prefer while-loop *in this case* due to less characters
> to parse to understand the logic.
The two loops are also have different values for 'max_chars'
inside the loop body.
If 'max_chars' is known to be non-zero the do ... while (--max_chars);
loop will probable generate better code.
But there is no accounting for just how odd some decisions gcc
makes are.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf
2021-02-05 15:23 ` David Laight
@ 2021-02-05 15:53 ` Andy Shevchenko
0 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2021-02-05 15:53 UTC (permalink / raw)
To: David Laight
Cc: Richard Fitzgerald, Petr Mladek, Andy Shevchenko, Steven Rostedt,
Sergey Senozhatsky, Rasmus Villemoes, Shuah Khan,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
patches
On Fri, Feb 5, 2021 at 5:23 PM David Laight <David.Laight@aculab.com> wrote:
> From: Andy Shevchenko
> > Sent: 05 February 2021 12:51
> > On Fri, Feb 5, 2021 at 1:35 PM Richard Fitzgerald
> > <rf@opensource.cirrus.com> wrote:
> > > On 04/02/2021 16:35, Petr Mladek wrote:
> > > > On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
> > > >> On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:
> >
> > ...
> >
> > > >>> + for (; max_chars > 0; max_chars--) {
> > > >>
> > > >> Less fragile is to write
> > > >>
> > > >> while (max_chars--)
> > > >
> > > > Except that the original was more obvious at least for me.
> > > > I always prefer more readable code when the compiler might do
> > > > the optimization easily. But this is my personal taste.
> > > > I am fine with both variants.
> >
> > I *slightly* prefer while-loop *in this case* due to less characters
> > to parse to understand the logic.
>
> The two loops are also have different values for 'max_chars'
> inside the loop body.
off-by-one to be precise.
> If 'max_chars' is known to be non-zero the do ... while (--max_chars);
> loop will probable generate better code.
What?! while (--x) and while(x--) are equivalent.
> But there is no accounting for just how odd some decisions gcc
> makes are.
Why should we care about the compiler bugs here?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf
2021-02-04 16:35 ` Petr Mladek
2021-02-05 11:28 ` Richard Fitzgerald
@ 2021-02-08 11:47 ` Richard Fitzgerald
1 sibling, 0 replies; 15+ messages in thread
From: Richard Fitzgerald @ 2021-02-08 11:47 UTC (permalink / raw)
To: Petr Mladek, Andy Shevchenko
Cc: rostedt, sergey.senozhatsky, linux, shuah, linux-kernel,
linux-kselftest, patches
On 04/02/2021 16:35, Petr Mladek wrote:
> On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
>> On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:
>>> The existing code attempted to handle numbers by doing a strto[u]l(),
>>> ignoring the field width, and then repeatedly dividing to extract the
>>> field out of the full 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 dividing can recover the correct value.
>
>> ...
>>
>>> + for (; max_chars > 0; max_chars--) {
>>
>> Less fragile is to write
>>
>> while (max_chars--)
>
> Except that the original was more obvious at least for me.
> I always prefer more readable code when the compiler might do
> the optimization easily. But this is my personal taste.
> I am fine with both variants.
>
>>
>> This allows max_char to be an unsigned type.
>>
>> Moreover...
>>
>>> + return _parse_integer_limit(s, base, p, INT_MAX);
>>
>> You have inconsistency with INT_MAX vs, size_t above.
>
> Ah, this was on my request. INT_MAX is already used on many other
> locations in vsnprintf() for this purpose.
>
Strictly speaking this should be SIZE_MAX because the argument is a
size_t.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf
2021-02-05 12:50 ` Andy Shevchenko
2021-02-05 15:23 ` David Laight
@ 2021-02-08 17:56 ` Petr Mladek
1 sibling, 0 replies; 15+ messages in thread
From: Petr Mladek @ 2021-02-08 17:56 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Richard Fitzgerald, Andy Shevchenko, Steven Rostedt,
Sergey Senozhatsky, Rasmus Villemoes, Shuah Khan,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
patches
On Fri 2021-02-05 14:50:56, Andy Shevchenko wrote:
> On Fri, Feb 5, 2021 at 1:35 PM Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
> > On 04/02/2021 16:35, Petr Mladek wrote:
> > > On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
> > >> On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:
> > >> This allows max_char to be an unsigned type.
> > >>
> > >> Moreover...
> > >>
> > >>> + return _parse_integer_limit(s, base, p, INT_MAX);
> > >>
> > >> You have inconsistency with INT_MAX vs, size_t above.
> > >
> > > Ah, this was on my request. INT_MAX is already used on many other
> > > locations in vsnprintf() for this purpose.
> >
> > I originally had UINT_MAX and changed on Petr's request to be
> > consistent with other code. (Sorry Andy - my mistake not including
> > you on the earlier review versions).
> >
> > But 0 < INT_MAX < UINT_MAX, so ok to pass to an unsigned. And as Petr
> > said on his original review, INT_MAX is "big enough".
>
> Some code has INT_MAX, some has UINT_MAX, while the parameter is size_t.
Yeah, if I remember correctly I wanted to have INT_MAX everywhere but
I did not want to nitpick about it in the later versions. It looked
like an arbitrary number anyway.
> I think all of these inconsistencies should have a comment either in
> the code, or in the commit message, or in the cover letter (depending
> on the importance).
> Or being fixed to be more consistent with existing code. Whichever you
> consider better.
OK, you made me to do some archaeology. The INT_MAX limit has
been added into vsnprintf() in 2.6.2 by the commit:
Author: Linus Torvalds <torvalds@home.osdl.org>
Date: Mon Feb 2 21:17:29 2004 -0800
Warn loudly if somebody passes a negative value as
the size to "vsnprintf()".
That's a pretty clear case of overflow.
It might catch problems. And the limit seems to have worked all the time.
IMHO, it would make sense to have INT_MAX limit also in
_parse_integer_limit() and WARN() when a larger value is passed.
By other words, it would mean to add this check and use INT_MAX
everywhere in this patch.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-02-08 19:15 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-03 16:50 [PATCH v4 1/4] lib: vsprintf: scanf: Negative number must have field width > 1 Richard Fitzgerald
2021-02-03 16:50 ` [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf Richard Fitzgerald
2021-02-03 19:45 ` Andy Shevchenko
2021-02-04 16:35 ` Petr Mladek
2021-02-05 11:28 ` Richard Fitzgerald
2021-02-05 12:50 ` Andy Shevchenko
2021-02-05 15:23 ` David Laight
2021-02-05 15:53 ` Andy Shevchenko
2021-02-08 17:56 ` Petr Mladek
2021-02-08 11:47 ` Richard Fitzgerald
2021-02-03 16:50 ` [PATCH v4 3/4] lib: test_scanf: Add tests for sscanf number conversion Richard Fitzgerald
2021-02-03 19:47 ` Andy Shevchenko
2021-02-03 16:50 ` [PATCH v4 4/4] selftests: lib: Add wrapper script for test_scanf Richard Fitzgerald
2021-02-03 19:48 ` Andy Shevchenko
2021-02-03 19:48 ` [PATCH v4 1/4] lib: vsprintf: scanf: Negative number must have field width > 1 Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).