[PATCH] handle whitespace before %% in scanf

[PATCH] handle whitespace before %% in scanf

[Bartosz Brachaczek]

this is mandated by C and POSIX standards and is in accordance with
glibc behavior.
---
 src/stdio/vfscanf.c  | 10 +++++++---
 src/stdio/vfwscanf.c |  8 ++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/stdio/vfscanf.c b/src/stdio/vfscanf.c
index d4d2454b..9e030fc4 100644
--- a/src/stdio/vfscanf.c
+++ b/src/stdio/vfscanf.c
@@ -89,15 +89,19 @@ int vfscanf(FILE *restrict f, const char *restrict fmt, va_list ap)
 			continue;
 		}
 		if (*p != '%' || p[1] == '%') {
-			p += *p=='%';
 			shlim(f, 0);
-			c = shgetc(f);
+			if (*p == '%') {
+				p++;
+				while (isspace((c=shgetc(f))));
+			} else {
+				c = shgetc(f);
+			}
 			if (c!=*p) {
 				shunget(f);
 				if (c<0) goto input_fail;
 				goto match_fail;
 			}
-			pos++;
+			pos += shcnt(f);
 			continue;
 		}
 
diff --git a/src/stdio/vfwscanf.c b/src/stdio/vfwscanf.c
index ad8e2b9a..5d387a2d 100644
--- a/src/stdio/vfwscanf.c
+++ b/src/stdio/vfwscanf.c
@@ -117,8 +117,12 @@ int vfwscanf(FILE *restrict f, const wchar_t *restrict fmt, va_list ap)
 			continue;
 		}
 		if (*p != '%' || p[1] == '%') {
-			p += *p=='%';
-			c = getwc(f);
+			if (*p == '%') {
+				p++;
+				while (iswspace((c=getwc(f)))) pos++;
+			} else {
+				c = getwc(f);
+			}
 			if (c!=*p) {
 				ungetwc(c, f);
 				if (c<0) goto input_fail;
-- 
2.13.0

Re: [PATCH] handle whitespace before %% in scanf

[Rich Felker]

On Sun, Jul 09, 2017 at 11:00:18PM +0200, Bartosz Brachaczek wrote:
> this is mandated by C and POSIX standards and is in accordance with
  ^^^^
> glibc behavior.

Can you explain exactly what "this" refers to?

> ---
>  src/stdio/vfscanf.c  | 10 +++++++---
>  src/stdio/vfwscanf.c |  8 ++++++--
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/src/stdio/vfscanf.c b/src/stdio/vfscanf.c
> index d4d2454b..9e030fc4 100644
> --- a/src/stdio/vfscanf.c
> +++ b/src/stdio/vfscanf.c
> @@ -89,15 +89,19 @@ int vfscanf(FILE *restrict f, const char *restrict fmt, va_list ap)
>  			continue;
>  		}
>  		if (*p != '%' || p[1] == '%') {
> -			p += *p=='%';
>  			shlim(f, 0);
> -			c = shgetc(f);
> +			if (*p == '%') {
> +				p++;
> +				while (isspace((c=shgetc(f))));
> +			} else {
> +				c = shgetc(f);
> +			}

It looks like you're claiming %% consumes space, which I can't find
any support for in the C standard. Has this topic been discussed
somewhere I should see?

Rich

Re: [PATCH] handle whitespace before %% in scanf

[Bartosz Brachaczek]

Hello,

On 7/10/2017 4:00 AM, Rich Felker wrote:
> On Sun, Jul 09, 2017 at 11:00:18PM +0200, Bartosz Brachaczek wrote:
>> this is mandated by C and POSIX standards and is in accordance with
>    ^^^^
>> glibc behavior.
> 
> Can you explain exactly what "this" refers to?

Ah, poor wording choice on my part. Yes, I meant that %% consumes 
whitespace. Shall I resend the patch with restated commit message if you 
think it's otherwise good?

> It looks like you're claiming %% consumes space, which I can't find
> any support for in the C standard. Has this topic been discussed
> somewhere I should see?

Sorry, I didn't think this would be controversial. No prior discussion. 
Let me present my reasoning below.

The following paragraph in the description of the fscanf function in the 
C11 standard, §7.21.6.2, establishes that '%%' is a "conversion 
specification", where '%' is the "conversion specifier":

> The format shall be a multibyte character sequence, beginning and
> ending in its initial shift state. The format is composed of zero or
> more directives: one or more white-space characters, an ordinary
> multibyte character (neither '%' nor a white-space character), or a
> conversion specification. Each conversion specification is introduced
> by the character '%'. After the '%', the following appear in sequence:
> 
> -- . . .
> 
> -- A "conversion specifier" character that specifies the type of
>    conversion to be applied.

That '%' is a valid conversion specifier is established a few paragraphs 
below:

> The conversion specifiers and their meanings are:
> 
> . . .
> 
> '%'     Matches a single '%' character; no conversion or assignment
>         occurs. The complete conversion specification shall be '%%'.

Between the above paragraphs, there is a definition of how a conversion 
specification is executed:

> A directive that is a conversion specification defines a set of matching
> input sequences, as described below for each specifier. A conversion
> specification is executed in the following steps:
> 
> Input white-space characters (as specified by the 'isspace' function)
> are skipped, unless the specification includes a '[', 'c', or 'n'
> specifier.
> 
> . . .

 From the above I conclude that all conversion specifications, except 
'%[', '%c', and '%n', consume whitespace. This includes the '%%' 
conversion specification.

The above can be applied just as well to C99. However, C11 added a new 
example (still in §7.21.6.2) that seems to confirm my reading of the 
normative text:

> EXAMPLE 5 The call:
> 
>     #include <stdio.h>
>     /* ... */
>     int n, i;
>     n = sscanf("foo % bar 42", "foo%%bar%d", &i);
> 
> will assign to 'n' the value 1 and to 'i' the value 42 because input
> white-space characters are skipped for both the '%' and 'd' conversion
> specifiers.

Now, the code in the example is clearly broken, as either the format 
string should be "foo%% bar%d" or the input string should be
"foo %bar 42", but the explanation does imply that '%%' consumes whitespace.

Bartosz

Re: [PATCH] handle whitespace before %% in scanf

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

Hello Bartosz,

On Mon, 10 Jul 2017 10:22:37 +0200 Bartosz Brachaczek
<b.brachaczek@gmail.com> wrote:

> > EXAMPLE 5 The call:
> > 
> >     #include <stdio.h>
> >     /* ... */
> >     int n, i;
> >     n = sscanf("foo % bar 42", "foo%%bar%d", &i);
> > 
> > will assign to 'n' the value 1 and to 'i' the value 42 because input
> > white-space characters are skipped for both the '%' and 'd'
> > conversion specifiers.  
> 
> Now, the code in the example is clearly broken, as either the format 
> string should be "foo%% bar%d" or the input string should be
> "foo %bar 42", but the explanation does imply that '%%' consumes
> whitespace.

Yes, I have noted this as an editorial fix for C17

Thanks for reporting
Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] handle whitespace before %% in scanf

[Rich Felker]

On Mon, Jul 10, 2017 at 04:44:45PM +0200, Jens Gustedt wrote:
> Hello Bartosz,
> 
> On Mon, 10 Jul 2017 10:22:37 +0200 Bartosz Brachaczek
> <b.brachaczek@gmail.com> wrote:
> 
> > > EXAMPLE 5 The call:
> > > 
> > >     #include <stdio.h>
> > >     /* ... */
> > >     int n, i;
> > >     n = sscanf("foo % bar 42", "foo%%bar%d", &i);
> > > 
> > > will assign to 'n' the value 1 and to 'i' the value 42 because input
> > > white-space characters are skipped for both the '%' and 'd'
> > > conversion specifiers.  
> > 
> > Now, the code in the example is clearly broken, as either the format 
> > string should be "foo%% bar%d" or the input string should be
> > "foo %bar 42", but the explanation does imply that '%%' consumes
> > whitespace.
> 
> Yes, I have noted this as an editorial fix for C17

So you can confirm that it's intentional that %% consume space? That
seems utterly awful but I guess you can use %*1[%] in place of %% to
do what %% should...

Rich

Re: [PATCH] handle whitespace before %% in scanf

[Bartosz Brachaczek]

On 7/10/2017 4:44 PM, Jens Gustedt wrote:
> Hello Bartosz,
> 
> On Mon, 10 Jul 2017 10:22:37 +0200 Bartosz Brachaczek
> <b.brachaczek@gmail.com> wrote:
> 
>>> EXAMPLE 5 The call:
>>>
>>>      #include <stdio.h>
>>>      /* ... */
>>>      int n, i;
>>>      n = sscanf("foo % bar 42", "foo%%bar%d", &i);
>>>
>>> will assign to 'n' the value 1 and to 'i' the value 42 because input
>>> white-space characters are skipped for both the '%' and 'd'
>>> conversion specifiers.
>>
>> Now, the code in the example is clearly broken, as either the format
>> string should be "foo%% bar%d" or the input string should be
>> "foo %bar 42", but the explanation does imply that '%%' consumes
>> whitespace.
> 
> Yes, I have noted this as an editorial fix for C17
> 
> Thanks for reporting

Nice, thanks. Looks this one has already been reported here:

http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2033.htm

I can't find what's the status of that document, though.

Re: [PATCH] handle whitespace before %% in scanf

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

Hello Bartosz,

On Tue, 5 Sep 2017 00:00:46 +0200 Bartosz Brachaczek
<b.brachaczek@gmail.com> wrote:

> Nice, thanks. Looks this one has already been reported here:
> 
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2033.htm

right!

> I can't find what's the status of that document, though.

In fact it has been "handled" at the London meeting last year:

> 8.1.4.1 N2033, fscanf example 5
> Is this editorial ? Yes.

So all of this is settled.

Thanks
Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] handle whitespace before %% in scanf

