Fix memory leak in printf_positional
Commit Message
On Mon, Aug 31, 2015 at 3:41 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Sat, 29 Aug 2015, Paul Pluzhnikov wrote:
>
>> It doesn't take very long: all I needed is a printf invocation with >=
>> 65536 / 3 / sizeof(void*) arguments.
>>
>> Writing such invocation by hand is of course toublesome, plumbing
>> Makefile to generate it for me, and figuring out why it doesn't work
>> is what takes time :-(
>
> Normally such invocations would be generated with macros, e.g.
>
> #define A a, a, a, a, a, a, a, a, a, a
> #define B A, A, A, A, A, A, A, A, A, A
>
> etc., unless the sort of expansion you require is unsuited to that for
> some reason.
Well, to show the leak requires positional format, like this:
"%1$s %2$s ..."
However, I was able to use
"%1$s %s %s ..."
if I disable -Wformat for the test.
>> In addition, there is a GCC regression: compiling a printf call with
>> 2800 arguments takes 4.8.4-2ubuntu1~14.04 0.06s without optimization,
>> 0.86s with -O2. Same numbers for current GCC trunk (@r227321): 0.06s
>> and 4m46s. This is on a very recent and fast PC. I expect there could
>> be PCs in current use where the time will be 3x longer.
>
> To me this suggests building the test with -O0
That doesn't work: glibc requires to be built with optimization :-)
But #pragma GCC optimize does work.
The test case revealed an additional leak, so at least that effort was
not in vain.
Combined patch attached. Tested on Linux/x86_64.
Thanks,
2015-09-01 Paul Eggert <eggert@cs.ucla.edu>
Paul Pluzhnikov <ppluzhnikov@google.com>
[BZ #18872]
* stdio-common/Makefile (tst-printf-bz18872): New test.
(tst-printf-bz18872-mem.out): Likewise.
* stdio-common/tst-printf-bz18872.c: New test.
* stdio-common/vfprintf.c: Fix memory leaks.
Comments
Paul Pluzhnikov <ppluzhnikov@google.com> writes:
> However, I was able to use
>
> "%1$s %s %s ..."
>
> if I disable -Wformat for the test.
You should probably add a comment that you are relying on undefined
behaviour here.
Andreas.
On Tue, 1 Sep 2015, Paul Pluzhnikov wrote:
> Well, to show the leak requires positional format, like this:
>
> "%1$s %2$s ..."
>
> However, I was able to use
>
> "%1$s %s %s ..."
That's may be simplest if we think glibc should not have leaks for this
usage. It's possible to produce examples with positional formats using
macros if desired, e.g.:
#define str1(x) #x
#define str(x) str1(x)
#define strc(a,b,c) str(a##b##c)
#define e(x,y,z) "%"strc(x,y,z)"$s "
#define f(x,y) e(x,y,0) e(x,y,1) e(x,y,2) e(x,y,3) e(x,y,4) e(x,y,5) e(x,y,6) e(x,y,7) e(x,y,8) e(x,y,9)
#define g(x) f(x,0) f(x,1) f(x,2) f(x,3) f(x,4) f(x,5) f(x,6) f(x,7) f(x,8) f(x,9)
g(0) g(1) g(2) g(3) g(4) g(5) g(6) g(7) g(8) g(9)
(that example produces numbers with leading 0s, but you can produce
numbers with 1, 2, 3 and 4 digits separately to avoid that).
On Wed, Sep 2, 2015 at 3:07 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Tue, 1 Sep 2015, Paul Pluzhnikov wrote:
> #define str1(x) #x
> #define str(x) str1(x)
> #define strc(a,b,c) str(a##b##c)
> #define e(x,y,z) "%"strc(x,y,z)"$s "
> #define f(x,y) e(x,y,0) e(x,y,1) e(x,y,2) e(x,y,3) e(x,y,4) e(x,y,5) e(x,y,6) e(x,y,7) e(x,y,8) e(x,y,9)
> #define g(x) f(x,0) f(x,1) f(x,2) f(x,3) f(x,4) f(x,5) f(x,6) f(x,7) f(x,8) f(x,9)
> g(0) g(1) g(2) g(3) g(4) g(5) g(6) g(7) g(8) g(9)
>
> (that example produces numbers with leading 0s, but you can produce
> numbers with 1, 2, 3 and 4 digits separately to avoid that).
I think at that point it becomes easier to just use a generator script
to write the test. Any objection to that?
Thanks,
On Wed, 2 Sep 2015, Paul Pluzhnikov wrote:
> I think at that point it becomes easier to just use a generator script
> to write the test. Any objection to that?
I don't object to that given an appropriate comment on why it is being
used.
On 09/02/2015 10:28 AM, Joseph Myers wrote:
> On Wed, 2 Sep 2015, Paul Pluzhnikov wrote:
>
>> I think at that point it becomes easier to just use a generator script
>> to write the test. Any objection to that?
>
> I don't object to that given an appropriate comment on why it is being
> used.
Agreed.
c.
@@ -57,17 +57,23 @@ tests := tstscanf test_rdwr test-popen tstgetln test-fseek \
bug19 bug19a tst-popen2 scanf13 scanf14 scanf15 bug20 bug21 bug22 \
scanf16 scanf17 tst-setvbuf1 tst-grouping bug23 bug24 \
bug-vfprintf-nargs tst-long-dbl-fphex tst-fphex-wide tst-sprintf3 \
- bug25 tst-printf-round bug23-2 bug23-3 bug23-4 bug26 tst-fmemopen3
+ bug25 tst-printf-round bug23-2 bug23-3 bug23-4 bug26 tst-fmemopen3 \
+ tst-printf-bz18872
test-srcs = tst-unbputc tst-printf
ifeq ($(run-built-tests),yes)
tests-special += $(objpfx)tst-unbputc.out $(objpfx)tst-printf.out \
+ $(objpfx)tst-printf-bz18872-mem.out \
$(objpfx)tst-setvbuf1-cmp.out
+generated += tst-printf-bz18872.mtrace tst-printf-bz18872-mem.out
endif
include ../Rules
+tst-printf-bz18872-ENV = MALLOC_TRACE=$(objpfx)tst-printf-bz18872.mtrace
+CFLAGS-tst-printf-bz18872.c += -Wno-format
+
ifeq ($(run-built-tests),yes)
$(objpfx)tst-unbputc.out: tst-unbputc.sh $(objpfx)tst-unbputc
$(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \
@@ -76,6 +82,10 @@ $(objpfx)tst-unbputc.out: tst-unbputc.sh $(objpfx)tst-unbputc
$(objpfx)tst-printf.out: tst-printf.sh $(objpfx)tst-printf
$(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \
$(evaluate-test)
+
+$(objpfx)tst-printf-bz18872-mem.out: $(objpfx)tst-printf-bz18872.out
+ $(common-objpfx)malloc/mtrace $(objpfx)tst-printf-bz18872.mtrace > $@; \
+ $(evaluate-test)
endif
CFLAGS-vfprintf.c = -Wno-uninitialized
new file mode 100644
@@ -0,0 +1,47 @@
+/* Copyright (C) 1991-2015 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <mcheck.h>
+
+/*
+ Compile do_bz18872 without optimization: GCC 4.9/5.0/6.0 takes a long time
+ to build this source. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67396 */
+#pragma GCC push_options
+#pragma GCC optimize ("-O0")
+
+static int
+do_bz18872 (void)
+{
+ mtrace ();
+
+#define A10 "a", "a", "a", "a", "a", "a", "a", "a", "a", "a",
+#define S10 "%s" "%s" "%s" "%s" "%s" "%s" "%s" "%s" "%s" "%s"
+#define X10(a) a a a a a a a a a a
+
+ printf ("%1$s" X10(X10(X10(S10))) "%s", X10(X10(X10(A10))) "\n");
+
+#undef A10
+#undef S10
+#undef X10
+ return 0;
+}
+
+#pragma GCC pop_options
+
+#define TEST_FUNCTION do_bz18872 ()
+#include "../test-skeleton.c"
@@ -2091,6 +2091,10 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
- specs[nspecs_done].end_of_fmt);
}
all_done:
+ if (__glibc_unlikely (specs_malloced))
+ free (specs);
+ if (__glibc_unlikely (args_malloced != NULL))
+ free (args_malloced);
if (__glibc_unlikely (workstart != NULL))
free (workstart);
return done;