[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20180603024201.GW1392@brightrain.aerifal.cx>
Date: Sat, 2 Jun 2018 22:42:01 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] scanf: stop scanning if the L modifier is used
for integers
On Fri, Jun 01, 2018 at 11:00:50AM +0300, Andrei Vagin wrote:
> From: Andrei Vagin <avagin@...nvz.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@...tuozzo.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