[Rich Felker]

On Sun, Jul 09, 2017 at 11:00:18PM +0200, Bartosz Brachaczek wrote:
> this is mandated by C and POSIX standards and is in accordance with
> glibc behavior.
> ---
>  src/stdio/vfscanf.c  | 10 +++++++---
>  src/stdio/vfwscanf.c |  8 ++++++--
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/src/stdio/vfscanf.c b/src/stdio/vfscanf.c
> index d4d2454b..9e030fc4 100644
> --- a/src/stdio/vfscanf.c
> +++ b/src/stdio/vfscanf.c
> @@ -89,15 +89,19 @@ int vfscanf(FILE *restrict f, const char *restrict fmt, va_list ap)
>  			continue;
>  		}
>  		if (*p != '%' || p[1] == '%') {
> -			p += *p=='%';
>  			shlim(f, 0);
> -			c = shgetc(f);
> +			if (*p == '%') {
> +				p++;
> +				while (isspace((c=shgetc(f))));
> +			} else {
> +				c = shgetc(f);
> +			}
>  			if (c!=*p) {
>  				shunget(f);
>  				if (c<0) goto input_fail;
>  				goto match_fail;
>  			}
> -			pos++;
> +			pos += shcnt(f);
>  			continue;
>  		}

Assuming your interpretation is correct, I have no objection to going
forward with the change, but I don't think this is the right way to do
it. The only reason %% was handled in the code that handles literal
characters is because I assumed it behaves like one, but if it
doesn't, it should just be handled as a format specifier that consumes
space where it can use the existing code that does that, rather than
complicting the code for literals and adding a duplicate of the
space-skipping code to it.

