[v2,3/4] kstrtox: add unit tests for memparse_safe()
Commit Message
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(+)
Comments
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>
>
@@ -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();