[PATCH] Fix printf_unknown and do_positional (BZ #4514)

[PATCH] Fix printf_unknown and do_positional (BZ #4514)

[Jakub Jelinek]

Hi!

In printf_unknown we use work_buffer only for _itoa_word, printing
info->width or info->prec value.  So we only need enough bytes
to write those numbers in decimal, rather than info->width bytes + 32
(info->spec was a typo for info->prec I believe).

Now, when I wrote a testcase for this and in addition to the bugzilla
provided testcase used another format string afterwards, I discovered
another bug.  While the normal non-positional loop reinitializes
workend at the beginning of each iteration and frees workstart if
it was malloced at the end of each iteration, do_positional loop
only frees workstart at the end of each loop, without resetting
workend at the start of the loop.  This can be reproduced even without
any unknown format spec, just with positional args (see first test in the
testcase below).  Another problem is that do_positional loop uses its
own workstart variable which shadows the function's one.  This can lead to
memory leaks if e.g. outchar decides to goto all_done.

I believe we also have memory leaks if *printf call is cancelled while
it has some memory (workstart resp. string (string_malloced != 0)) malloced,
but I haven't addressed this in the patch below.

Also, wonder if we shouldn't compute the sum of allocaed memory sizes
within whole *printf and use it for __libc_use_alloca instead of testing
sizes separately.  Or don't throw away the so far largest work buffer
allocated through alloca, but instead reuse it if it is big enough.
Otherwise, e.g. printf ("%.*s%.*s%.*s%.*s%.*s%.*s%.*s%.*s%.*s%.*s%.*s%.*s",
			N, "", N, "", N, "", N, "", N, "", N, "",
			N, "", N, "", N, "", N, "", N, "", N, "");
for N such that __libc_use_alloca (N + 32) && !__libc_use_alloca ((N + 32) * 12)
could overflow the stack.

2007-05-21  Jakub Jelinek  <jakub@redhat.com>

	[BZ #4514]
	* stdio-common/vfprintf.c (vfprintf): Don't shadow workstart variable,
	reinitialize workend at the start of each do_positional format spec
	loop, free workstart before do_positional loops.
	(printf_unknown): Fix size of work_buffer.
	* stdio-common/tst-sprintf.c (main): Add 3 new testcases.

--- libc/stdio-common/vfprintf.c.jj	2007-05-09 13:26:13.000000000 +0200
+++ libc/stdio-common/vfprintf.c	2007-05-21 14:28:39.000000000 +0200
@@ -1627,6 +1627,9 @@ do_positional:
     /* Just a counter.  */
     size_t cnt;
 
+    if (__builtin_expect (workstart != NULL, 0))
+      free (workstart);
+    workstart = NULL;
 
     if (grouping == (const char *) -1)
       {
@@ -1801,7 +1804,9 @@ do_positional:
 	int use_outdigits = specs[nspecs_done].info.i18n;
 	char pad = specs[nspecs_done].info.pad;
 	CHAR_T spec = specs[nspecs_done].info.spec;
-	CHAR_T *workstart = NULL;
+
+	workstart = NULL;
+	workend = &work_buffer[sizeof (work_buffer) / sizeof (CHAR_T)];
 
 	/* Fill in last information.  */
 	if (specs[nspecs_done].width_arg != -1)
@@ -1926,7 +1931,7 @@ printf_unknown (FILE *s, const struct pr
 
 {
   int done = 0;
-  CHAR_T work_buffer[MAX (info->width, info->spec) + 32];
+  CHAR_T work_buffer[MAX (sizeof (info->width), sizeof (info->prec)) * 3];
   CHAR_T *const workend
     = &work_buffer[sizeof (work_buffer) / sizeof (CHAR_T)];
   register CHAR_T *w;
--- libc/stdio-common/tst-sprintf.c.jj	2007-01-25 10:58:43.000000000 +0100
+++ libc/stdio-common/tst-sprintf.c	2007-05-21 14:55:09.000000000 +0200
@@ -37,5 +37,26 @@ main (void)
       free (dst);
     }
 
+  if (sprintf (buf, "%1$d%3$.*2$s%4$d", 7, 67108863, "x", 8) != 3
+      || strcmp (buf, "7x8") != 0)
+    {
+      printf ("sprintf (buf, \"%%1$d%%3$.*2$s%%4$d\", 7, 67108863, \"x\", 8) produced `%s' output", buf);
+      result = 1;
+    }
+
+  if (sprintf (buf, "%67108863.16\"%d", 7) != 14
+      || strcmp (buf, "%67108863.16\"7") != 0)
+    {
+      printf ("sprintf (buf, \"%%67108863.16\\\"%%d\", 7) produced `%s' output", buf);
+      result = 1;
+    }
+
+  if (sprintf (buf, "%*\"%d", 0x3ffffff, 7) != 11
+      || strcmp (buf, "%67108863\"7") != 0)
+    {
+      printf ("sprintf (buf, \"%%*\\\"%%d\", 0x3ffffff, 7) produced `%s' output", buf);
+      result = 1;
+    }
+
   return result;
 }

	Jakub

Re: [PATCH] Fix printf_unknown and do_positional (BZ #4514)

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I applied the patch.

> Also, wonder if we shouldn't compute the sum of allocaed memory sizes
> within whole *printf and use it for __libc_use_alloca instead of testing
> sizes separately.  Or don't throw away the so far largest work buffer
> allocated through alloca, but instead reuse it if it is big enough.

Ideally this should be done, yes.

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFGUeAK2ijCOnn/RHQRAjilAJ4/tWVxXpZU1pVRYRPEqgZLjwtQlwCfVgHY
3XtNcjy0LrQj4zfvd753vgs=
=cXDn
-----END PGP SIGNATURE-----