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

[Petr Mladek]

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:
> > > 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.
> > > 
> > > -unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base)
> > > +static unsigned long long simple_strntoull(const char *startp, size_t max_chars,
> > > +					   char **endp, unsigned int base)
> > >   {
> > > -	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);
> > > +	cp = _parse_integer_fixup_radix(startp, &base);
> > > +	if ((cp - startp) >= max_chars) {
> > > +		cp = startp + max_chars;
> > > +		goto out;
> > > +	}
> > > +
> > > +	max_chars -= (cp - startp);
> > > +	rv = _parse_integer_limit(cp, base, &result, max_chars);
> > >   	/* FIXME */
> > >   	cp += (rv & ~KSTRTOX_OVERFLOW);
> > > +out:
> > >   	if (endp)
> > >   		*endp = (char *)cp;
> > >   	return result;
> > >   }
> > 
> > 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);
> > 
> > A nit-pick: Wouldn't be negative field_width "big enough" to just being used as

 
> field_width is s16 so really should be sign-extended

I guess that Andy just missed that it was a signed type. And it has to be
because  -1 means SIZE_MAX.

> to make it "very
> big". I think this would be less readable what the intention is and what
> assumptions it is based on. There's a risk someone would look at
> 
> (size_t)(long)field_width
> 
> and think the (long) is redundant.
> Perhaps change field_width to int? There I ask myself "if it can be an
> int, why is it declared s16?" and worry there is something subtle in the
> code.
> 
> My personal preference is to avoid using tricks in code that isn't time
> critical.

I agree. Let's keep the check with signed type.

> > is? Also, is field_width == 0 should be treated as "parse to the MAX"?

filed_width == 0 actually means that no characters are read. I should
return zero value.

> > ...
> 
> Earlier code terminates scanning if the width parsed from the format
> string is <= 0.

To make it clear what earlier code means. vsscanf() bail out earlier
when field_width == 0. It is handled by this code:

		/* get field width */
		field_width = -1;
		if (isdigit(*fmt)) {
			field_width = skip_atoi(&fmt);
			if (field_width <= 0)
				break;
		}

> 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.

Best Regards,
Petr