public inbox for libc-hacker@sourceware.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Ulrich Drepper <drepper@redhat.com>
Cc: Glibc hackers <libc-hacker@sources.redhat.com>
Subject: [PATCH] Fix printf_unknown and do_positional (BZ #4514)
Date: Mon, 21 May 2007 13:17:00 -0000	[thread overview]
Message-ID: <20070521131923.GD3081@sunsite.mff.cuni.cz> (raw)

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

             reply	other threads:[~2007-05-21 13:17 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-05-21 13:17 Jakub Jelinek [this message]
2007-05-21 18:08 ` Ulrich Drepper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070521131923.GD3081@sunsite.mff.cuni.cz \
    --to=jakub@redhat.com \
    --cc=drepper@redhat.com \
    --cc=libc-hacker@sources.redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).