From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2221 invoked by alias); 21 May 2007 13:17:57 -0000 Received: (qmail 2200 invoked by uid 22791); 21 May 2007 13:17:56 -0000 X-Spam-Check-By: sourceware.org Received: from sunsite.ms.mff.cuni.cz (HELO sunsite.mff.cuni.cz) (195.113.15.26) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 21 May 2007 13:17:52 +0000 Received: from sunsite.mff.cuni.cz (localhost.localdomain [127.0.0.1]) by sunsite.mff.cuni.cz (8.13.8/8.13.8) with ESMTP id l4LDJOGE023164; Mon, 21 May 2007 15:19:24 +0200 Received: (from jakub@localhost) by sunsite.mff.cuni.cz (8.13.8/8.13.8/Submit) id l4LDJNQp023161; Mon, 21 May 2007 15:19:23 +0200 Date: Mon, 21 May 2007 13:17:00 -0000 From: Jakub Jelinek To: Ulrich Drepper Cc: Glibc hackers Subject: [PATCH] Fix printf_unknown and do_positional (BZ #4514) Message-ID: <20070521131923.GD3081@sunsite.mff.cuni.cz> Reply-To: Jakub Jelinek Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.4.2.2i Mailing-List: contact libc-hacker-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-hacker-owner@sourceware.org X-SW-Source: 2007-05/txt/msg00017.txt.bz2 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 [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