This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] libio: Free backup area when it not required (BZ#22415)
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Tue, 12 Dec 2017 15:04:41 -0200
- Subject: Re: [PATCH] libio: Free backup area when it not required (BZ#22415)
- Authentication-results: sourceware.org; auth=none
- References: <1511962793-14533-1-git-send-email-adhemerval.zanella@linaro.org>
Ping.
On 29/11/2017 11:39, Adhemerval Zanella wrote:
> Some libio operations fail to correctly free the backup area (created
> by _IO_{w}default_pbackfail on unget{w}c) resulting in either invalid
> buffer free operations or memory leaks.
>
> For instance, on the example provided by BZ#22415 a following
> fputc after a fseek to rewind the stream issues an invalid free on
> the buffer. It is because although _IO_file_overflow correctly
> (from fputc) correctly calls _IO_free_backup_area, the
> _IO_new_file_seekoff (called by fseek) updates the FILE internal
> pointers without first free the backup area (resulting in invalid
> values in the internal pointers).
>
> The wide version also shows an issue, but instead of accessing invalid
> pointers it leaks the backup memory on fseek/fputwc operation.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
>
> * libio/Makefile (tests): Add tst-bz22415.
> (tst-bz22415-ENV): New rule.
> (generated): Add tst-bz22415.mtrace and tst-bz22415.check.
> (tests-special): Add tst-bz22415-mem.out.
> ($(objpfx)tst-bz22415-mem.out): New rule.
> * libio/fileops.c (_IO_new_file_seekoff): Call _IO_free_backup_area
> in case of a successful seek operation.
> * libio/wfileops.c (_IO_wfile_seekoff): Likewise.
> (_IO_wfile_overflow): Call _IO_free_wbackup_area in case a write
> buffer is required.
> * libio/tst-bz22415.c: New test.
> ---
> ChangeLog | 14 ++++++++
> libio/Makefile | 11 ++++--
> libio/fileops.c | 3 ++
> libio/tst-bz22415.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> libio/wfileops.c | 4 +++
> 5 files changed, 127 insertions(+), 2 deletions(-)
> create mode 100644 libio/tst-bz22415.c
>
> diff --git a/libio/Makefile b/libio/Makefile
> index 9d09bd8..70b5530 100644
> --- a/libio/Makefile
> +++ b/libio/Makefile
> @@ -62,7 +62,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \
> bug-memstream1 bug-wmemstream1 \
> tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \
> tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \
> - tst-ftell-append tst-fputws
> + tst-ftell-append tst-fputws tst-bz22415
> ifeq (yes,$(build-shared))
> # Add test-fopenloc only if shared library is enabled since it depends on
> # shared localedata objects.
> @@ -151,9 +151,11 @@ tst_wprintf2-ARGS = "Some Text"
>
> test-fmemopen-ENV = MALLOC_TRACE=$(objpfx)test-fmemopen.mtrace
> tst-fopenloc-ENV = MALLOC_TRACE=$(objpfx)tst-fopenloc.mtrace
> +tst-bz22415-ENV = MALLOC_TRACE=$(objpfx)tst-bz22415.mtrace
>
> generated += test-fmemopen.mtrace test-fmemopen.check
> generated += tst-fopenloc.mtrace tst-fopenloc.check
> +generated += tst-bz22415.mtrace tst-bz22415.check
>
> aux := fileops genops stdfiles stdio strops
>
> @@ -167,7 +169,8 @@ shared-only-routines = oldiofopen oldiofdopen oldiofclose oldfileops \
> oldiofsetpos64
>
> ifeq ($(run-built-tests),yes)
> -tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-mem.out
> +tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-mem.out \
> + $(objpfx)tst-bz22415-mem.out
> ifeq (yes,$(build-shared))
> # Run tst-fopenloc-cmp.out and tst-openloc-mem.out only if shared
> # library is enabled since they depend on tst-fopenloc.out.
> @@ -218,3 +221,7 @@ $(objpfx)test-fmemopen-mem.out: $(objpfx)test-fmemopen.out
> $(objpfx)tst-fopenloc-mem.out: $(objpfx)tst-fopenloc.out
> $(common-objpfx)malloc/mtrace $(objpfx)tst-fopenloc.mtrace > $@; \
> $(evaluate-test)
> +
> +$(objpfx)tst-bz22415-mem.out: $(objpfx)tst-bz22415.out
> + $(common-objpfx)malloc/mtrace $(objpfx)tst-bz22415.mtrace > $@; \
> + $(evaluate-test)
> diff --git a/libio/fileops.c b/libio/fileops.c
> index a9e948f..72a0cb5 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -994,6 +994,9 @@ _IO_new_file_seekoff (_IO_FILE *fp, _IO_off64_t offset, int dir, int mode)
> goto dumb;
> }
> }
> +
> + _IO_free_backup_area (fp);
> +
> /* At this point, dir==_IO_seek_set. */
>
> /* If destination is within current buffer, optimize: */
> diff --git a/libio/tst-bz22415.c b/libio/tst-bz22415.c
> new file mode 100644
> index 0000000..d7b23fe
> --- /dev/null
> +++ b/libio/tst-bz22415.c
> @@ -0,0 +1,97 @@
> +/* Check static buffer handling with setvbuf (BZ #22415)
> +
> + Copyright (C) 2017 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/>. */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <wchar.h>
> +#include <mcheck.h>
> +
> +#include <support/check.h>
> +#include <support/temp_file.h>
> +
> +static int
> +do_test (void)
> +{
> + mtrace ();
> +
> + char *temp_file;
> + TEST_VERIFY_EXIT (create_temp_file ("tst-bz22145.", &temp_file));
> +
> + char buf[BUFSIZ];
> +
> + {
> + /* Check if backup buffer is correctly freed and changing back
> + to normal buffer does not trigger an invalid free in case of
> + static buffer set by setvbuf. */
> +
> + FILE *f = fopen (temp_file, "w+b");
> + TEST_VERIFY_EXIT (f != NULL);
> +
> + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0);
> + TEST_VERIFY_EXIT (ungetc ('x', f) == 'x');
> + TEST_VERIFY_EXIT (fseek (f, 0L, SEEK_SET) == 0);
> + TEST_VERIFY_EXIT (fputc ('y', f) == 'y');
> +
> + TEST_VERIFY_EXIT (fclose (f) == 0);
> + }
> +
> + {
> + /* Check if backup buffer is correctly freed and changing back
> + to normal buffer does not trigger an invalid free in case of
> + static buffer set by setvbuf. */
> +
> + FILE *f = fopen (temp_file, "w+b");
> + TEST_VERIFY_EXIT (f != NULL);
> +
> + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0);
> + TEST_VERIFY_EXIT (ungetc ('x', f) == 'x');
> + TEST_VERIFY_EXIT (fputc ('y', f) == 'y');
> +
> + TEST_VERIFY_EXIT (fclose (f) == 0);
> + }
> +
> + {
> + FILE *f = fopen (temp_file, "w+b");
> + TEST_VERIFY_EXIT (f != NULL);
> +
> + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0);
> + TEST_VERIFY_EXIT (ungetwc (L'x', f) == L'x');
> + TEST_VERIFY_EXIT (fseek (f, 0L, SEEK_SET) == 0);
> + TEST_VERIFY_EXIT (fputwc (L'y', f) == L'y');
> +
> + TEST_VERIFY_EXIT (fclose (f) == 0);
> + }
> +
> + {
> + FILE *f = fopen (temp_file, "w+b");
> + TEST_VERIFY_EXIT (f != NULL);
> +
> + TEST_VERIFY_EXIT (setvbuf (f, buf, _IOFBF, BUFSIZ) == 0);
> + TEST_VERIFY_EXIT (ungetwc (L'x', f) == L'x');
> + TEST_VERIFY_EXIT (fputwc (L'y', f) == L'y');
> +
> + TEST_VERIFY_EXIT (fclose (f) == 0);
> + }
> +
> + free (temp_file);
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/libio/wfileops.c b/libio/wfileops.c
> index 8756b6f..91139e1 100644
> --- a/libio/wfileops.c
> +++ b/libio/wfileops.c
> @@ -421,6 +421,7 @@ _IO_wfile_overflow (_IO_FILE *f, wint_t wch)
> if (f->_wide_data->_IO_write_base == 0)
> {
> _IO_wdoallocbuf (f);
> + _IO_free_wbackup_area (f);
> _IO_wsetg (f, f->_wide_data->_IO_buf_base,
> f->_wide_data->_IO_buf_base, f->_wide_data->_IO_buf_base);
>
> @@ -850,6 +851,9 @@ _IO_wfile_seekoff (_IO_FILE *fp, _IO_off64_t offset, int dir, int mode)
> goto dumb;
> }
> }
> +
> + _IO_free_wbackup_area (fp);
> +
> /* At this point, dir==_IO_seek_set. */
>
> /* If destination is within current buffer, optimize: */
>