diff mbox

[v2] _scanf.c: Implement 'm' modifier for 'c' and '[' conversions.

Message ID 1431036919-4088-1-git-send-email-jcmvbkbc@gmail.com
State Accepted
Commit fe7d30e6337f280a451ac057f47d48349b63e462
Headers show

Commit Message

Max Filippov May 7, 2015, 10:15 p.m. UTC 
From: Will Newton <will.newton@imgtec.com>

The current code implements the 'm' modifier only for 's'
conversions and would cause a segfault if it was used for 'c'
or '[' conversions. This patch extends the code to cover these
cases too.

The original version could write scanned data outside the passed buffer
because index i used in the '[' conversion handling block was clobbered.

Signed-off-by: Will Newton <will.newton@imgtec.com>
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
Changes v1->v2:
- add testcase for %[...]/%c/%m[...]/%mc.

 libc/stdio/_scanf.c  | 51 +++++++++++++++++++++++++++++++++------------------
 test/stdio/scanf_m.c | 24 ++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 18 deletions(-)
 create mode 100644 test/stdio/scanf_m.c

Comments

Max Filippov May 15, 2015, 6:44 p.m. UTC | #1 
On Fri, May 8, 2015 at 1:15 AM, Max Filippov <jcmvbkbc@gmail.com> wrote:
> From: Will Newton <will.newton@imgtec.com>
>
> The current code implements the 'm' modifier only for 's'
> conversions and would cause a segfault if it was used for 'c'
> or '[' conversions. This patch extends the code to cover these
> cases too.
>
> The original version could write scanned data outside the passed buffer
> because index i used in the '[' conversion handling block was clobbered.
>
> Signed-off-by: Will Newton <will.newton@imgtec.com>
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
> Changes v1->v2:
> - add testcase for %[...]/%c/%m[...]/%mc.

Ping?
Bernhard Reutner-Fischer May 15, 2015, 8 p.m. UTC | #2 
On May 15, 2015 8:44:11 PM GMT+02:00, Max Filippov <jcmvbkbc@gmail.com> wrote:
>On Fri, May 8, 2015 at 1:15 AM, Max Filippov <jcmvbkbc@gmail.com>
>wrote:
>> From: Will Newton <will.newton@imgtec.com>
>>
>> The current code implements the 'm' modifier only for 's'
>> conversions and would cause a segfault if it was used for 'c'
>> or '[' conversions. This patch extends the code to cover these
>> cases too.
>>
>> The original version could write scanned data outside the passed
>buffer
>> because index i used in the '[' conversion handling block was
>clobbered.
>>
>> Signed-off-by: Will Newton <will.newton@imgtec.com>
>> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
>> ---
>> Changes v1->v2:
>> - add testcase for %[...]/%c/%m[...]/%mc.
>
>Ping?

This is OK, will install the pending patches early next week.
Thanks!
Bernhard Reutner-Fischer May 26, 2015, 8:28 p.m. UTC | #3 
On Fri, May 15, 2015 at 10:00:23PM +0200, Bernhard Reutner-Fischer wrote:
> On May 15, 2015 8:44:11 PM GMT+02:00, Max Filippov <jcmvbkbc@gmail.com> wrote:
> >On Fri, May 8, 2015 at 1:15 AM, Max Filippov <jcmvbkbc@gmail.com>
> >wrote:
> >> From: Will Newton <will.newton@imgtec.com>
> >>
> >> The current code implements the 'm' modifier only for 's'
> >> conversions and would cause a segfault if it was used for 'c'
> >> or '[' conversions. This patch extends the code to cover these
> >> cases too.
> >>
> >> The original version could write scanned data outside the passed
> >buffer
> >> because index i used in the '[' conversion handling block was
> >clobbered.
> >>
> >> Signed-off-by: Will Newton <will.newton@imgtec.com>
> >> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> >> ---
> >> Changes v1->v2:
> >> - add testcase for %[...]/%c/%m[...]/%mc.
> >
> >Ping?
> 
> This is OK, will install the pending patches early next week.
> Thanks!

Applied, thanks!
diff mbox

Patch

