diff mbox series

[v3,3/4] vsscanf(): do not skip spaces

Message ID 20230610204044.3653-4-demi@invisiblethingslab.com
State New
Headers
Series Make sscanf() stricter |

Commit Message

Demi Marie Obenour June 10, 2023, 8:40 p.m. UTC 
  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(-)

Comments

Rasmus Villemoes June 12, 2023, 11:08 a.m. UTC | #1 
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
David Laight June 12, 2023, 11:11 a.m. UTC | #2 
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)
David Laight June 13, 2023, 12:42 p.m. UTC | #3 
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 mbox series

Patch

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)