Re: Fix memory leak in __printf_fp_l (bug 26215)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

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.