diff --git a/libc/stdio/_scanf.c b/libc/stdio/_scanf.c
index 6ecb3cb..a5828c3 100644
--- a/libc/stdio/_scanf.c
+++ b/libc/stdio/_scanf.c
@@ -1352,7 +1352,20 @@  int VFSCANF (FILE *__restrict fp, const Wchar *__restrict format, va_list arg)
 				(psfs.conv_num >= CONV_c)
 #endif /* __UCLIBC_HAS_WCHAR__ */
 			{
+				/* We might have to handle the allocation ourselves */
+				int len;
+				unsigned char **ptr;
+
 				b = (psfs.store ? ((unsigned char *) psfs.cur_ptr) : buf);
+				/* With 'm', we actually got a pointer to a pointer */
+				ptr = (void *)b;
+
+				if (psfs.flags & FLAG_MALLOC) {
+					len = 0;
+					b = NULL;
+				} else
+					len = -1;
+
 				fail = 1;
 
 				if (psfs.conv_num == CONV_c) {
@@ -1360,32 +1373,28 @@  int VFSCANF (FILE *__restrict fp, const Wchar *__restrict format, va_list arg)
 						sc.width = 1;
 					}
 
+					if (psfs.flags & FLAG_MALLOC)
+						b = malloc(sc.width);
+
+					i = 0;
 					while (__scan_getc(&sc) >= 0) {
 						zero_conversions = 0;
-						*b = sc.cc;
-						b += psfs.store;
+						b[i] = sc.cc;
+						i += psfs.store;
 					}
 					__scan_ungetc(&sc);
 					if (sc.width > 0) {	/* Failed to read all required. */
 						goto DONE;
 					}
+					if (psfs.flags & FLAG_MALLOC)
+						*ptr = b;
 					psfs.cnt += psfs.store;
 					goto NEXT_FMT;
 				}
 
 				if (psfs.conv_num == CONV_s) {
-					/* We might have to handle the allocation ourselves */
-					int len;
-					/* With 'm', we actually got a pointer to a pointer */
-					unsigned char **ptr = (void *)b;
 
 					i = 0;
-					if (psfs.flags & FLAG_MALLOC) {
-						len = 0;
-						b = NULL;
-					} else
-						len = -1;
-
 					/* Yes, believe it or not, a %s conversion can store nuls. */
 					while ((__scan_getc(&sc) >= 0) && !isspace(sc.cc)) {
 						zero_conversions = 0;
@@ -1400,10 +1409,6 @@  int VFSCANF (FILE *__restrict fp, const Wchar *__restrict format, va_list arg)
 						fail = 0;
 					}
 
-					if (psfs.flags & FLAG_MALLOC)
-						*ptr = b;
-					/* The code below takes care of terminating NUL */
-					b += i;
 				} else {
 #ifdef __UCLIBC_HAS_WCHAR__
 					assert((psfs.conv_num == CONV_LEFTBRACKET) || \
@@ -1454,13 +1459,20 @@  int VFSCANF (FILE *__restrict fp, const Wchar *__restrict format, va_list arg)
 #endif /* __UCLIBC_HAS_WCHAR__ */
 
 
+					i = 0;
 					while (__scan_getc(&sc) >= 0) {
 						zero_conversions = 0;
 						if (!scanset[sc.cc]) {
 							break;
 						}
-						*b = sc.cc;
-						b += psfs.store;
+						if (i == len) {
+							/* Pick a size that won't trigger a lot of
+							 * mallocs early on ... */
+							len += 256;
+							b = realloc(b, len + 1);
+						}
+						b[i] = sc.cc;
+						i += psfs.store;
 						fail = 0;
 					}
 				}
@@ -1470,6 +1482,9 @@  int VFSCANF (FILE *__restrict fp, const Wchar *__restrict format, va_list arg)
 				if (fail) {	/* nothing stored! */
 					goto DONE;
 				}
+				if (psfs.flags & FLAG_MALLOC)
+					*ptr = b;
+				b += i;
 				*b = 0;		/* Nul-terminate string. */
 				psfs.cnt += psfs.store;
 				goto NEXT_FMT;
diff --git a/test/stdio/scanf_m.c b/test/stdio/scanf_m.c
new file mode 100644
index 0000000..0ce78b6
--- /dev/null
+++ b/test/stdio/scanf_m.c
@@ -0,0 +1,24 @@ 
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+int main(void)
+{
+	const char *buf = "hello world";
+	char *ps = NULL, *pc = NULL;
+	char s[6], c;
+
+	/* Check that %[...]/%c work. */
+	sscanf(buf, "%[a-z] %c", s, &c);
+	/* Check that %m[...]/%mc work. */
+	sscanf(buf, "%m[a-z] %mc", &ps, &pc);
+
+	if (strcmp(ps, "hello") != 0 || *pc != 'w' ||
+	    strcmp(s, "hello") != 0 || c != 'w')
+		return 1;
+
+	free(ps);
+	free(pc);
+
+	return 0;
+}