[PATCH] fix APE sscanf [ specifier

[PATCH] fix APE sscanf [ specifier

[telephil9]

Hi,

APE sscanf return value is wrong when using a [ specifier and there is no match.
The following code highlights the issue (full example: http://okturing.com/src/8478/body)
	char s[2];
	sscanf("H", " %1[Zz] ", s); // SHOULD NOT RETURN 1

This patch fixes the issue:
---

diff -r 8006152d13d2 sys/src/ape/lib/ap/stdio/vfscanf.c
--- a/sys/src/ape/lib/ap/stdio/vfscanf.c	Tue Apr 28 20:51:19 2020 -0700
+++ b/sys/src/ape/lib/ap/stdio/vfscanf.c	Thu May 07 09:44:39 2020 +0200
@@ -396,13 +396,14 @@
 			if(nn==0) return 0;
 			else goto Done;
 		}
-		if(!match(c, pat))
-			break;
+		if(!match(c, pat)){
+			nungetc(c, f);
+			return 0;
+		}
 		if(store)
 			*s++=c;
 		nn++;
 	}
-	nungetc(c, f);
 Done:
 	if(store) *s='\0';
 	return 1;

---

Regards,
Philippe

Re: [PATCH] fix APE sscanf [ specifier

[telephil9]

Hi,

In addition, same patch applied to libstdio:
---

diff -r 8006152d13d2 sys/src/libstdio/vfscanf.c
--- a/sys/src/libstdio/vfscanf.c	Tue Apr 28 20:51:19 2020 -0700
+++ b/sys/src/libstdio/vfscanf.c	Thu May 07 18:15:29 2020 +0200
@@ -353,11 +353,13 @@
 			if(nn==0) return 0;
 			else goto Done;
 		}
-		if(!match(c, pat)) break;
+		if(!match(c, pat)){
+			nungetc(c, f);
+			return 0;
+		}
 		if(store) *s++=c;
 		nn++;
 	}
-	nungetc(c, f);
 Done:
 	if(store) *s='\0';
 	return 1;

---

Regards,
Philippe

Re: [9front] Re: [PATCH] fix APE sscanf [ specifier

[ori]

> Hi,
> 
> In addition, same patch applied to libstdio:
> ---
> 
> diff -r 8006152d13d2 sys/src/libstdio/vfscanf.c
> --- a/sys/src/libstdio/vfscanf.c	Tue Apr 28 20:51:19 2020 -0700
> +++ b/sys/src/libstdio/vfscanf.c	Thu May 07 18:15:29 2020 +0200
> @@ -353,11 +353,13 @@
>  			if(nn==0) return 0;
>  			else goto Done;
>  		}
> -		if(!match(c, pat)) break;
> +		if(!match(c, pat)){
> +			nungetc(c, f);
> +			return 0;
> +		}
>  		if(store) *s++=c;
>  		nn++;
>  	}
> -	nungetc(c, f);
>  Done:
>  	if(store) *s='\0';
>  	return 1;
> 
> ---
> 
> Regards,
> Philippe

Looks good to me -- will commit. Note, this still isn't fully up to
spec, since it should in theory handle length modifiers for unicode,
but I think it covers everything we're likely to see.

http://port70.net/~nsz/c/c99/n1256.html#7.19.6.2

	If an l length modifier is present, the input shall be a
	sequence of multibyte characters that begins in the initial
	shift state.  Each multibyte character is converted to a wide
	character as if by a call to the mbrtowc function, with the
	conversion state described by an mbstate_t object initialized
	to zero before the first multibyte character is converted.

I'll commit this and the ape version.

Re: [9front] Re: [PATCH] fix APE sscanf [ specifier

[telephil9]

Hi,

Thank you for the feedback Ori.

> Looks good to me -- will commit. Note, this still isn't fully up to
> spec, since it should in theory handle length modifiers for unicode,
> but I think it covers everything we're likely to see.

Regarding unicode you are right, but you surely have noted that our versions of vfscanf are not unicode aware at all. At least, this is document in the manual (see BUGS in sscanf(2)).

Below is a new version of the patch that includes a fix for an additionnal issue found with %n directive this time.
vfscanf is returning as soon as it reaches the end of the input stream which will prevent any trailing %n directives from being processed.
This bug can be observed in the following call:
	sscanf("z", " %1[Zz] %n ", s, &n);
Here n will not be set as the %n directive is never reached.

[
diff -r 8006152d13d2 sys/src/ape/lib/ap/stdio/vfscanf.c
--- a/sys/src/ape/lib/ap/stdio/vfscanf.c	Tue Apr 28 20:51:19 2020 -0700
+++ b/sys/src/ape/lib/ap/stdio/vfscanf.c	Sat May 09 08:26:27 2020 +0200
@@ -72,7 +72,6 @@
 			do
 				c=ngetc(f);
 			while(isspace(c));
-			if(c==EOF) return ncvt?ncvt:EOF;
 			nungetc(c, f);
 			break;
 		}
@@ -396,13 +395,14 @@
 			if(nn==0) return 0;
 			else goto Done;
 		}
-		if(!match(c, pat))
-			break;
+		if(!match(c, pat)){
+			nungetc(c, f);
+			return 0;
+		}
 		if(store)
 			*s++=c;
 		nn++;
 	}
-	nungetc(c, f);
 Done:
 	if(store) *s='\0';
 	return 1;
diff -r 8006152d13d2 sys/src/libstdio/vfscanf.c
--- a/sys/src/libstdio/vfscanf.c	Tue Apr 28 20:51:19 2020 -0700
+++ b/sys/src/libstdio/vfscanf.c	Sat May 09 08:26:27 2020 +0200
@@ -68,7 +68,6 @@
 			do
 				c=ngetc(f);
 			while(isspace(c));
-			if(c==EOF) return ncvt?ncvt:EOF;
 			nungetc(c, f);
 			break;
 		}
@@ -353,11 +352,13 @@
 			if(nn==0) return 0;
 			else goto Done;
 		}
-		if(!match(c, pat)) break;
+		if(!match(c, pat)){
+			nungetc(c, f);
+			return 0;
+		}
 		if(store) *s++=c;
 		nn++;
 	}
-	nungetc(c, f);
 Done:
 	if(store) *s='\0';
 	return 1;
]

Regards,
Philippe

PS: patch enclosed in square brackets for ease of selection :)

Re: [9front] Re: [PATCH] fix APE sscanf [ specifier

[ori]

> Hi,
> 
> Thank you for the feedback Ori.
> 
>> Looks good to me -- will commit. Note, this still isn't fully up to
>> spec, since it should in theory handle length modifiers for unicode,
>> but I think it covers everything we're likely to see.
> 
> Regarding unicode you are right, but you surely have noted that our versions of vfscanf are not unicode aware at all. At least, this is document in the manual (see BUGS in sscanf(2)).
> 
> Below is a new version of the patch that includes a fix for an additionnal issue found with %n directive this time.
> vfscanf is returning as soon as it reaches the end of the input stream which will prevent any trailing %n directives from being processed.
> This bug can be observed in the following call:
> 	sscanf("z", " %1[Zz] %n ", s, &n);
> Here n will not be set as the %n directive is never reached.
> 

Committed, thanks!