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

[Petr Mladek]

On Thu 2021-02-11 13:55:26, Petr Mladek wrote:
> On Mon 2021-02-08 17:38:29, Richard Fitzgerald wrote:
> > On 08/02/2021 15:18, Andy Shevchenko wrote:
> > > On Mon, Feb 08, 2021 at 02:01:52PM +0000, Richard Fitzgerald wrote:
> > > A nit-pick: What if we rewrite above as
> > > 
> > > static unsigned long long simple_strntoull(const char *cp, size_t max_chars,
> > > 					   char **endp, unsigned int base)
> > > {
> > > 	unsigned long long result = 0ULL;
> > > 	const char *startp = cp;
> > > 	unsigned int rv;
> > > 	size_t chars;
> > > 
> > > 	cp = _parse_integer_fixup_radix(cp, &base);
> > > 	chars = cp - startp;
> > > 	if (chars >= max_chars) {
> > > 		/* We hit the limit */
> > > 		cp = startp + max_chars;
> > > 	} else {
> > > 		rv = _parse_integer_limit(cp, base, &result, max_chars - chars);
> > > 		/* FIXME */
> > > 		cp += (rv & ~KSTRTOX_OVERFLOW);
> > > 	}
> > > 
> > > 	if (endp)
> > > 		*endp = (char *)cp;
> > > 
> > > 	return result;
> > > }
> > > 
> > > ...
> > 
> > 
> > I don't mind rewriting that code if you prefer that way.
> > I am used to working on other kernel subsytems where the preference is
> > to bail out on the error case so that the "normal" case flows without
> > nesting.
> 
> Yeah. But in this case Andy's variant looks slightly better redable to me.


> ...
> 
> > > 
> > > > +			val.s = simple_strntoll(str,
> > > > +						field_width > 0 ? field_width : SIZE_MAX,
> > > > +						&next, base);
> > > 
> > > is? Also, is field_width == 0 should be treated as "parse to the MAX"?
> > 
> > Earlier code terminates scanning if the width parsed from the format
> > string is <= 0.
> 
> > So field_width can only be -1 or > 0 here. But now you
> > point it out, that test would be better as field_width >= 0 ... so
> > it deals with 0 if it ever happened to sneak through to here
> > somehow.
> 
> It might make sense to be proactive and change it to >= 0.
> But I would do it in a separate patch. The "< 0" condition
> matches the original code.

Ah, I have missed that you have already sent v6 where you did this change
in the same patch. There is no need to resend it just because of this.
I am going to look at v6.

Best Regards,
Petr