Message ID | alpine.DEB.2.21.2007091806270.14941@digraph.polyomino.org.uk |
---|---|
State | New |
Headers | show |
Series | Fix memory leak in __printf_fp_l (bug 26215) | expand |
On 09/07/2020 15:07, Joseph Myers wrote: > __printf_fp_l has a memory leak in the case of some I/O errors, where > both buffer and wbuffer have been malloced but the handling of I/O > errors only frees wbuffer. This patch fixes this by moving the > declaration of buffer to an outer scope and ensuring that it is freed > when wbuffer is freed. > > Tested for x86_64 and x86. LGTM, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > --- > > This patch is relative to a tree with my previous fix for bug 26214 > <https://sourceware.org/pipermail/libc-alpha/2020-July/116043.html> > (pending review) applied. > > diff --git a/stdio-common/Makefile b/stdio-common/Makefile > index dc3fd38a19..8475fd1f09 100644 > --- a/stdio-common/Makefile > +++ b/stdio-common/Makefile > @@ -68,7 +68,8 @@ tests := tstscanf test_rdwr test-popen tstgetln test-fseek \ > scanf14a scanf16a \ > tst-printf-bz25691 \ > tst-vfprintf-width-prec-alloc \ > - tst-printf-fp-free > + tst-printf-fp-free \ > + tst-printf-fp-leak > > > test-srcs = tst-unbputc tst-printf tst-printfsz-islongdouble Ok. > @@ -80,12 +81,14 @@ tests-special += $(objpfx)tst-unbputc.out $(objpfx)tst-printf.out \ > $(objpfx)tst-vfprintf-width-prec-mem.out \ > $(objpfx)tst-printfsz-islongdouble.out \ > $(objpfx)tst-printf-bz25691-mem.out \ > - $(objpfx)tst-printf-fp-free-mem.out > + $(objpfx)tst-printf-fp-free-mem.out \ > + $(objpfx)tst-printf-fp-leak-mem.out > generated += tst-printf-bz18872.c tst-printf-bz18872.mtrace \ > tst-printf-bz18872-mem.out \ > tst-vfprintf-width-prec.mtrace tst-vfprintf-width-prec-mem.out \ > tst-printf-bz25691.mtrace tst-printf-bz25691-mem.out \ > - tst-printf-fp-free.mtrace tst-printf-fp-free-mem.out > + tst-printf-fp-free.mtrace tst-printf-fp-free-mem.out \ > + tst-printf-fp-leak.mtrace tst-printf-fp-leak-mem.out > endif > > tests-special += $(objpfx)tst-errno-manual.out > @@ -113,6 +116,8 @@ tst-printf-bz25691-ENV = \ > MALLOC_TRACE=$(objpfx)tst-printf-bz25691.mtrace > tst-printf-fp-free-ENV = \ > MALLOC_TRACE=$(objpfx)tst-printf-fp-free.mtrace > +tst-printf-fp-leak-ENV = \ > + MALLOC_TRACE=$(objpfx)tst-printf-fp-leak.mtrace > > $(objpfx)tst-unbputc.out: tst-unbputc.sh $(objpfx)tst-unbputc > $(SHELL) $< $(common-objpfx) '$(test-program-prefix)' > $@; \ Ok. > diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c > index 49c693575e..c0b79f2184 100644 > --- a/stdio-common/printf_fp.c > +++ b/stdio-common/printf_fp.c > @@ -72,7 +72,10 @@ > if (putc (outc, fp) == EOF) \ > { \ > if (buffer_malloced) \ > - free (wbuffer); \ > + { \ > + free (buffer); \ > + free (wbuffer); \ > + } \ > return -1; \ > } \ > ++done; \ > @@ -87,7 +90,10 @@ > if (PUT (fp, wide ? (const char *) wptr : ptr, outlen) != outlen) \ > { \ > if (buffer_malloced) \ > - free (wbuffer); \ > + { \ > + free (buffer); \ > + free (wbuffer); \ > + } \ > return -1; \ > } \ > ptr += outlen; \ > @@ -110,7 +116,10 @@ > if (PAD (fp, ch, len) != len) \ > { \ > if (buffer_malloced) \ > - free (wbuffer); \ > + { \ > + free (buffer); \ > + free (wbuffer); \ > + } \ > return -1; \ > } \ > done += len; \ Ok. > @@ -259,7 +268,8 @@ __printf_fp_l (FILE *fp, locale_t loc, > > /* Buffer in which we produce the output. */ > wchar_t *wbuffer = NULL; > - /* Flag whether wbuffer is malloc'ed or not. */ > + char *buffer = NULL; > + /* Flag whether wbuffer and buffer are malloc'ed or not. */ > int buffer_malloced = 0; > > p.expsign = 0; > @@ -1172,7 +1182,6 @@ __printf_fp_l (FILE *fp, locale_t loc, > PADN ('0', width); > > { > - char *buffer = NULL; > char *buffer_end = NULL; > char *cp = NULL; > char *tmpptr; > @@ -1252,6 +1261,7 @@ __printf_fp_l (FILE *fp, locale_t loc, > free (wbuffer); > /* Avoid a double free if the subsequent PADN encounters an > I/O error. */ > + buffer = NULL; > wbuffer = NULL; > } > } Ok. > diff --git a/stdio-common/tst-printf-fp-leak.c b/stdio-common/tst-printf-fp-leak.c > new file mode 100644 > index 0000000000..da11a486dd > --- /dev/null > +++ b/stdio-common/tst-printf-fp-leak.c > @@ -0,0 +1,34 @@ > +/* Test memory leak in __printf_fp_l (bug 26215). > + Copyright (C) 2020 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 > + <https://www.gnu.org/licenses/>. */ > + > +#include <mcheck.h> > +#include <stdio.h> > +#include <support/check.h> > + > +static int > +do_test (void) > +{ > + mtrace (); > + FILE *fp = fopen ("/dev/full", "w"); > + TEST_VERIFY_EXIT (fp != NULL); > + TEST_COMPARE (fprintf (fp, "%.65536f", 1.0), -1); > + fclose (fp); > + return 0; > +} > + > +#include <support/test-driver.c> > Ok.
* Joseph Myers: > __printf_fp_l has a memory leak in the case of some I/O errors, where > both buffer and wbuffer have been malloced but the handling of I/O > errors only frees wbuffer. This patch fixes this by moving the > declaration of buffer to an outer scope and ensuring that it is freed > when wbuffer is freed. Do we need to treat this as a securityh vulnerability? I don't think so, because there are no impacted applications as far as I can tell. Thanks, Florian
On Fri, 10 Jul 2020, Florian Weimer via Libc-alpha wrote: > * Joseph Myers: > > > __printf_fp_l has a memory leak in the case of some I/O errors, where > > both buffer and wbuffer have been malloced but the handling of I/O > > errors only frees wbuffer. This patch fixes this by moving the > > declaration of buffer to an outer scope and ensuring that it is freed > > when wbuffer is freed. > > Do we need to treat this as a securityh vulnerability? I don't think > so, because there are no impacted applications as far as I can tell. I don't know whether any of bugs 26201, 26211, 26214, 26215 (all memory issues in printf with valid but unusual format strings) should be considered as security issues.
diff --git a/stdio-common/Makefile b/stdio-common/Makefile index dc3fd38a19..8475fd1f09 100644 --- a/stdio-common/Makefile +++ b/stdio-common/Makefile @@ -68,7 +68,8 @@ tests := tstscanf test_rdwr test-popen tstgetln test-fseek \ scanf14a scanf16a \ tst-printf-bz25691 \ tst-vfprintf-width-prec-alloc \ - tst-printf-fp-free + tst-printf-fp-free \ + tst-printf-fp-leak test-srcs = tst-unbputc tst-printf tst-printfsz-islongdouble @@ -80,12 +81,14 @@ tests-special += $(objpfx)tst-unbputc.out $(objpfx)tst-printf.out \ $(objpfx)tst-vfprintf-width-prec-mem.out \ $(objpfx)tst-printfsz-islongdouble.out \ $(objpfx)tst-printf-bz25691-mem.out \ - $(objpfx)tst-printf-fp-free-mem.out + $(objpfx)tst-printf-fp-free-mem.out \ + $(objpfx)tst-printf-fp-leak-mem.out generated += tst-printf-bz18872.c tst-printf-bz18872.mtrace \ tst-printf-bz18872-mem.out \ tst-vfprintf-width-prec.mtrace tst-vfprintf-width-prec-mem.out \ tst-printf-bz25691.mtrace tst-printf-bz25691-mem.out \ - tst-printf-fp-free.mtrace tst-printf-fp-free-mem.out + tst-printf-fp-free.mtrace tst-printf-fp-free-mem.out \ + tst-printf-fp-leak.mtrace tst-printf-fp-leak-mem.out endif tests-special += $(objpfx)tst-errno-manual.out @@ -113,6 +116,8 @@ tst-printf-bz25691-ENV = \ MALLOC_TRACE=$(objpfx)tst-printf-bz25691.mtrace tst-printf-fp-free-ENV = \ MALLOC_TRACE=$(objpfx)tst-printf-fp-free.mtrace +tst-printf-fp-leak-ENV = \ + MALLOC_TRACE=$(objpfx)tst-printf-fp-leak.mtrace $(objpfx)tst-unbputc.out: tst-unbputc.sh $(objpfx)tst-unbputc $(SHELL) $< $(common-objpfx) '$(test-program-prefix)' > $@; \ diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c index 49c693575e..c0b79f2184 100644 --- a/stdio-common/printf_fp.c +++ b/stdio-common/printf_fp.c @@ -72,7 +72,10 @@ if (putc (outc, fp) == EOF) \ { \ if (buffer_malloced) \ - free (wbuffer); \ + { \ + free (buffer); \ + free (wbuffer); \ + } \ return -1; \ } \ ++done; \ @@ -87,7 +90,10 @@ if (PUT (fp, wide ? (const char *) wptr : ptr, outlen) != outlen) \ { \ if (buffer_malloced) \ - free (wbuffer); \ + { \ + free (buffer); \ + free (wbuffer); \ + } \ return -1; \ } \ ptr += outlen; \ @@ -110,7 +116,10 @@ if (PAD (fp, ch, len) != len) \ { \ if (buffer_malloced) \ - free (wbuffer); \ + { \ + free (buffer); \ + free (wbuffer); \ + } \ return -1; \ } \ done += len; \ @@ -259,7 +268,8 @@ __printf_fp_l (FILE *fp, locale_t loc, /* Buffer in which we produce the output. */ wchar_t *wbuffer = NULL; - /* Flag whether wbuffer is malloc'ed or not. */ + char *buffer = NULL; + /* Flag whether wbuffer and buffer are malloc'ed or not. */ int buffer_malloced = 0; p.expsign = 0; @@ -1172,7 +1182,6 @@ __printf_fp_l (FILE *fp, locale_t loc, PADN ('0', width); { - char *buffer = NULL; char *buffer_end = NULL; char *cp = NULL; char *tmpptr; @@ -1252,6 +1261,7 @@ __printf_fp_l (FILE *fp, locale_t loc, free (wbuffer); /* Avoid a double free if the subsequent PADN encounters an I/O error. */ + buffer = NULL; wbuffer = NULL; } } diff --git a/stdio-common/tst-printf-fp-leak.c b/stdio-common/tst-printf-fp-leak.c new file mode 100644 index 0000000000..da11a486dd --- /dev/null +++ b/stdio-common/tst-printf-fp-leak.c @@ -0,0 +1,34 @@ +/* Test memory leak in __printf_fp_l (bug 26215). + Copyright (C) 2020 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 + <https://www.gnu.org/licenses/>. */ + +#include <mcheck.h> +#include <stdio.h> +#include <support/check.h> + +static int +do_test (void) +{ + mtrace (); + FILE *fp = fopen ("/dev/full", "w"); + TEST_VERIFY_EXIT (fp != NULL); + TEST_COMPARE (fprintf (fp, "%.65536f", 1.0), -1); + fclose (fp); + return 0; +} + +#include <support/test-driver.c>