Re: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf

[Petr Mladek]

On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
> On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:
> > The existing code attempted to handle numbers by doing a strto[u]l(),
> > ignoring the field width, and then repeatedly dividing to extract the
> > field out of the full converted value. If the string contains a run of
> > valid digits longer than will fit in a long or long long, this would
> > overflow and no amount of dividing can recover the correct value.

> ...
> 
> > +	for (; max_chars > 0; max_chars--) {
> 
> Less fragile is to write
> 
> 	while (max_chars--)

Except that the original was more obvious at least for me.
I always prefer more readable code when the compiler might do
the optimization easily. But this is my personal taste.
I am fine with both variants.

> 
> This allows max_char to be an unsigned type.
> 
> Moreover...
> 
> > +	return _parse_integer_limit(s, base, p, INT_MAX);
> 
> You have inconsistency with INT_MAX vs, size_t above.

Ah, this was on my request. INT_MAX is already used on many other
locations in vsnprintf() for this purpose.

An alternative is to fix all the other locations. We would need to
check if it is really safe. Well, I do not want to force Richard
to fix this historical mess. He already put a lot lot of effort
into fixing this long term issue.

...

> > -	unsigned long long result;
> > +	const char *cp;
> > +	unsigned long long result = 0ULL;
> >  	unsigned int rv;
> >  
> > -	cp = _parse_integer_fixup_radix(cp, &base);
> > -	rv = _parse_integer(cp, base, &result);
> 
> > +	if (max_chars == 0) {
> > +		cp = startp;
> > +		goto out;
> > +	}
> 
> It's redundant if I'm not mistaken.

Also this is more obvious and less error prone from my POV.
But I agree that it is redundant. I am not sure if this
function is used in some fast paths.

Again I am fine with both variants.

> > +	cp = _parse_integer_fixup_radix(startp, &base);
> > +	if ((cp - startp) >= max_chars) {
> > +		cp = startp + max_chars;
> > +		goto out;
> > +	}
> 
> This will be exactly the same, no?

Best Regards,
Petr