lkml.org 
[lkml]   [2021]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf
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

\
 
 \ /
  Last update: 2021-02-08 20:16    [W:0.058 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site