On Fri 2021-02-05 14:50:56, Andy Shevchenko wrote:
On Fri, Feb 5, 2021 at 1:35 PM Richard Fitzgerald rf@opensource.cirrus.com wrote:
On 04/02/2021 16:35, Petr Mladek wrote:
On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote: 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.
I originally had UINT_MAX and changed on Petr's request to be consistent with other code. (Sorry Andy - my mistake not including you on the earlier review versions).
But 0 < INT_MAX < UINT_MAX, so ok to pass to an unsigned. And as Petr said on his original review, INT_MAX is "big enough".
Some code has INT_MAX, some has UINT_MAX, while the parameter is size_t.
Yeah, if I remember correctly I wanted to have INT_MAX everywhere but I did not want to nitpick about it in the later versions. It looked like an arbitrary number anyway.
I think all of these inconsistencies should have a comment either in the code, or in the commit message, or in the cover letter (depending on the importance). Or being fixed to be more consistent with existing code. Whichever you consider better.
OK, you made me to do some archaeology. The INT_MAX limit has been added into vsnprintf() in 2.6.2 by the commit:
Author: Linus Torvalds torvalds@home.osdl.org Date: Mon Feb 2 21:17:29 2004 -0800
Warn loudly if somebody passes a negative value as the size to "vsnprintf()".
That's a pretty clear case of overflow.
It might catch problems. And the limit seems to have worked all the time.
IMHO, it would make sense to have INT_MAX limit also in _parse_integer_limit() and WARN() when a larger value is passed.
By other words, it would mean to add this check and use INT_MAX everywhere in this patch.
Best Regards, Petr