Rich

Re: [PATCH] handle whitespace before %% in scanf

[Rich Felker]

On Mon, Jul 10, 2017 at 09:20:39PM -0400, Rich Felker wrote:
> On Sun, Jul 09, 2017 at 11:00:18PM +0200, Bartosz Brachaczek wrote:
> > this is mandated by C and POSIX standards and is in accordance with
> > glibc behavior.
> > ---
> >  src/stdio/vfscanf.c  | 10 +++++++---
> >  src/stdio/vfwscanf.c |  8 ++++++--
> >  2 files changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/stdio/vfscanf.c b/src/stdio/vfscanf.c
> > index d4d2454b..9e030fc4 100644
> > --- a/src/stdio/vfscanf.c
> > +++ b/src/stdio/vfscanf.c
> > @@ -89,15 +89,19 @@ int vfscanf(FILE *restrict f, const char *restrict fmt, va_list ap)
> >  			continue;
> >  		}
> >  		if (*p != '%' || p[1] == '%') {
> > -			p += *p=='%';
> >  			shlim(f, 0);
> > -			c = shgetc(f);
> > +			if (*p == '%') {
> > +				p++;
> > +				while (isspace((c=shgetc(f))));
> > +			} else {
> > +				c = shgetc(f);
> > +			}
> >  			if (c!=*p) {
> >  				shunget(f);
> >  				if (c<0) goto input_fail;
> >  				goto match_fail;
> >  			}
> > -			pos++;
> > +			pos += shcnt(f);
> >  			continue;
> >  		}
> 
> Assuming your interpretation is correct, I have no objection to going
> forward with the change, but I don't think this is the right way to do
> it. The only reason %% was handled in the code that handles literal
> characters is because I assumed it behaves like one, but if it
> doesn't, it should just be handled as a format specifier that consumes
> space where it can use the existing code that does that, rather than
> complicting the code for literals and adding a duplicate of the
> space-skipping code to it.

I tried going forward with the idea I proposed, but it looks like it's
actually more invasive: in addition to adding the final case to
actually handle '%', it adds a new case where a conversion specifier
does not consume a variadic input, and a new case where width is
forced to 1 and modifier flags and explicit widths are rejected.

As such I think your patch as originally submitted is probably the
best approach. Sorry for the delay in reviewing and accepting it.

