[PATCH 3/3] ungetc: Fix backup buffer leak on program exit [BZ #27821]

[Carlos O'Donell]

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>

-- 
Cheers,
Carlos.