[PATCH] scanf: stop scanning if the L modifier is used for integers

[PATCH] scanf: stop scanning if the L modifier is used for integers

[Andrei Vagin]

From: Andrei Vagin <avagin@openvz.org>

According to a posix man page, the L modifier can not be
used for integers.

Let's look at this code:
        char str[] = "0x200 0x200";
        unsigned long long a = 0xb, b = 0xa;
	int ret;

	ret = sscanf(str, "%llx %Lx", &a, &b);
        printf("%d %llx %llx\n", ret, a, b);

Without this patch, this code prints "2 200 a", this means that two
items were parsed, but we see that the second item was parsed
incorrectly.

Actually scanf() should stop scanning as soon as it meets %Lx and
return 1.

Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
---
 src/stdio/vfscanf.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/stdio/vfscanf.c b/src/stdio/vfscanf.c
index 9e030fc4..8088b1b1 100644
--- a/src/stdio/vfscanf.c
+++ b/src/stdio/vfscanf.c
@@ -19,9 +19,9 @@
 #define SIZE_L   2
 #define SIZE_ll  3
 
-static void store_int(void *dest, int size, unsigned long long i)
+static int store_int(void *dest, int size, unsigned long long i)
 {
-	if (!dest) return;
+	if (!dest) return 0;
 	switch (size) {
 	case SIZE_hh:
 		*(char *)dest = i;
@@ -38,7 +38,10 @@ static void store_int(void *dest, int size, unsigned long long i)
 	case SIZE_ll:
 		*(long long *)dest = i;
 		break;
+	default:
+		return -1;
 	}
+	return 0;
 }
 
 static void *arg_n(va_list ap, unsigned int n)
@@ -292,8 +295,10 @@ int vfscanf(FILE *restrict f, const char *restrict fmt, va_list ap)
 		int_common:
 			x = __intscan(f, base, 0, ULLONG_MAX);
 			if (!shcnt(f)) goto match_fail;
-			if (t=='p' && dest) *(void **)dest = (void *)(uintptr_t)x;
-			else store_int(dest, size, x);
+			if (t=='p' && dest)
+				*(void **)dest = (void *)(uintptr_t)x;
+			else if (store_int(dest, size, x))
+				goto fmt_fail;
 			break;
 		case 'a': case 'A':
 		case 'e': case 'E':
-- 
2.14.3

Re: [PATCH] scanf: stop scanning if the L modifier is used for integers

[Rich Felker]

On Fri, Jun 01, 2018 at 11:00:50AM +0300, Andrei Vagin wrote:
> From: Andrei Vagin <avagin@openvz.org>
> 
> According to a posix man page, the L modifier can not be
> used for integers.
> 
> Let's look at this code:
>         char str[] = "0x200 0x200";
>         unsigned long long a = 0xb, b = 0xa;
> 	int ret;
> 
> 	ret = sscanf(str, "%llx %Lx", &a, &b);
>         printf("%d %llx %llx\n", ret, a, b);
> 
> Without this patch, this code prints "2 200 a", this means that two
> items were parsed, but we see that the second item was parsed
> incorrectly.
> 
> Actually scanf() should stop scanning as soon as it meets %Lx and
> return 1.
> 
> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> ---
>  src/stdio/vfscanf.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/stdio/vfscanf.c b/src/stdio/vfscanf.c
> index 9e030fc4..8088b1b1 100644
> --- a/src/stdio/vfscanf.c
> +++ b/src/stdio/vfscanf.c
> @@ -19,9 +19,9 @@
>  #define SIZE_L   2
>  #define SIZE_ll  3
>  
> -static void store_int(void *dest, int size, unsigned long long i)
> +static int store_int(void *dest, int size, unsigned long long i)
>  {
> -	if (!dest) return;
> +	if (!dest) return 0;
>  	switch (size) {
>  	case SIZE_hh:
>  		*(char *)dest = i;
> @@ -38,7 +38,10 @@ static void store_int(void *dest, int size, unsigned long long i)
>  	case SIZE_ll:
>  		*(long long *)dest = i;
>  		break;
> +	default:
> +		return -1;
>  	}
> +	return 0;
>  }
>  
>  static void *arg_n(va_list ap, unsigned int n)
> @@ -292,8 +295,10 @@ int vfscanf(FILE *restrict f, const char *restrict fmt, va_list ap)
>  		int_common:
>  			x = __intscan(f, base, 0, ULLONG_MAX);
>  			if (!shcnt(f)) goto match_fail;
> -			if (t=='p' && dest) *(void **)dest = (void *)(uintptr_t)x;
> -			else store_int(dest, size, x);
> +			if (t=='p' && dest)
> +				*(void **)dest = (void *)(uintptr_t)x;
> +			else if (store_int(dest, size, x))
> +				goto fmt_fail;
>  			break;
>  		case 'a': case 'A':
>  		case 'e': case 'E':
> -- 
> 2.14.3

I was thinking store_int was the wrong place for this logic and that
it should go in the state machine in the main function, but unlike
printf our scanf doesn't really have any proper state machine for the
format string, and presumably fails to diagnose lots of invalid ones.
This is not a conformance problem, since invalid ones have undefined
behavior, but it does seem somewhat undesirable from a QoI standpoint.

I'll think about it a bit more and probably commit this patch or
something very close to it for now, and possibly revisit making scanf
handle invalid formats more robustly in the future.

Thanks for your work on this.

Rich