Fix memory leak in printf_positional
diff mbox

Message ID CALoOobMusa6usQCEAR2bs8ES2EbHRa9YuCARd9=ewq1eROPLmQ@mail.gmail.com
State New
Headers show

Commit Message

Paul Pluzhnikov Sept. 3, 2015, 5:48 a.m. UTC
On Wed, Sep 2, 2015 at 10:39 AM, Carlos O'Donell <carlos@redhat.com> 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.

What's the appropriate place to put this comment?

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.sh: Generate new test.
        * stdio-common/vfprintf.c: Fix memory leaks.

Comments

Andreas Schwab Sept. 3, 2015, 7:23 a.m. UTC | #1
Paul Pluzhnikov <ppluzhnikov@google.com> writes:

> @@ -76,6 +82,13 @@ $(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.c: tst-printf-bz18872.sh
> +	rm -f $@
> +	$(BASH) $^ > $@

Please make that atomic by using a temporary file.

> +for j in $(seq 1 $n_args); do
> +  if [[ $(($j % 10)) == 1 ]]; then printf "\n"; fi
> +  printf '"%%%d$s" ' $j
> +done
> +
> +printf ' "%%%d$s",' $(($n_args + 1))
> +
> +for j in $(seq 1 $n_args); do
> +  if [[ $(($j % 10)) == 1 ]]; then printf "\n"; fi
> +  printf '"a", '
> +done

Since you are already using bash you can also use `for ((j = 1; j <=
n_args; j++))'.

Andreas.
Joseph Myers Sept. 3, 2015, 11:24 a.m. UTC | #2
On Wed, 2 Sep 2015, Paul Pluzhnikov wrote:

> On Wed, Sep 2, 2015 at 10:39 AM, Carlos O'Donell <carlos@redhat.com> 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.
> 
> What's the appropriate place to put this comment?

I'd suggest in the Makefile.

> +# Copyright (C) 1999-2015 Free Software Foundation, Inc.

This is a new test; I'd expect just 2015 unless closely based on an 
existing test.
Carlos O'Donell Sept. 3, 2015, 12:47 p.m. UTC | #3
On 09/03/2015 01:48 AM, Paul Pluzhnikov wrote:
> On Wed, Sep 2, 2015 at 10:39 AM, Carlos O'Donell <carlos@redhat.com> 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.
> 
> What's the appropriate place to put this comment?
> 
> 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.sh: Generate new test.
>         * stdio-common/vfprintf.c: Fix memory leaks.

This looks good to me. The comment in tst-printf-bz18872.sh is sufficient for me.

Cheers,
Carlos.
Paul Pluzhnikov Sept. 3, 2015, 2:16 p.m. UTC | #4
On Thu, Sep 3, 2015 at 12:23 AM, Andreas Schwab <schwab@suse.de> wrote:
> Paul Pluzhnikov <ppluzhnikov@google.com> writes:

>> +$(objpfx)tst-printf-bz18872.c: tst-printf-bz18872.sh
>> +     rm -f $@
>> +     $(BASH) $^ > $@
>
> Please make that atomic by using a temporary file.

Sorry, I didn't understand that comment.

Do you mean that I should have a rule like this:

$(objpfx)tst-printf-bz18872.out: tst-printf-bz18872.sh
        $(BASH) $^  | $(CC) ... -o $(objpfx)tst-printf-bz18872 -xc -
        $(test-something-or-other)

or something else?
(Figuring out the commands for that rule gives me a pause.)

Thanks,
Andreas Schwab Sept. 3, 2015, 2:38 p.m. UTC | #5
Paul Pluzhnikov <ppluzhnikov@google.com> writes:

> On Thu, Sep 3, 2015 at 12:23 AM, Andreas Schwab <schwab@suse.de> wrote:
>> Paul Pluzhnikov <ppluzhnikov@google.com> writes:
>
>>> +$(objpfx)tst-printf-bz18872.c: tst-printf-bz18872.sh
>>> +     rm -f $@
>>> +     $(BASH) $^ > $@
>>
>> Please make that atomic by using a temporary file.
>
> Sorry, I didn't understand that comment.

The target must always be either complete or absent.  A redirection is
not atomic.

Andreas.
Paul Pluzhnikov Sept. 3, 2015, 2:44 p.m. UTC | #6
On Thu, Sep 3, 2015 at 7:38 AM, Andreas Schwab <schwab@suse.de> wrote:

>> Sorry, I didn't understand that comment.
>
> The target must always be either complete or absent.  A redirection is
> not atomic.

Understood.

Thanks,

Patch
diff mbox

diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index d0bf0e1..d91533f 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.c tst-printf-bz18872.mtrace \
+	     tst-printf-bz18872-mem.out
 endif
 
 include ../Rules
 
+tst-printf-bz18872-ENV = MALLOC_TRACE=$(objpfx)tst-printf-bz18872.mtrace
+
 ifeq ($(run-built-tests),yes)
 $(objpfx)tst-unbputc.out: tst-unbputc.sh $(objpfx)tst-unbputc
 	$(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \
@@ -76,6 +82,13 @@  $(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.c: tst-printf-bz18872.sh
+	rm -f $@
+	$(BASH) $^ > $@
+$(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.sh b/stdio-common/tst-printf-bz18872.sh
new file mode 100755
index 0000000..75ec92f
--- /dev/null
+++ b/stdio-common/tst-printf-bz18872.sh
@@ -0,0 +1,66 @@ 
+#!/bin/bash
+# Copyright (C) 1999-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/>.
+
+# To test BZ #18872, we need a printf() with 10K arguments.
+# Such a printf could be generated with non-trivial macro
+# application, but it's simpler to generate the test source
+# via this script.
+
+n_args=10000
+
+cat <<'EOF'
+#include <stdio.h>
+#include <mcheck.h>
+
+/*
+  Compile do_test 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")
+
+int do_test (void)
+{
+    mtrace ();
+    printf (
+EOF
+
+for j in $(seq 1 $n_args); do
+  if [[ $(($j % 10)) == 1 ]]; then printf "\n"; fi
+  printf '"%%%d$s" ' $j
+done
+
+printf ' "%%%d$s",' $(($n_args + 1))
+
+for j in $(seq 1 $n_args); do
+  if [[ $(($j % 10)) == 1 ]]; then printf "\n"; fi
+  printf '"a", '
+done
+
+printf '"\\n");'
+
+
+cat <<'EOF'
+
+  return 0;
+}
+#pragma GCC pop_options
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
+
+EOF
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;