libstdio fscanf regression

libstdio fscanf regression

[qwx]

Hello,

Commit 7551 (fix '%[]' specifiers and '%n' (thanks phil9)) seems to have
broken libstdio's *scanf (ape version untested).

Test case is games/doom's configuration file loading, which uses fscanf:
/sys/src/games/doom/m_misc.c:314

...

	    if (fscanf (f, "%79s %[^\n]\n", def, strparm) == 2)

...

This reads key value pairs, one per line, like:

mouse_sensitivity		5

Despite successfully reading both strings on each line, the fscanf call
now returns 1 instead of 2, and the check always fails.

The effect is that games/doom is unable to read in any value from the
configuration file, and since it still can write a config file on exit,
the file is clobbered and all values are reset to default upon quitting.

I haven't looked at ape's stdio, perhaps the code's semantics are slightly
different.

Thanks for the help,

qwx

Re: [9front] libstdio fscanf regression

[ori]

> Hello,
> 
> Commit 7551 (fix '%[]' specifiers and '%n' (thanks phil9)) seems to have
> broken libstdio's *scanf (ape version untested).
> 
> Test case is games/doom's configuration file loading, which uses fscanf:
> /sys/src/games/doom/m_misc.c:314
> 
> ...
> 
> 	    if (fscanf (f, "%79s %[^\n]\n", def, strparm) == 2)
> 
> ...

Thanks for the report -- I think this is what we want. At least,
it works for your test case, and it tracks with what I'd expect
the semantics to be. I haven't pored over the spec yet.

Test code:

#include <u.h>
#include <libc.h>
#include <stdio.h>

void
main(int argc, char **argv)
{
	char def[80], strparm[128];

	print("%d\n", scanf("%79s %[^\n]\n", def, strparm));
	exits(nil);
}

Patch:

diff -r 35459627f401 sys/src/ape/lib/ap/stdio/vfscanf.c
--- a/sys/src/ape/lib/ap/stdio/vfscanf.c	Wed May 13 18:50:01 2020 -0700
+++ b/sys/src/ape/lib/ap/stdio/vfscanf.c	Thu May 14 17:48:32 2020 -0700
@@ -416,7 +416,7 @@
 		}
 		if(!match(c, pat)){
 			nungetc(c, f);
-			return 0;
+			return nn > 0;
 		}
 		if(store)
 			*s++=c;
diff -r 35459627f401 sys/src/libstdio/vfscanf.c
--- a/sys/src/libstdio/vfscanf.c	Wed May 13 18:50:01 2020 -0700
+++ b/sys/src/libstdio/vfscanf.c	Thu May 14 17:48:32 2020 -0700
@@ -339,6 +339,7 @@
 	int c, nn;
 	register char *s;
 	register const char *pat;
+
 	pat=++fmtp;
 	if(*fmtp=='^') fmtp++;
 	if(*fmtp!='\0') fmtp++;
@@ -354,7 +355,7 @@
 		}
 		if(!match(c, pat)){
 			nungetc(c, f);
-			return 0;
+			return nn > 0;
 		}
 		if(store) *s++=c;
 		nn++;

Re: [9front] libstdio fscanf regression

[qwx]

With this patch, fscanf returns the correct value, but the second string isn't
correctly nul terminated.
games/doom now crashes in crazy ways since it doesn't really do validation of
the config file's values.

I tweaked your test code:

#include <u.h>
#include <libc.h>
#include <stdio.h>

void
main(int, char **)
{
	char def[80], strparm[128];

	while(scanf("%79s %[^\n]\n", def, strparm) == 2)
		print("%s %s\n", def, strparm);
	exits(nil);
}


Test config file:

mouse_sensitivity		6
sfx_volume		2
music_volume		15
show_messages		1
key_right		174
key_left		172
key_up		119
key_down		115
key_strafeleft		97
key_straferight		100
key_fire		157
key_use		32
key_strafe		184
key_speed		182
autorun		1
use_mouse		1
mouseb_fire		0
mouseb_strafe		2
mouseb_forward		1
use_joystick		0
joyb_fire		0
joyb_strafe		1
joyb_use		3
joyb_speed		2
screenblocks		10
detaillevel		0
snd_channels		8
usegamma		1
chatmacro0		"No"
chatmacro1		"I'm ready to kick butt!"
chatmacro2		"I'm OK."
chatmacro3		"I'm not looking too good!"
chatmacro4		"Help!"
chatmacro5		"You suck!"
chatmacro6		"Next time, scumbag..."
chatmacro7		"Come here!"
chatmacro8		"I'll take care of it."
chatmacro9		"Yes"