Rich

Re: [PATCH] handle whitespace before %% in scanf

[Bartosz Brachaczek]

[-- Attachment #1: Type: text/plain, Size: 2615 bytes --]

On 9/4/2017 10:59 PM, Rich Felker wrote:
> On Mon, Jul 10, 2017 at 09:20:39PM -0400, Rich Felker wrote:
>> On Sun, Jul 09, 2017 at 11:00:18PM +0200, Bartosz Brachaczek wrote:
>>> this is mandated by C and POSIX standards and is in accordance with
>>> glibc behavior.
>>> ---
>>>   src/stdio/vfscanf.c  | 10 +++++++---
>>>   src/stdio/vfwscanf.c |  8 ++++++--
>>>   2 files changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/stdio/vfscanf.c b/src/stdio/vfscanf.c
>>> index d4d2454b..9e030fc4 100644
>>> --- a/src/stdio/vfscanf.c
>>> +++ b/src/stdio/vfscanf.c
>>> @@ -89,15 +89,19 @@ int vfscanf(FILE *restrict f, const char *restrict fmt, va_list ap)
>>>   			continue;
>>>   		}
>>>   		if (*p != '%' || p[1] == '%') {
>>> -			p += *p=='%';
>>>   			shlim(f, 0);
>>> -			c = shgetc(f);
>>> +			if (*p == '%') {
>>> +				p++;
>>> +				while (isspace((c=shgetc(f))));
>>> +			} else {
>>> +				c = shgetc(f);
>>> +			}
>>>   			if (c!=*p) {
>>>   				shunget(f);
>>>   				if (c<0) goto input_fail;
>>>   				goto match_fail;
>>>   			}
>>> -			pos++;
>>> +			pos += shcnt(f);
>>>   			continue;
>>>   		}
>>
>> Assuming your interpretation is correct, I have no objection to going
>> forward with the change, but I don't think this is the right way to do
>> it. The only reason %% was handled in the code that handles literal
>> characters is because I assumed it behaves like one, but if it
>> doesn't, it should just be handled as a format specifier that consumes
>> space where it can use the existing code that does that, rather than
>> complicting the code for literals and adding a duplicate of the
>> space-skipping code to it.
> 
> I tried going forward with the idea I proposed, but it looks like it's
> actually more invasive: in addition to adding the final case to
> actually handle '%', it adds a new case where a conversion specifier
> does not consume a variadic input, and a new case where width is
> forced to 1 and modifier flags and explicit widths are rejected.
> 
> As such I think your patch as originally submitted is probably the
> best approach. Sorry for the delay in reviewing and accepting it.

Oh, that's perfect, thanks. Sorry I didn't get to responding to your 
request. I originally tried both approaches and chose the one that had 
smaller impact on code size in vfscanf.o.

While at it, you might want to have a look at another trivial patch for 
vfwscanf I submitted:

http://www.openwall.com/lists/musl/2017/07/09/5

If anything, it avoids confusion for people reading the code. I'm 
attaching a version of this patch with enough context lines for inline 
review.

[-- Attachment #2: 0001-slightly-simplify-a-condition-in-vfwscanf.patch --]
[-- Type: text/plain, Size: 1004 bytes --]

From 5909c8a9b87db57285122c87368457558bebf3b6 Mon Sep 17 00:00:00 2001
From: Bartosz Brachaczek <b.brachaczek@gmail.com>
Date: Sun, 9 Jul 2017 22:25:54 +0200
Subject: [PATCH] slightly simplify a condition in vfwscanf

the OR is not needed since commit
de80ea9f1c2821cbb4205533b86d5d17f9e8d376.
---
 src/stdio/vfwscanf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/stdio/vfwscanf.c b/src/stdio/vfwscanf.c
index 1ebc5cef..ad8e2b9a 100644
--- a/src/stdio/vfwscanf.c
+++ b/src/stdio/vfwscanf.c
@@ -184,21 +184,21 @@ int vfwscanf(FILE *restrict f, const wchar_t *restrict fmt, va_list ap)
 
 		t = *p;
 
 		/* Transform S,C -> ls,lc */
 		if ((t&0x2f)==3) {
 			size = SIZE_l;
 			t |= 32;
 		}
 
 		if (t != 'n') {
-			if (t != '[' && (t|32) != 'c')
+			if (t != '[' && t != 'c')
 				while (iswspace((c=getwc(f)))) pos++;
 			else
 				c=getwc(f);
 			if (c < 0) goto input_fail;
 			ungetwc(c, f);
 		}
 
 		switch (t) {
 		case 'n':
 			store_int(dest, size, pos);
-- 
2.14.1