| Message ID | a56def269d7885840a19a57aca0169891f5f0f32.1704168510.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kstrtox: introduce memparse_safe() | expand |
Hi Qu, On Tue, Jan 2, 2024 at 5:13 AM Qu Wenruo <wqu@suse.com> wrote: > The new tests cases for memparse_safe() include: > > - The existing test cases for kstrtoull() > Including all the 3 bases (8, 10, 16), and all the ok and failure > cases. > Although there are something we need to verify specific for > memparse_safe(): > > * @retptr and @value are not modified for failure cases > > * return value are correct for failure cases > > * @retptr is correct for the good cases > > - New test cases > Not only testing the result value, but also the @retptr, including: > > * good cases with extra tailing chars, but without valid prefix > The @retptr should point to the first char after a valid string. > 3 cases for all the 3 bases. > > * good cases with extra tailing chars, with valid prefix > 5 cases for all the suffixes. > > * bad cases without any number but stray suffix > Should be rejected with -EINVAL > > Signed-off-by: Qu Wenruo <wqu@suse.com> Thanks for your patch! > --- a/lib/test-kstrtox.c > +++ b/lib/test-kstrtox.c > @@ -268,6 +268,237 @@ static void __init test_kstrtoll_ok(void) > TEST_OK(kstrtoll, long long, "%lld", test_ll_ok); > } > > +/* > + * The special pattern to make sure the result is not modified for error cases. > + */ > +#define ULL_PATTERN (0xefefefef7a7a7a7aULL) > +#if BITS_PER_LONG == 32 > +#define POINTER_PATTERN (0xefef7a7a7aUL) This pattern needs 40 bits to fit, so it doesn't fit in a 32-bit unsigned long or pointer. Probably you wanted to use 0xef7a7a7aUL instead? > +#else > +#define POINTER_PATTERN (ULL_PATTERN) > +#endif Shouldn't a simple cast to uintptr_t work fine for both 32-bit and 64-bit systems: #define POINTER_PATTERN ((uintptr_t)ULL_PATTERN) Or even better, incorporate the cast to a pointer: #define POINTER_PATTERN ((void *)(uintptr_t)ULL_PATTERN) so you can drop the extra cast when assigning/comparing retptr below. > + > +/* Want to include "E" suffix for full coverage. */ > +#define MEMPARSE_TEST_SUFFIX (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\ > + MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\ > + MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E) > + > +static void __init test_memparse_safe_fail(void) > +{ [...] > + for_each_test(i, tests) { > + const struct memparse_test_fail *t = &tests[i]; > + unsigned long long tmp = ULL_PATTERN; > + char *retptr = (char *)POINTER_PATTERN; > + int ret; [...] + if (retptr != (char *)POINTER_PATTERN) Gr{oetje,eeting}s, Geert
On Tue, 2 Jan 2024 14:42:13 +1030, Qu Wenruo wrote: > The new tests cases for memparse_safe() include: > > - The existing test cases for kstrtoull() > Including all the 3 bases (8, 10, 16), and all the ok and failure > cases. > Although there are something we need to verify specific for > memparse_safe(): > > * @retptr and @value are not modified for failure cases > > * return value are correct for failure cases > > * @retptr is correct for the good cases > > - New test cases > Not only testing the result value, but also the @retptr, including: > > * good cases with extra tailing chars, but without valid prefix > The @retptr should point to the first char after a valid string. > 3 cases for all the 3 bases. > > * good cases with extra tailing chars, with valid prefix > 5 cases for all the suffixes. > > * bad cases without any number but stray suffix > Should be rejected with -EINVAL > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > lib/test-kstrtox.c | 235 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 235 insertions(+) > > diff --git a/lib/test-kstrtox.c b/lib/test-kstrtox.c > index f355f67169b6..97c2f65a16cb 100644 > --- a/lib/test-kstrtox.c > +++ b/lib/test-kstrtox.c > @@ -268,6 +268,237 @@ static void __init test_kstrtoll_ok(void) > TEST_OK(kstrtoll, long long, "%lld", test_ll_ok); > } > > +/* > + * The special pattern to make sure the result is not modified for error cases. > + */ > +#define ULL_PATTERN (0xefefefef7a7a7a7aULL) > +#if BITS_PER_LONG == 32 > +#define POINTER_PATTERN (0xefef7a7a7aUL) > +#else > +#define POINTER_PATTERN (ULL_PATTERN) > +#endif > + > +/* Want to include "E" suffix for full coverage. */ > +#define MEMPARSE_TEST_SUFFIX (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\ > + MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\ > + MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E) > + > +static void __init test_memparse_safe_fail(void) > +{ > + struct memparse_test_fail { > + const char *str; > + /* Expected error number, either -EINVAL or -ERANGE. */ > + unsigned int expected_ret; > + }; > + static const struct memparse_test_fail tests[] __initconst = { > + /* No valid string can be found at all. */ > + {"", -EINVAL}, > + {"\n", -EINVAL}, > + {"\n0", -EINVAL}, > + {"+", -EINVAL}, > + {"-", -EINVAL}, > + > + /* Only hex prefix, but no valid string. */ > + {"0x", -EINVAL}, > + {"0X", -EINVAL}, > + > + /* Only hex prefix, with suffix but still no valid string. */ > + {"0xK", -EINVAL}, > + {"0xM", -EINVAL}, > + {"0xG", -EINVAL}, > + > + /* Only hex prefix, with invalid chars. */ > + {"0xH", -EINVAL}, > + {"0xy", -EINVAL}, > + > + /* > + * No support for any leading "+-" chars, even followed by a valid > + * number. > + */ > + {"-0", -EINVAL}, > + {"+0", -EINVAL}, > + {"-1", -EINVAL}, > + {"+1", -EINVAL}, > + > + /* Stray suffix would also be rejected. */ > + {"K", -EINVAL}, > + {"P", -EINVAL}, > + > + /* Overflow in the string itself*/ > + {"18446744073709551616", -ERANGE}, > + {"02000000000000000000000", -ERANGE}, > + {"0x10000000000000000", -ERANGE}, nit: ^ whitespace damage > + > + /* > + * Good string but would overflow with suffix. > + * > + * Note, for "E" suffix, one should not use with hex, or "0x1E" > + * would be treated as 0x1e (30 in decimal), not 0x1 and "E" suffix. > + * Another reason "E" suffix is cursed. > + */ > + {"16E", -ERANGE}, > + {"020E", -ERANGE}, > + {"16384P", -ERANGE}, > + {"040000P", -ERANGE}, > + {"16777216T", -ERANGE}, > + {"0100000000T", -ERANGE}, > + {"17179869184G", -ERANGE}, > + {"0200000000000G", -ERANGE}, > + {"17592186044416M", -ERANGE}, > + {"0400000000000000M", -ERANGE}, > + {"18014398509481984K", -ERANGE}, > + {"01000000000000000000K", -ERANGE}, > + }; > + unsigned int i; > + > + for_each_test(i, tests) { > + const struct memparse_test_fail *t = &tests[i]; > + unsigned long long tmp = ULL_PATTERN; > + char *retptr = (char *)POINTER_PATTERN; > + int ret; > + > + ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr); > + if (ret != t->expected_ret) { > + WARN(1, "str '%s', expected ret %d got %d\n", t->str, > + t->expected_ret, ret); > + continue; > + } > + if (tmp != ULL_PATTERN) > + WARN(1, "str '%s' failed as expected, but result got modified", > + t->str); > + if (retptr != (char *)POINTER_PATTERN) > + WARN(1, "str '%s' failed as expected, but pointer got modified", > + t->str); > + } > +} > + > +static void __init test_memparse_safe_ok(void) > +{ > + struct memparse_test_ok { > + const char *str; > + unsigned long long expected_value; > + /* How many bytes the @retptr pointer should be moved forward. */ > + unsigned int retptr_off; > + }; > + static DEFINE_TEST_OK(struct memparse_test_ok, tests) = { > + /* > + * The same pattern of kstrtoull, just with extra @retptr > + * verification. > + */ > + {"0", 0ULL, 1}, > + {"1", 1ULL, 1}, > + {"127", 127ULL, 3}, > + {"128", 128ULL, 3}, > + {"129", 129ULL, 3}, > + {"255", 255ULL, 3}, > + {"256", 256ULL, 3}, > + {"257", 257ULL, 3}, > + {"32767", 32767ULL, 5}, > + {"32768", 32768ULL, 5}, > + {"32769", 32769ULL, 5}, > + {"65535", 65535ULL, 5}, > + {"65536", 65536ULL, 5}, > + {"65537", 65537ULL, 5}, > + {"2147483647", 2147483647ULL, 10}, > + {"2147483648", 2147483648ULL, 10}, > + {"2147483649", 2147483649ULL, 10}, > + {"4294967295", 4294967295ULL, 10}, > + {"4294967296", 4294967296ULL, 10}, > + {"4294967297", 4294967297ULL, 10}, > + {"9223372036854775807", 9223372036854775807ULL, 19}, > + {"9223372036854775808", 9223372036854775808ULL, 19}, > + {"9223372036854775809", 9223372036854775809ULL, 19}, > + {"18446744073709551614", 18446744073709551614ULL, 20}, > + {"18446744073709551615", 18446744073709551615ULL, 20}, > + > + {"00", 00ULL, 2}, > + {"01", 01ULL, 2}, > + {"0177", 0177ULL, 4}, > + {"0200", 0200ULL, 4}, > + {"0201", 0201ULL, 4}, > + {"0377", 0377ULL, 4}, > + {"0400", 0400ULL, 4}, > + {"0401", 0401ULL, 4}, > + {"077777", 077777ULL, 6}, > + {"0100000", 0100000ULL, 7}, > + {"0100001", 0100001ULL, 7}, > + {"0177777", 0177777ULL, 7}, > + {"0200000", 0200000ULL, 7}, > + {"0200001", 0200001ULL, 7}, > + {"017777777777", 017777777777ULL, 12}, > + {"020000000000", 020000000000ULL, 12}, > + {"020000000001", 020000000001ULL, 12}, > + {"037777777777", 037777777777ULL, 12}, > + {"040000000000", 040000000000ULL, 12}, > + {"040000000001", 040000000001ULL, 12}, > + {"0777777777777777777777", 0777777777777777777777ULL, 22}, > + {"01000000000000000000000", 01000000000000000000000ULL, 23}, > + {"01000000000000000000001", 01000000000000000000001ULL, 23}, > + {"01777777777777777777776", 01777777777777777777776ULL, 23}, > + {"01777777777777777777777", 01777777777777777777777ULL, 23}, > + > + {"0x0", 0x0ULL, 3}, > + {"0x1", 0x1ULL, 3}, > + {"0x7f", 0x7fULL, 4}, > + {"0x80", 0x80ULL, 4}, > + {"0x81", 0x81ULL, 4}, > + {"0xff", 0xffULL, 4}, > + {"0x100", 0x100ULL, 5}, > + {"0x101", 0x101ULL, 5}, > + {"0x7fff", 0x7fffULL, 6}, > + {"0x8000", 0x8000ULL, 6}, > + {"0x8001", 0x8001ULL, 6}, > + {"0xffff", 0xffffULL, 6}, > + {"0x10000", 0x10000ULL, 7}, > + {"0x10001", 0x10001ULL, 7}, > + {"0x7fffffff", 0x7fffffffULL, 10}, > + {"0x80000000", 0x80000000ULL, 10}, > + {"0x80000001", 0x80000001ULL, 10}, > + {"0xffffffff", 0xffffffffULL, 10}, > + {"0x100000000", 0x100000000ULL, 11}, > + {"0x100000001", 0x100000001ULL, 11}, > + {"0x7fffffffffffffff", 0x7fffffffffffffffULL, 18}, > + {"0x8000000000000000", 0x8000000000000000ULL, 18}, > + {"0x8000000000000001", 0x8000000000000001ULL, 18}, > + {"0xfffffffffffffffe", 0xfffffffffffffffeULL, 18}, > + {"0xffffffffffffffff", 0xffffffffffffffffULL, 18}, > + > + /* Now with extra non-suffix chars to test @retptr update. */ > + {"1q84", 1, 1}, > + {"02o45", 2, 2}, > + {"0xffvii", 0xff, 4}, > + > + /* > + * Finally one suffix then tailing chars, to test the @retptr > + * behavior. > + */ > + {"68k ", 69632, 3}, > + {"8MS", 8388608, 2}, > + {"0xaeGis", 0x2b80000000, 5}, > + {"0xaTx", 0xa0000000000, 4}, > + {"3E8", 0x3000000000000000, 2}, In future it'd be good to get some coverage for non-MEMPARSE_TEST_SUFFIX use cases, e.g.: /* supported suffix, but not provided with @suffixes */ {"7K", (MEMPARSE_SUFFIX_M |\ MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\ MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E), 7, 1}, > + }; > + unsigned int i; > + > + for_each_test(i, tests) { > + const struct memparse_test_ok *t = &tests[i]; > + unsigned long long tmp; > + char *retptr; > + int ret; > + > + ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr); > + if (ret != 0) { > + WARN(1, "str '%s', expected ret 0 got %d\n", t->str, ret); > + continue; > + } > + if (tmp != t->expected_value) > + WARN(1, "str '%s' incorrect result, expected %llu got %llu", > + t->str, t->expected_value, tmp); > + if (retptr != t->str + t->retptr_off) > + WARN(1, "str '%s' incorrect endptr, expected %u got %zu", > + t->str, t->retptr_off, retptr - t->str); > + } > +} > static void __init test_kstrtoll_fail(void) > { > static DEFINE_TEST_FAIL(test_ll_fail) = { > @@ -710,6 +941,10 @@ static int __init test_kstrtox_init(void) > test_kstrtoll_ok(); > test_kstrtoll_fail(); > > + test_memparse_safe_ok(); > + test_memparse_safe_fail(); > + > + nit: whitespace ^ > test_kstrtou64_ok(); > test_kstrtou64_fail(); > test_kstrtos64_ok(); With Geert's comments addressed: Reviewed-by: David Disseldorp <ddiss@suse.de>
On 2024/1/2 23:53, Geert Uytterhoeven wrote: > Hi Qu, > > On Tue, Jan 2, 2024 at 5:13 AM Qu Wenruo <wqu@suse.com> wrote: >> The new tests cases for memparse_safe() include: >> >> - The existing test cases for kstrtoull() >> Including all the 3 bases (8, 10, 16), and all the ok and failure >> cases. >> Although there are something we need to verify specific for >> memparse_safe(): >> >> * @retptr and @value are not modified for failure cases >> >> * return value are correct for failure cases >> >> * @retptr is correct for the good cases >> >> - New test cases >> Not only testing the result value, but also the @retptr, including: >> >> * good cases with extra tailing chars, but without valid prefix >> The @retptr should point to the first char after a valid string. >> 3 cases for all the 3 bases. >> >> * good cases with extra tailing chars, with valid prefix >> 5 cases for all the suffixes. >> >> * bad cases without any number but stray suffix >> Should be rejected with -EINVAL >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > Thanks for your patch! > >> --- a/lib/test-kstrtox.c >> +++ b/lib/test-kstrtox.c >> @@ -268,6 +268,237 @@ static void __init test_kstrtoll_ok(void) >> TEST_OK(kstrtoll, long long, "%lld", test_ll_ok); >> } >> >> +/* >> + * The special pattern to make sure the result is not modified for error cases. >> + */ >> +#define ULL_PATTERN (0xefefefef7a7a7a7aULL) >> +#if BITS_PER_LONG == 32 >> +#define POINTER_PATTERN (0xefef7a7a7aUL) > > This pattern needs 40 bits to fit, so it doesn't fit in a 32-bit > unsigned long or pointer. Probably you wanted to use 0xef7a7a7aUL > instead? My bad, one extra byte... > >> +#else >> +#define POINTER_PATTERN (ULL_PATTERN) >> +#endif > > Shouldn't a simple cast to uintptr_t work fine for both 32-bit and > 64-bit systems: > > #define POINTER_PATTERN ((uintptr_t)ULL_PATTERN) > > Or even better, incorporate the cast to a pointer: > > #define POINTER_PATTERN ((void *)(uintptr_t)ULL_PATTERN) The problem is reported by sparse, which warns about that ULL_PATTERN converted to a pointer would lose its width: lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from constant value (efefefef7a7a7a7a becomes 7a7a7a7a) I'm not sure if using uiintptr_t would solve it, thus I go the macro to switch the value to avoid the static checker's warning. I tried to check how other locations handles patterned pointer value, like CONFIG_INIT_STACK_ALL_PATTERN, but they're either relying on the compiler or just memset(). Any better idea to solve the problem in a better way? Thanks, Qu > > so you can drop the extra cast when assigning/comparing retptr below. > >> + >> +/* Want to include "E" suffix for full coverage. */ >> +#define MEMPARSE_TEST_SUFFIX (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\ >> + MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\ >> + MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E) >> + >> +static void __init test_memparse_safe_fail(void) >> +{ > > [...] > >> + for_each_test(i, tests) { >> + const struct memparse_test_fail *t = &tests[i]; >> + unsigned long long tmp = ULL_PATTERN; >> + char *retptr = (char *)POINTER_PATTERN; >> + int ret; > > [...] > > + if (retptr != (char *)POINTER_PATTERN) > > Gr{oetje,eeting}s, > > Geert >
Hi Qu, On Tue, Jan 2, 2024 at 9:56 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > On 2024/1/2 23:53, Geert Uytterhoeven wrote: > > On Tue, Jan 2, 2024 at 5:13 AM Qu Wenruo <wqu@suse.com> wrote: > >> The new tests cases for memparse_safe() include: > >> > >> - The existing test cases for kstrtoull() > >> Including all the 3 bases (8, 10, 16), and all the ok and failure > >> cases. > >> Although there are something we need to verify specific for > >> memparse_safe(): > >> > >> * @retptr and @value are not modified for failure cases > >> > >> * return value are correct for failure cases > >> > >> * @retptr is correct for the good cases > >> > >> - New test cases > >> Not only testing the result value, but also the @retptr, including: > >> > >> * good cases with extra tailing chars, but without valid prefix > >> The @retptr should point to the first char after a valid string. > >> 3 cases for all the 3 bases. > >> > >> * good cases with extra tailing chars, with valid prefix > >> 5 cases for all the suffixes. > >> > >> * bad cases without any number but stray suffix > >> Should be rejected with -EINVAL > >> > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > > > Thanks for your patch! > > > >> --- a/lib/test-kstrtox.c > >> +++ b/lib/test-kstrtox.c > >> @@ -268,6 +268,237 @@ static void __init test_kstrtoll_ok(void) > >> TEST_OK(kstrtoll, long long, "%lld", test_ll_ok); > >> } > >> > >> +/* > >> + * The special pattern to make sure the result is not modified for error cases. > >> + */ > >> +#define ULL_PATTERN (0xefefefef7a7a7a7aULL) > >> +#if BITS_PER_LONG == 32 > >> +#define POINTER_PATTERN (0xefef7a7a7aUL) > > > > This pattern needs 40 bits to fit, so it doesn't fit in a 32-bit > > unsigned long or pointer. Probably you wanted to use 0xef7a7a7aUL > > instead? > > My bad, one extra byte... So did that fix the sparse warning? ;-) > >> +#else > >> +#define POINTER_PATTERN (ULL_PATTERN) > >> +#endif > > > > Shouldn't a simple cast to uintptr_t work fine for both 32-bit and > > 64-bit systems: > > > > #define POINTER_PATTERN ((uintptr_t)ULL_PATTERN) > > > > Or even better, incorporate the cast to a pointer: > > > > #define POINTER_PATTERN ((void *)(uintptr_t)ULL_PATTERN) > > The problem is reported by sparse, which warns about that ULL_PATTERN > converted to a pointer would lose its width: > > lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from > constant value (efefefef7a7a7a7a becomes 7a7a7a7a) Ah yes, sparse can be annoying. I'm still looking for a clean and concise way to shut up [1]. > I'm not sure if using uiintptr_t would solve it, thus I go the macro to > switch the value to avoid the static checker's warning. > > I tried to check how other locations handles patterned pointer value, > like CONFIG_INIT_STACK_ALL_PATTERN, but they're either relying on the > compiler or just memset(). > > Any better idea to solve the problem in a better way? Masking off the extra bits, like lower_32_bits()[2] does? #define POINTER_PATTERN ((void *)(uintptr_t)((ULL_PATTERN) & UINTPTR_MAX)) [1] https://lore.kernel.org/oe-kbuild-all/202312181649.u6k6hLIm-lkp@intel.com/ [2] https://elixir.bootlin.com/linux/latest/source/include/linux/kernel.h#L82 Gr{oetje,eeting}s, Geert
On 2024/1/3 19:57, Geert Uytterhoeven wrote: > Hi Qu, > > On Tue, Jan 2, 2024 at 9:56 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >> On 2024/1/2 23:53, Geert Uytterhoeven wrote: >>> On Tue, Jan 2, 2024 at 5:13 AM Qu Wenruo <wqu@suse.com> wrote: >>>> The new tests cases for memparse_safe() include: >>>> >>>> - The existing test cases for kstrtoull() >>>> Including all the 3 bases (8, 10, 16), and all the ok and failure >>>> cases. >>>> Although there are something we need to verify specific for >>>> memparse_safe(): >>>> >>>> * @retptr and @value are not modified for failure cases >>>> >>>> * return value are correct for failure cases >>>> >>>> * @retptr is correct for the good cases >>>> >>>> - New test cases >>>> Not only testing the result value, but also the @retptr, including: >>>> >>>> * good cases with extra tailing chars, but without valid prefix >>>> The @retptr should point to the first char after a valid string. >>>> 3 cases for all the 3 bases. >>>> >>>> * good cases with extra tailing chars, with valid prefix >>>> 5 cases for all the suffixes. >>>> >>>> * bad cases without any number but stray suffix >>>> Should be rejected with -EINVAL >>>> >>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> >>> Thanks for your patch! >>> >>>> --- a/lib/test-kstrtox.c >>>> +++ b/lib/test-kstrtox.c >>>> @@ -268,6 +268,237 @@ static void __init test_kstrtoll_ok(void) >>>> TEST_OK(kstrtoll, long long, "%lld", test_ll_ok); >>>> } >>>> >>>> +/* >>>> + * The special pattern to make sure the result is not modified for error cases. >>>> + */ >>>> +#define ULL_PATTERN (0xefefefef7a7a7a7aULL) >>>> +#if BITS_PER_LONG == 32 >>>> +#define POINTER_PATTERN (0xefef7a7a7aUL) >>> >>> This pattern needs 40 bits to fit, so it doesn't fit in a 32-bit >>> unsigned long or pointer. Probably you wanted to use 0xef7a7a7aUL >>> instead? >> >> My bad, one extra byte... > > So did that fix the sparse warning? ;-) Intel guys have already masked this particular warning. But your newer suggestion is much better. > >>>> +#else >>>> +#define POINTER_PATTERN (ULL_PATTERN) >>>> +#endif >>> >>> Shouldn't a simple cast to uintptr_t work fine for both 32-bit and >>> 64-bit systems: >>> >>> #define POINTER_PATTERN ((uintptr_t)ULL_PATTERN) >>> >>> Or even better, incorporate the cast to a pointer: >>> >>> #define POINTER_PATTERN ((void *)(uintptr_t)ULL_PATTERN) >> >> The problem is reported by sparse, which warns about that ULL_PATTERN >> converted to a pointer would lose its width: >> >> lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from >> constant value (efefefef7a7a7a7a becomes 7a7a7a7a) > > Ah yes, sparse can be annoying. > I'm still looking for a clean and concise way to shut up [1]. > >> I'm not sure if using uiintptr_t would solve it, thus I go the macro to >> switch the value to avoid the static checker's warning. >> >> I tried to check how other locations handles patterned pointer value, >> like CONFIG_INIT_STACK_ALL_PATTERN, but they're either relying on the >> compiler or just memset(). >> >> Any better idea to solve the problem in a better way? > > Masking off the extra bits, like lower_32_bits()[2] does? > > #define POINTER_PATTERN ((void *)(uintptr_t)((ULL_PATTERN) & UINTPTR_MAX)) This sounds much better to me. I would go this path instead, and finally no need to manually count how many bytes (which I already failed once). Thanks, Qu > > [1] https://lore.kernel.org/oe-kbuild-all/202312181649.u6k6hLIm-lkp@intel.com/ > [2] https://elixir.bootlin.com/linux/latest/source/include/linux/kernel.h#L82 > > Gr{oetje,eeting}s, > > Geert >
On 2024/1/3 00:47, David Disseldorp wrote: > On Tue, 2 Jan 2024 14:42:13 +1030, Qu Wenruo wrote: > [...] >> + {"P", -EINVAL}, >> + >> + /* Overflow in the string itself*/ >> + {"18446744073709551616", -ERANGE}, >> + {"02000000000000000000000", -ERANGE}, >> + {"0x10000000000000000", -ERANGE}, > nit: ^ whitespace damage Sorry, I didn't get the point here. I checked the patch it's a single space. Or I missed/screwed up something? >> + >> + /* [...] >> + >> + /* >> + * Finally one suffix then tailing chars, to test the @retptr >> + * behavior. >> + */ >> + {"68k ", 69632, 3}, >> + {"8MS", 8388608, 2}, >> + {"0xaeGis", 0x2b80000000, 5}, >> + {"0xaTx", 0xa0000000000, 4}, >> + {"3E8", 0x3000000000000000, 2}, > > In future it'd be good to get some coverage for non-MEMPARSE_TEST_SUFFIX > use cases, e.g.: > /* supported suffix, but not provided with @suffixes */ > {"7K", (MEMPARSE_SUFFIX_M |\ > MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\ > MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E), 7, 1}, That's a great idea, since I'm still prepare a v3, it's not hard to add it into v3. Thanks, Qu > >> + }; >> + unsigned int i; >> + >> + for_each_test(i, tests) { >> + const struct memparse_test_ok *t = &tests[i]; >> + unsigned long long tmp; >> + char *retptr; >> + int ret; >> + >> + ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr); >> + if (ret != 0) { >> + WARN(1, "str '%s', expected ret 0 got %d\n", t->str, ret); >> + continue; >> + } >> + if (tmp != t->expected_value) >> + WARN(1, "str '%s' incorrect result, expected %llu got %llu", >> + t->str, t->expected_value, tmp); >> + if (retptr != t->str + t->retptr_off) >> + WARN(1, "str '%s' incorrect endptr, expected %u got %zu", >> + t->str, t->retptr_off, retptr - t->str); >> + } >> +} >> static void __init test_kstrtoll_fail(void) >> { >> static DEFINE_TEST_FAIL(test_ll_fail) = { >> @@ -710,6 +941,10 @@ static int __init test_kstrtox_init(void) >> test_kstrtoll_ok(); >> test_kstrtoll_fail(); >> >> + test_memparse_safe_ok(); >> + test_memparse_safe_fail(); >> + >> + > nit: whitespace ^ > >> test_kstrtou64_ok(); >> test_kstrtou64_fail(); >> test_kstrtos64_ok(); > > With Geert's comments addressed: > Reviewed-by: David Disseldorp <ddiss@suse.de> >
diff --git a/lib/test-kstrtox.c b/lib/test-kstrtox.c index f355f67169b6..97c2f65a16cb 100644 --- a/lib/test-kstrtox.c +++ b/lib/test-kstrtox.c @@ -268,6 +268,237 @@ static void __init test_kstrtoll_ok(void) TEST_OK(kstrtoll, long long, "%lld", test_ll_ok); } +/* + * The special pattern to make sure the result is not modified for error cases. + */ +#define ULL_PATTERN (0xefefefef7a7a7a7aULL) +#if BITS_PER_LONG == 32 +#define POINTER_PATTERN (0xefef7a7a7aUL) +#else +#define POINTER_PATTERN (ULL_PATTERN) +#endif + +/* Want to include "E" suffix for full coverage. */ +#define MEMPARSE_TEST_SUFFIX (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\ + MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\ + MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E) + +static void __init test_memparse_safe_fail(void) +{ + struct memparse_test_fail { + const char *str; + /* Expected error number, either -EINVAL or -ERANGE. */ + unsigned int expected_ret; + }; + static const struct memparse_test_fail tests[] __initconst = { + /* No valid string can be found at all. */ + {"", -EINVAL}, + {"\n", -EINVAL}, + {"\n0", -EINVAL}, + {"+", -EINVAL}, + {"-", -EINVAL}, + + /* Only hex prefix, but no valid string. */ + {"0x", -EINVAL}, + {"0X", -EINVAL}, + + /* Only hex prefix, with suffix but still no valid string. */ + {"0xK", -EINVAL}, + {"0xM", -EINVAL}, + {"0xG", -EINVAL}, + + /* Only hex prefix, with invalid chars. */ + {"0xH", -EINVAL}, + {"0xy", -EINVAL}, + + /* + * No support for any leading "+-" chars, even followed by a valid + * number. + */ + {"-0", -EINVAL}, + {"+0", -EINVAL}, + {"-1", -EINVAL}, + {"+1", -EINVAL}, + + /* Stray suffix would also be rejected. */ + {"K", -EINVAL}, + {"P", -EINVAL}, + + /* Overflow in the string itself*/ + {"18446744073709551616", -ERANGE}, + {"02000000000000000000000", -ERANGE}, + {"0x10000000000000000", -ERANGE}, + + /* + * Good string but would overflow with suffix. + * + * Note, for "E" suffix, one should not use with hex, or "0x1E" + * would be treated as 0x1e (30 in decimal), not 0x1 and "E" suffix. + * Another reason "E" suffix is cursed. + */ + {"16E", -ERANGE}, + {"020E", -ERANGE}, + {"16384P", -ERANGE}, + {"040000P", -ERANGE}, + {"16777216T", -ERANGE}, + {"0100000000T", -ERANGE}, + {"17179869184G", -ERANGE}, + {"0200000000000G", -ERANGE}, + {"17592186044416M", -ERANGE}, + {"0400000000000000M", -ERANGE}, + {"18014398509481984K", -ERANGE}, + {"01000000000000000000K", -ERANGE}, + }; + unsigned int i; + + for_each_test(i, tests) { + const struct memparse_test_fail *t = &tests[i]; + unsigned long long tmp = ULL_PATTERN; + char *retptr = (char *)POINTER_PATTERN; + int ret; + + ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr); + if (ret != t->expected_ret) { + WARN(1, "str '%s', expected ret %d got %d\n", t->str, + t->expected_ret, ret); + continue; + } + if (tmp != ULL_PATTERN) + WARN(1, "str '%s' failed as expected, but result got modified", + t->str); + if (retptr != (char *)POINTER_PATTERN) + WARN(1, "str '%s' failed as expected, but pointer got modified", + t->str); + } +} + +static void __init test_memparse_safe_ok(void) +{ + struct memparse_test_ok { + const char *str; + unsigned long long expected_value; + /* How many bytes the @retptr pointer should be moved forward. */ + unsigned int retptr_off; + }; + static DEFINE_TEST_OK(struct memparse_test_ok, tests) = { + /* + * The same pattern of kstrtoull, just with extra @retptr + * verification. + */ + {"0", 0ULL, 1}, + {"1", 1ULL, 1}, + {"127", 127ULL, 3}, + {"128", 128ULL, 3}, + {"129", 129ULL, 3}, + {"255", 255ULL, 3}, + {"256", 256ULL, 3}, + {"257", 257ULL, 3}, + {"32767", 32767ULL, 5}, + {"32768", 32768ULL, 5}, + {"32769", 32769ULL, 5}, + {"65535", 65535ULL, 5}, + {"65536", 65536ULL, 5}, + {"65537", 65537ULL, 5}, + {"2147483647", 2147483647ULL, 10}, + {"2147483648", 2147483648ULL, 10}, + {"2147483649", 2147483649ULL, 10}, + {"4294967295", 4294967295ULL, 10}, + {"4294967296", 4294967296ULL, 10}, + {"4294967297", 4294967297ULL, 10}, + {"9223372036854775807", 9223372036854775807ULL, 19}, + {"9223372036854775808", 9223372036854775808ULL, 19}, + {"9223372036854775809", 9223372036854775809ULL, 19}, + {"18446744073709551614", 18446744073709551614ULL, 20}, + {"18446744073709551615", 18446744073709551615ULL, 20}, + + {"00", 00ULL, 2}, + {"01", 01ULL, 2}, + {"0177", 0177ULL, 4}, + {"0200", 0200ULL, 4}, + {"0201", 0201ULL, 4}, + {"0377", 0377ULL, 4}, + {"0400", 0400ULL, 4}, + {"0401", 0401ULL, 4}, + {"077777", 077777ULL, 6}, + {"0100000", 0100000ULL, 7}, + {"0100001", 0100001ULL, 7}, + {"0177777", 0177777ULL, 7}, + {"0200000", 0200000ULL, 7}, + {"0200001", 0200001ULL, 7}, + {"017777777777", 017777777777ULL, 12}, + {"020000000000", 020000000000ULL, 12}, + {"020000000001", 020000000001ULL, 12}, + {"037777777777", 037777777777ULL, 12}, + {"040000000000", 040000000000ULL, 12}, + {"040000000001", 040000000001ULL, 12}, + {"0777777777777777777777", 0777777777777777777777ULL, 22}, + {"01000000000000000000000", 01000000000000000000000ULL, 23}, + {"01000000000000000000001", 01000000000000000000001ULL, 23}, + {"01777777777777777777776", 01777777777777777777776ULL, 23}, + {"01777777777777777777777", 01777777777777777777777ULL, 23}, + + {"0x0", 0x0ULL, 3}, + {"0x1", 0x1ULL, 3}, + {"0x7f", 0x7fULL, 4}, + {"0x80", 0x80ULL, 4}, + {"0x81", 0x81ULL, 4}, + {"0xff", 0xffULL, 4}, + {"0x100", 0x100ULL, 5}, + {"0x101", 0x101ULL, 5}, + {"0x7fff", 0x7fffULL, 6}, + {"0x8000", 0x8000ULL, 6}, + {"0x8001", 0x8001ULL, 6}, + {"0xffff", 0xffffULL, 6}, + {"0x10000", 0x10000ULL, 7}, + {"0x10001", 0x10001ULL, 7}, + {"0x7fffffff", 0x7fffffffULL, 10}, + {"0x80000000", 0x80000000ULL, 10}, + {"0x80000001", 0x80000001ULL, 10}, + {"0xffffffff", 0xffffffffULL, 10}, + {"0x100000000", 0x100000000ULL, 11}, + {"0x100000001", 0x100000001ULL, 11}, + {"0x7fffffffffffffff", 0x7fffffffffffffffULL, 18}, + {"0x8000000000000000", 0x8000000000000000ULL, 18}, + {"0x8000000000000001", 0x8000000000000001ULL, 18}, + {"0xfffffffffffffffe", 0xfffffffffffffffeULL, 18}, + {"0xffffffffffffffff", 0xffffffffffffffffULL, 18}, + + /* Now with extra non-suffix chars to test @retptr update. */ + {"1q84", 1, 1}, + {"02o45", 2, 2}, + {"0xffvii", 0xff, 4}, + + /* + * Finally one suffix then tailing chars, to test the @retptr + * behavior. + */ + {"68k ", 69632, 3}, + {"8MS", 8388608, 2}, + {"0xaeGis", 0x2b80000000, 5}, + {"0xaTx", 0xa0000000000, 4}, + {"3E8", 0x3000000000000000, 2}, + }; + unsigned int i; + + for_each_test(i, tests) { + const struct memparse_test_ok *t = &tests[i]; + unsigned long long tmp; + char *retptr; + int ret; + + ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr); + if (ret != 0) { + WARN(1, "str '%s', expected ret 0 got %d\n", t->str, ret); + continue; + } + if (tmp != t->expected_value) + WARN(1, "str '%s' incorrect result, expected %llu got %llu", + t->str, t->expected_value, tmp); + if (retptr != t->str + t->retptr_off) + WARN(1, "str '%s' incorrect endptr, expected %u got %zu", + t->str, t->retptr_off, retptr - t->str); + } +} static void __init test_kstrtoll_fail(void) { static DEFINE_TEST_FAIL(test_ll_fail) = { @@ -710,6 +941,10 @@ static int __init test_kstrtox_init(void) test_kstrtoll_ok(); test_kstrtoll_fail(); + test_memparse_safe_ok(); + test_memparse_safe_fail(); + + test_kstrtou64_ok(); test_kstrtou64_fail(); test_kstrtos64_ok();
The new tests cases for memparse_safe() include: - The existing test cases for kstrtoull() Including all the 3 bases (8, 10, 16), and all the ok and failure cases. Although there are something we need to verify specific for memparse_safe(): * @retptr and @value are not modified for failure cases * return value are correct for failure cases * @retptr is correct for the good cases - New test cases Not only testing the result value, but also the @retptr, including: * good cases with extra tailing chars, but without valid prefix The @retptr should point to the first char after a valid string. 3 cases for all the 3 bases. * good cases with extra tailing chars, with valid prefix 5 cases for all the suffixes. * bad cases without any number but stray suffix Should be rejected with -EINVAL Signed-off-by: Qu Wenruo <wqu@suse.com> --- lib/test-kstrtox.c | 235 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 235 insertions(+)