Results:

mouse_sensitivity 6
sfx_volume 2
music_volume 15
show_messages 15
key_right 174
key_left 172
key_up 119
key_down 115
key_strafeleft 975
key_straferight 100
key_fire 157
key_use 327
key_strafe 184
key_speed 182
autorun 182
use_mouse 182
mouseb_fire 082
mouseb_strafe 282
mouseb_forward 182
use_joystick 082
joyb_fire 082
joyb_strafe 182
joyb_use 382
joyb_speed 282
screenblocks 102
detaillevel 002
snd_channels 802
usegamma 102
chatmacro0 "No"
chatmacro1 "I'm ready to kick butt!"
chatmacro2 "I'm OK."y to kick butt!"
chatmacro3 "I'm not looking too good!"
chatmacro4 "Help!"t looking too good!"
chatmacro5 "You suck!"oking too good!"
chatmacro6 "Next time, scumbag..."od!"
chatmacro7 "Come here!"scumbag..."od!"
chatmacro8 "I'll take care of it."od!"
chatmacro9 "Yes" take care of it."od!"

Oddly, the results are slightly different from within doom's code:

[...]
usegamma 102
chatmacro0 "No"�5@
chatmacro1 "I'm ready to kick butt!"
chatmacro2 "I'm OK."y to kick butt!
chatmacro3 "I'm not looking too good!"
chatmacro4 "Help!"t looking too good!
chatmacro5 "You suck!"oking too good
chatmacro6 "Next time, scumbag..."o
chatmacro7 "Come here!"scumbag..."
chatmacro8 "I'll take care of it."
chatmacro9 "Yes" take care of it.

Thanks,

qwx

Re: [9front] libstdio fscanf regression

[ori]

> With this patch, fscanf returns the correct value, but the second string isn't
> correctly nul terminated.
> games/doom now crashes in crazy ways since it doesn't really do validation of
> the config file's values.

Got it. This should null terminate:

Not committing today -- I'm a bit drunk -- but I think this is correct:


diff -r 35459627f401 sys/src/ape/lib/ap/stdio/vfscanf.c
--- a/sys/src/ape/lib/ap/stdio/vfscanf.c	Wed May 13 18:50:01 2020 -0700
+++ b/sys/src/ape/lib/ap/stdio/vfscanf.c	Thu May 14 18:41:40 2020 -0700
@@ -416,7 +416,7 @@
 		}
 		if(!match(c, pat)){
 			nungetc(c, f);
-			return 0;
+			goto Done;
 		}
 		if(store)
 			*s++=c;
@@ -424,5 +424,5 @@
 	}
 Done:
 	if(store) *s='\0';
-	return 1;
+	return nn > 0;
 }
diff -r 35459627f401 sys/src/libstdio/vfscanf.c
--- a/sys/src/libstdio/vfscanf.c	Wed May 13 18:50:01 2020 -0700
+++ b/sys/src/libstdio/vfscanf.c	Thu May 14 18:41:40 2020 -0700
@@ -339,6 +339,7 @@
 	int c, nn;
 	register char *s;
 	register const char *pat;
+
 	pat=++fmtp;
 	if(*fmtp=='^') fmtp++;
 	if(*fmtp!='\0') fmtp++;
@@ -354,12 +355,12 @@
 		}
 		if(!match(c, pat)){
 			nungetc(c, f);
-			return 0;
+			goto Done;
 		}
 		if(store) *s++=c;
 		nn++;
 	}
 Done:
 	if(store) *s='\0';
-	return 1;
+	return nn > 0;
 }

Re: [9front] libstdio fscanf regression

[qwx]

Yep, that works.  By the way, your replies were sent to both my mail adress
and to the mailing list, so I got them twice, not sure if that was intended.

Anyway, thanks!

Have another beer on me,

qwx