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

[Bartosz Brachaczek <b.brachaczek@gmail.com>]

[-- 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