| Message ID | 20230610204044.3653-4-demi@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make sscanf() stricter | expand |
On 10/06/2023 22.40, Demi Marie Obenour wrote: > Passing spaces before e.g. an integer is usually > not intended. Maybe, maybe not. But it's mandated by POSIX/C99. And of course we are free to ignore that and implement our own semantics (though within the constraints that we really want -Wformat to help us), but there seems to be existing code in-tree that relies on this behavior. For example I think this will break fsl_sata_intr_coalescing_store() which uses a scanf format of "%u%u". Sure, that could just say "%u %u" instead, but the point is that currently it doesn't. So without some reasonably thorough analysis across the tree, and updates of affected callers, NAK. Rasmus
From: Demi Marie Obenour > Sent: 10 June 2023 21:41 > > Passing spaces before e.g. an integer is usually > not intended. This was suggested by Christoph in > https://lore.kernel.org/lkml/ZIQrohcizoj4bZWx@infradead.org/. This is contrary to libc scanf and could easily affect userspace writing fixed width values into sysctl nodes (etc). IIRC strtoul() (etc) are also expected to strip leading spaces. Removing the sign in sscanf() may lead to "- 12" being valid - which may not be desired. David > > Suggested-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> > --- > lib/vsprintf.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 9e53355c35b1d6260631868228ede1d178fe3325..665f6197f8313d653f67d7886b12c43942e058dd 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -3551,8 +3551,6 @@ int vsscanf(const char *buf, const char *fmt, va_list args) > char *s = (char *)va_arg(args, char *); > if (field_width == -1) > field_width = SHRT_MAX; > - /* first, skip leading white space in buffer */ > - str = skip_spaces(str); > > /* now copy until next white space */ > while (*str && !isspace(*str) && field_width--) > @@ -3639,11 +3637,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args) > return num; > } > > - /* have some sort of integer conversion. > - * first, skip white space in buffer. > - */ > - str = skip_spaces(str); > - > + /* have some sort of integer conversion. */ > digit = *str; > if (is_sign && digit == '-') { > if (field_width == 1) > -- > Sincerely, > Demi Marie Obenour (she/her/hers) > Invisible Things Lab - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: Rasmus Villemoes > Sent: 12 June 2023 12:08 > > On 10/06/2023 22.40, Demi Marie Obenour wrote: > > Passing spaces before e.g. an integer is usually > > not intended. > > Maybe, maybe not. But it's mandated by POSIX/C99. > > And of course we are free to ignore that and implement our own semantics > (though within the constraints that we really want -Wformat to help us), > but there seems to be existing code in-tree that relies on this > behavior. For example I think this will break > fsl_sata_intr_coalescing_store() which uses a scanf format of "%u%u". > > Sure, that could just say "%u %u" instead, but the point is that > currently it doesn't. So without some reasonably thorough analysis > across the tree, and updates of affected callers, NAK. It would almost certainly need to be " %u %u" to allow for userspace generating the input with "%6u %6u", David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 9e53355c35b1d6260631868228ede1d178fe3325..665f6197f8313d653f67d7886b12c43942e058dd 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -3551,8 +3551,6 @@ int vsscanf(const char *buf, const char *fmt, va_list args) char *s = (char *)va_arg(args, char *); if (field_width == -1) field_width = SHRT_MAX; - /* first, skip leading white space in buffer */ - str = skip_spaces(str); /* now copy until next white space */ while (*str && !isspace(*str) && field_width--) @@ -3639,11 +3637,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args) return num; } - /* have some sort of integer conversion. - * first, skip white space in buffer. - */ - str = skip_spaces(str); - + /* have some sort of integer conversion. */ digit = *str; if (is_sign && digit == '-') { if (field_width == 1)
Passing spaces before e.g. an integer is usually not intended. This was suggested by Christoph in https://lore.kernel.org/lkml/ZIQrohcizoj4bZWx@infradead.org/. Suggested-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> --- lib/vsprintf.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)