diff mbox

Fix memory leak in printf_positional

Message ID CALoOobO2iV9hPNB_S7PDoL=cWVCPrSkKGaUQfqboxUg40fsrVw@mail.gmail.com
State New
Headers show

Commit Message

Paul Pluzhnikov Sept. 2, 2015, 1:35 a.m. UTC 
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

Andreas Schwab Sept. 2, 2015, 7:07 a.m. UTC | #1 
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.
Joseph Myers Sept. 2, 2015, 10:07 a.m. UTC | #2 
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).
Paul Pluzhnikov Sept. 2, 2015, 2:12 p.m. UTC | #3 
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,
Joseph Myers Sept. 2, 2015, 2:28 p.m. UTC | #4 
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.
Carlos O'Donell Sept. 2, 2015, 5:39 p.m. UTC | #5 
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.
diff mbox

Patch

diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index d0bf0e1..ad2c8a3 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -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
diff --git a/stdio-common/tst-printf-bz18872.c b/stdio-common/tst-printf-bz18872.c
new file mode 100644
index 0000000..c5f3513
--- /dev/null
+++ b/stdio-common/tst-printf-bz18872.c
@@ -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"
diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index 0592e70..45c4779 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.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;