Message-ID: <d91d9c20-3b17-40e4-96d0-49daecf6558e@infradead.org>
Date: Thu, 18 Jan 2024 09:52:33 -0800
From: Randy Dunlap <rdunlap@...radead.org>
To: Qu Wenruo <quwenruo.btrfs@....com>, Qu Wenruo <wqu@...e.com>,
 linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
 akpm@...ux-foundation.org, christophe.jaillet@...adoo.fr,
 andriy.shevchenko@...ux.intel.com, David.Laight@...LAB.COM, ddiss@...e.de,
 geert@...ux-m68k.org
Subject: Re: [PATCH v4 2/4] kstrtox: introduce a safer version of memparse()

Hi,

On 1/14/24 21:27, Qu Wenruo wrote:
> 
> 
> On 2024/1/15 14:57, Randy Dunlap wrote:
> [...]
>>> @@ -113,6 +113,105 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>>>       return 0;
>>>   }
>>>
>>> +/**
>>> + * memparse_safe - convert a string to an unsigned long long, safer version of
>>> + * memparse()
>>> + *
>>> + * @s:        The start of the string. Must be null-terminated.
>>
>> Unless I misunderstand, this is the biggest problem that I see with
>> memparse_safe(): "Must be null-terminated".
>> memparse() does not have that requirement.
> 
> This is just an extra safety requirement.
> 
> In reality, memparse_safe() would end at the either the first
> unsupported suffix after the valid numeric string (including '\0'),
> or won't be updated if any error is hit (either no valid string at all,
> or some overflow happened).
> 
> For most if not all call sites, the string passed in is already
> null-terminated.
> 
>>
>> And how is @retptr updated if the string is null-terminated?
> 
> E.g "123456G\0", in this case if suffix "G" is allowed, then @retptr
> would be updated to '\0'.
> 
> Or another example "123456\0", @retptr would still be updated to '\0'.
> 
>>
>> If the "Must be null-terminated." is correct, it requires that every user/caller
>> first determine the end of the number (how? space and/or any special character
>> or any alphabetic character that is not in KMGTPE? Then save that ending char,
>> change it to NUL, call memparse_safe(), then restore the saved char?
> 
> There are already test cases like "86k \0" (note all strings in the test
> case is all null terminated), which would lead to a success parse, with
> @retptr updated to ' ' (if suffix K is specified) or 'k' (if suffix K is
> not specified).
> 
> So the behavior is still the same.
> It may be my expression too confusing.
> 
> Any recommendation for the comments?

Well, "Must be null-terminated." is incorrect, so explain better where
the numeric conversion ends.

Thanks.

> 
> Thanks,
> Qu
> 
>>
>> I'm hoping that the documentation is not correct...
>>
>>> + *        The base is determined automatically, if it starts with "0x"
>>> + *        the base is 16, if it starts with "0" the base is 8, otherwise
>>> + *        the base is 10.
>>> + *        After a valid number string, there can be at most one
>>> + *        case-insensitive suffix character, specified by the @suffixes
>>> + *        parameter.
>>> + *
>>> + * @suffixes:    The suffixes which should be handled. Use logical ORed
>>> + *        memparse_suffix enum to indicate the supported suffixes.
>>> + *        The suffixes are case-insensitive, all 2 ^ 10 based.
>>> + *        Supported ones are "KMGPTE".
>>> + *        If one suffix (one of "KMGPTE") is hit but that suffix is
>>> + *        not specified in the @suffxies parameter, it ends the parse
>>> + *        normally, with @retptr pointed to the (unsupported) suffix.
>>> + *        E.g. "68k" with suffxies "M" returns 68 decimal, @retptr
>>> + *        updated to 'k'.
>>> + *
>>> + * @res:    Where to write the result.
>>> + *
>>> + * @retptr:    (output) Optional pointer to the next char after parse completes.
>>> + *
>>> + * Returns:
>>> + * * %0 if any valid numeric string can be parsed, and @retptr is updated.
>>> + * * %-EINVAL if no valid number string can be found.
>>> + * * %-ERANGE if the number overflows.
>>> + * * For negative return values, @retptr is not updated.
>>> + */
>>> +noinline int memparse_safe(const char *s, enum memparse_suffix suffixes,
>>> +               unsigned long long *res, char **retptr)
>>> +{
>>
>> Thanks.
> 

-- 
#Randy

Powered by blists - more mailing lists