linux-kernel - Re: [PATCH v3 2/4] kstrtox: introduce a safer version of memparse()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1dbb1cd1-15fa-401f-8356-474c85a46f3b@gmx.com>
Date: Thu, 4 Jan 2024 17:25:50 +1030
From: Qu Wenruo <quwenruo.btrfs@....com>
To: Randy Dunlap <rdunlap@...radead.org>, 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 v3 2/4] kstrtox: introduce a safer version of memparse()



On 2024/1/4 17:20, Randy Dunlap wrote:
>
>
> On 1/3/24 22:42, Qu Wenruo wrote:
>>
>>
>> On 2024/1/4 17:00, Randy Dunlap wrote:
>>> Hi,
>>>
>> [...]
>>>> + *        parameter.
>>>> + *
>>>> + * @suffixes:    The suffixes which should be parsed. Use logical ORed
>>>> + *        memparse_suffix enum to indicate the supported suffixes.
>>>> + *        The suffixes are case-insensive, all 2 ^ 10 based.
>>>
>>>                           case-insensitive
>>>
>>>> + *        Supported ones are "KMGPTE".
>>>> + *        NOTE: If one suffix out of the supported one is hit, it would
>>>
>>>                                                   ones
>>>
>>>> + *        end the parse normally, with @retptr pointed to the unsupported
>>>> + *        suffix.
>>>
>>> Could you explain (or give an example) of "to the unsupported suffix"?
>>> This isn't clear IMO.
>>
>> Oh, my bad, that sentence itself is not correct.
>>
>> What I really want to say is:
>>
>>   If one suffix (one of "KMGPTE") is hit but that suffix is not
>>   specified in the @suffxies parameter, it would end the parse normally,
>>   with @retptr pointed to the (unsupported) suffix.
>
> (corrected ^^^)
>
>> The example would be the "68k " case in the ok cases in the next patch.
>> We have two different cases for the same "68k" string, with different
>> @suffixes and different results:
>>
>>      "68k ", KMGPTE -> 68 * 1024, @retptr at " ".
>>      "68k ", M -> 68, @retptr at 'k'.
>>
>> I don't have a better expression here unfortunately, maybe the special
>> case is not even worthy explaining?
>>
>
> No, the corrected paragraph above is good. (s/would end/ends/)
>
> Except for one thing: the examples here terminate on a space character,
> but the function kernel-doc says "null-terminated":

Oh, all the strings used here and in the test cases are all "\0" ended.
Just to make it a little more readable (and easier to type), the tailing
"\0" is not explicitly showed.

That extra ' ' in the "68k \0" case is just to make sure the @retptr is
properly updated.

For all the other existing ok cases for memparse_safe() (those without
extra tailing chars), the @retptr points to the tailing NUL '\0'.

Thanks,
Qu

>
> +/**
> + * memparse_safe - convert a string to an unsigned long long, safer version of
> + * memparse()
> + *
> + * @s:		The start of the string. Must be null-terminated.
>
>
>
>
>>>
>>>> + *
>>>> + * @res:    Where to write the result.
>>>> + *
>>>> + * @retptr:    (output) Optional pointer to the next char after parse completes.
>>>> + *
>>>> + * Return 0 if any valid numberic string can be parsed, and @retptr updated.
>>>> + * Return -INVALID if no valid number string can be found.
>>>> + * Return -ERANGE if the number overflows.
>>>> + * For minus return values, @retptr would not be updated.
>>>
>>>    * 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.
>>>
>>>
>>> For *ALL* of the comments, I request/suggest that you change the "would be" or
>>> "would not be" to "is" or "is not" or whatever present tense words make the
>>> most sense.
>>
>> No problem.
>
>