| Message ID | 20240814233534.1469084-4-siddhesh@sourceware.org |
|---|---|
| State | New |
| Headers | show |
| Series | ungetc fixes for BZ #27821 | expand |
On Aug 14 2024, Siddhesh Poyarekar wrote:
> + /* Free up the backup area if it ever was ever allocated. */
One of "ever" is enogh.
On 2024-08-15 03:17, Andreas Schwab wrote: > On Aug 14 2024, Siddhesh Poyarekar wrote: > >> + /* Free up the backup area if it ever was ever allocated. */ > > One of "ever" is enogh. > Fixed locally. Is the change OK otherwise? Thanks, Sid
On 8/14/24 7:35 PM, Siddhesh Poyarekar wrote: > If a file descriptor is left unclosed and is cleaned up by _IO_cleanup > on exit, its backup buffer remains unfreed, registering as a leak in > valgrind. This is not strictly an issue since (1) the program should > ideally be closing the stream once it's not in use and (2) the program > is about to exit anyway, so keeping the backup buffer around a wee bit > longer isn't a real problem. Free it anyway to keep valgrind happy > when the streams in question are the standard ones, i.e. stdout, stdin > or stderr. > > Also, the _IO_have_backup macro checks for _IO_save_base, > which is a roundabout way to check for a backup buffer instead of > directly looking for _IO_backup_base. The roundabout check breaks when > the main get area has not been used and user pushes a char into the > backup buffer with ungetc. Fix this to use the _IO_backup_base > directly. > > Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> LGTM, you can keep my RB if you fix tab vs. spaces. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > --- > libio/genops.c | 6 ++++++ > libio/libioP.h | 4 ++-- > stdio-common/Makefile | 7 +++++++ > stdio-common/tst-ungetc-leak.c | 32 ++++++++++++++++++++++++++++++++ > 4 files changed, 47 insertions(+), 2 deletions(-) > create mode 100644 stdio-common/tst-ungetc-leak.c > > diff --git a/libio/genops.c b/libio/genops.c > index b012fa33d2..6ea95c5e68 100644 > --- a/libio/genops.c > +++ b/libio/genops.c > @@ -816,6 +816,12 @@ _IO_unbuffer_all (void) > legacy = 1; OK. In _IO_unbuffer_all. > #endif > > + /* Free up the backup area if it ever was ever allocated. */ > + if (_IO_have_backup (fp)) > + _IO_free_backup_area (fp); Fix tab vs. spaces. > + if (fp->_mode > 0 && _IO_have_wbackup (fp)) > + _IO_free_wbackup_area (fp); OK. > + > if (! (fp->_flags & _IO_UNBUFFERED) > /* Iff stream is un-orientated, it wasn't used. */ > && (legacy || fp->_mode != 0)) > diff --git a/libio/libioP.h b/libio/libioP.h > index 1af287b19f..616253fcd0 100644 > --- a/libio/libioP.h > +++ b/libio/libioP.h > @@ -577,8 +577,8 @@ extern void _IO_old_init (FILE *fp, int flags) __THROW; > ((__fp)->_wide_data->_IO_write_base \ > = (__fp)->_wide_data->_IO_write_ptr = __p, \ > (__fp)->_wide_data->_IO_write_end = (__ep)) > -#define _IO_have_backup(fp) ((fp)->_IO_save_base != NULL) > -#define _IO_have_wbackup(fp) ((fp)->_wide_data->_IO_save_base != NULL) > +#define _IO_have_backup(fp) ((fp)->_IO_backup_base != NULL) > +#define _IO_have_wbackup(fp) ((fp)->_wide_data->_IO_backup_base != NULL) OK. Agreed. > #define _IO_in_backup(fp) ((fp)->_flags & _IO_IN_BACKUP) > #define _IO_have_markers(fp) ((fp)->_markers != NULL) > #define _IO_blen(fp) ((fp)->_IO_buf_end - (fp)->_IO_buf_base) > diff --git a/stdio-common/Makefile b/stdio-common/Makefile > index e4f0146d2c..a91754f52d 100644 > --- a/stdio-common/Makefile > +++ b/stdio-common/Makefile > @@ -254,6 +254,7 @@ tests := \ > tst-swscanf \ > tst-tmpnam \ > tst-ungetc \ > + tst-ungetc-leak \ > tst-unlockedio \ > tst-vfprintf-mbs-prec \ > tst-vfprintf-user-type \ > @@ -316,6 +317,7 @@ tests-special += \ > $(objpfx)tst-printf-bz25691-mem.out \ > $(objpfx)tst-printf-fp-free-mem.out \ > $(objpfx)tst-printf-fp-leak-mem.out \ > + $(objpfx)tst-ungetc-leak-mem.out \ > $(objpfx)tst-vfprintf-width-prec-mem.out \ > # tests-special > > @@ -330,6 +332,8 @@ generated += \ > tst-printf-fp-leak-mem.out \ > tst-printf-fp-leak.mtrace \ > tst-scanf-bz27650.mtrace \ > + tst-ungetc-leak-mem.out \ > + tst-ungetc-leak.mtrace \ > tst-vfprintf-width-prec-mem.out \ > tst-vfprintf-width-prec.mtrace \ > # generated > @@ -424,6 +428,9 @@ tst-printf-fp-leak-ENV = \ > tst-scanf-bz27650-ENV = \ > MALLOC_TRACE=$(objpfx)tst-scanf-bz27650.mtrace \ > LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so > +tst-ungetc-leak-ENV = \ > + MALLOC_TRACE=$(objpfx)tst-ungetc-leak.mtrace \ > + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so OK. > > $(objpfx)tst-unbputc.out: tst-unbputc.sh $(objpfx)tst-unbputc > $(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \ > diff --git a/stdio-common/tst-ungetc-leak.c b/stdio-common/tst-ungetc-leak.c > new file mode 100644 > index 0000000000..6c5152b43f > --- /dev/null > +++ b/stdio-common/tst-ungetc-leak.c > @@ -0,0 +1,32 @@ > +/* Test for memory leak with ungetc when stream is unused. > + Copyright The GNU Toolchain Authors. > + 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 <stdio.h> > +#include <mcheck.h> > +#include <support/check.h> > +#include <support/support.h> > + > +static int > +do_test (void) > +{ > + mtrace (); > + TEST_COMPARE (ungetc('y', stdin), 'y'); > + return 0; > +} > + > +#include <support/test-driver.c>
diff --git a/libio/genops.c b/libio/genops.c index b012fa33d2..6ea95c5e68 100644 --- a/libio/genops.c +++ b/libio/genops.c @@ -816,6 +816,12 @@ _IO_unbuffer_all (void) legacy = 1; #endif + /* Free up the backup area if it ever was ever allocated. */ + if (_IO_have_backup (fp)) + _IO_free_backup_area (fp); + if (fp->_mode > 0 && _IO_have_wbackup (fp)) + _IO_free_wbackup_area (fp); + if (! (fp->_flags & _IO_UNBUFFERED) /* Iff stream is un-orientated, it wasn't used. */ && (legacy || fp->_mode != 0)) diff --git a/libio/libioP.h b/libio/libioP.h index 1af287b19f..616253fcd0 100644 --- a/libio/libioP.h +++ b/libio/libioP.h @@ -577,8 +577,8 @@ extern void _IO_old_init (FILE *fp, int flags) __THROW; ((__fp)->_wide_data->_IO_write_base \ = (__fp)->_wide_data->_IO_write_ptr = __p, \ (__fp)->_wide_data->_IO_write_end = (__ep)) -#define _IO_have_backup(fp) ((fp)->_IO_save_base != NULL) -#define _IO_have_wbackup(fp) ((fp)->_wide_data->_IO_save_base != NULL) +#define _IO_have_backup(fp) ((fp)->_IO_backup_base != NULL) +#define _IO_have_wbackup(fp) ((fp)->_wide_data->_IO_backup_base != NULL) #define _IO_in_backup(fp) ((fp)->_flags & _IO_IN_BACKUP) #define _IO_have_markers(fp) ((fp)->_markers != NULL) #define _IO_blen(fp) ((fp)->_IO_buf_end - (fp)->_IO_buf_base) diff --git a/stdio-common/Makefile b/stdio-common/Makefile index e4f0146d2c..a91754f52d 100644 --- a/stdio-common/Makefile +++ b/stdio-common/Makefile @@ -254,6 +254,7 @@ tests := \ tst-swscanf \ tst-tmpnam \ tst-ungetc \ + tst-ungetc-leak \ tst-unlockedio \ tst-vfprintf-mbs-prec \ tst-vfprintf-user-type \ @@ -316,6 +317,7 @@ tests-special += \ $(objpfx)tst-printf-bz25691-mem.out \ $(objpfx)tst-printf-fp-free-mem.out \ $(objpfx)tst-printf-fp-leak-mem.out \ + $(objpfx)tst-ungetc-leak-mem.out \ $(objpfx)tst-vfprintf-width-prec-mem.out \ # tests-special @@ -330,6 +332,8 @@ generated += \ tst-printf-fp-leak-mem.out \ tst-printf-fp-leak.mtrace \ tst-scanf-bz27650.mtrace \ + tst-ungetc-leak-mem.out \ + tst-ungetc-leak.mtrace \ tst-vfprintf-width-prec-mem.out \ tst-vfprintf-width-prec.mtrace \ # generated @@ -424,6 +428,9 @@ tst-printf-fp-leak-ENV = \ tst-scanf-bz27650-ENV = \ MALLOC_TRACE=$(objpfx)tst-scanf-bz27650.mtrace \ LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so +tst-ungetc-leak-ENV = \ + MALLOC_TRACE=$(objpfx)tst-ungetc-leak.mtrace \ + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so $(objpfx)tst-unbputc.out: tst-unbputc.sh $(objpfx)tst-unbputc $(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \ diff --git a/stdio-common/tst-ungetc-leak.c b/stdio-common/tst-ungetc-leak.c new file mode 100644 index 0000000000..6c5152b43f --- /dev/null +++ b/stdio-common/tst-ungetc-leak.c @@ -0,0 +1,32 @@ +/* Test for memory leak with ungetc when stream is unused. + Copyright The GNU Toolchain Authors. + 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 <stdio.h> +#include <mcheck.h> +#include <support/check.h> +#include <support/support.h> + +static int +do_test (void) +{ + mtrace (); + TEST_COMPARE (ungetc('y', stdin), 'y'); + return 0; +} + +#include <support/test-driver.c>
If a file descriptor is left unclosed and is cleaned up by _IO_cleanup on exit, its backup buffer remains unfreed, registering as a leak in valgrind. This is not strictly an issue since (1) the program should ideally be closing the stream once it's not in use and (2) the program is about to exit anyway, so keeping the backup buffer around a wee bit longer isn't a real problem. Free it anyway to keep valgrind happy when the streams in question are the standard ones, i.e. stdout, stdin or stderr. Also, the _IO_have_backup macro checks for _IO_save_base, which is a roundabout way to check for a backup buffer instead of directly looking for _IO_backup_base. The roundabout check breaks when the main get area has not been used and user pushes a char into the backup buffer with ungetc. Fix this to use the _IO_backup_base directly. Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> --- libio/genops.c | 6 ++++++ libio/libioP.h | 4 ++-- stdio-common/Makefile | 7 +++++++ stdio-common/tst-ungetc-leak.c | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 stdio-common/tst-ungetc-leak.c