Re: [PATCH v4 2/4] kstrtox: introduce a safer version of memparse()

[Qu Wenruo]

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?

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.