[v1] libio: Fix crash in fputws [BZ #20632]
Checks
Commit Message
This fixes a buffer overflow in wide character string output,
reproducing when output fails, such as if the output fd is closed.
Wide character output data attempts to maintain the invariant that
`_IO_buf_base <= _IO_write_base <= _IO_write_end <= _IO_buf_end`
(that is, that the write region is a sub-region of `_IO_buf`).
Prior to this commit, this invariant is violated by the
`_IO_wfile_overflow` function as so:
1. `_IO_wsetg` is called, assigning `_IO_write_base` to `_IO_buf_base`
2. `_IO_setg` is called, assigning `_IO_buf_base` to a malloc’d buffer.
Thus the invariant is violated. The fix is simply to reverse the order:
malloc the `_IO_buf` first and then assign `_IO_write_base` to it.
We also take this opportunity to defensively guard the initialization of
the number of unwritten characters via pointer arithmetic. We now check
that the buffer end is not before the buffer beginning; this matches a
similar defensive check in the narrow analogue `fileops.c`.
Add a test which fails without the fix.
Fixes https://sourceware.org/bugzilla/show_bug.cgi?id=20632
Signed-off-by: Peter Ammon <corydoras@ridiculousfish.com>
---
libio/wfileops.c | 10 +++--
string/Makefile | 1 +
string/test-fputs-regression-20632.c | 60 +++++++++++++++++++++++++++
wcsmbs/Makefile | 1 +
wcsmbs/test-fputws-regression-20632.c | 20 +++++++++
5 files changed, 88 insertions(+), 4 deletions(-)
create mode 100644 string/test-fputs-regression-20632.c
create mode 100644 wcsmbs/test-fputws-regression-20632.c
--
2.46.0
Comments
* Peter Ammon:
> This fixes a buffer overflow in wide character string output,
> reproducing when output fails, such as if the output fd is closed.
>
> Wide character output data attempts to maintain the invariant that
> `_IO_buf_base <= _IO_write_base <= _IO_write_end <= _IO_buf_end`
> (that is, that the write region is a sub-region of `_IO_buf`).
> Prior to this commit, this invariant is violated by the
> `_IO_wfile_overflow` function as so:
>
> 1. `_IO_wsetg` is called, assigning `_IO_write_base` to `_IO_buf_base`
> 2. `_IO_setg` is called, assigning `_IO_buf_base` to a malloc’d buffer.
>
> Thus the invariant is violated. The fix is simply to reverse the order:
> malloc the `_IO_buf` first and then assign `_IO_write_base` to it.
I think what is missing from the description is that as part of
switching a stream orientation to wide, the vtable is changed to
_IO_wfile_jumps, so that a call to _IO_doallocbuf ends up calling
_IO_wfile_doallocate in the end. This updates the _wide_data pointers
(but it also calls _IO_file_doallocate to allocate the multibyte buffer,
which we need as well). The way things are put together in the
implementation remains very suspect.
> diff --git a/string/test-fputs-regression-20632.c b/string/test-fputs-regression-20632.c
> new file mode 100644
> index 0000000000..2cf779eeec
> --- /dev/null
> +++ b/string/test-fputs-regression-20632.c
> @@ -0,0 +1,60 @@
> +/* Regression test for 20632.
> + Copyright (C) 2024 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
If you use Signed-off-by, this should say
Copyright The GNU Toolchain Authors.
as well.
> +
> + 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/>. */
> +
> +#define TEST_MAIN
That shouldn't be necessary?
> +#ifndef WIDE
> +# define TEST_NAME "fputs-regression-20632"
> +#else
> +# define TEST_NAME "fputws-regression-20632"
> +#endif /* WIDE */
> +#include "test-string.h"
I don't think this is the right header to include for this test.
Please use those instead:
#include <stdio.h>
#include <stdlib.h>
#include <support/check.h>
#include <support/xunistd.h>
#include <unistd.h>
And maybe both tests should go into libio instead?
> +
> +#ifndef WIDE
> +# define CHAR char
> +# define FPUTS fputs
> +# define TEXT "0123456789ABCDEF"
> +#else
> +# include <wchar.h>
> +# define CHAR wchar_t
> +# define FPUTS fputws
> +# define TEXT L"0123456789ABCDEF"
> +#endif
> +
> +
> +int main(void) {
Should be:
statc int
do_test void ()
{
> + /* Close stderr */
> + close(2);
Is it possible to fopen /dev/full instead? Or maybe dup2 over the
descriptor of an fopen'ed stream with something that cannot be written
to?
It's a bit iffy to break stderr like this. Tests should print
diagnostics to stdout instead, but it's still not ideal.
> + /* Output long string */
> + const int sz = 4096;
> + CHAR *buff = calloc(sz+1, sizeof *buff);
> + for (int i=0; i < sz; i++) buff[i] = (CHAR)'x';
> + buff[sz] = (CHAR)'\0';
> + FPUTS(buff, stderr);
> +
> + /* Output shorter string */
> + for (int i=0; i < 1024; i++) {
> + FPUTS(TEXT, stderr);
These calls should check for expected successes and failures using
TEST_COMPARE etc.
> + /* Call malloc, which should not crash.
> + However it will if malloc's function pointers
> + have been stomped. */
> + free(malloc(1));
You need to use
void *volatile ptr = malloc (1);
free (ptr);
or something like that to prevent the compiler for eliding the
> + }
> + return 0;
> +}
Add at the end:
#include <support/test-driver.c>
Thanks,
FLorian
On Aug 26 2024, Florian Weimer wrote:
>> + /* Close stderr */
>> + close(2);
>
> Is it possible to fopen /dev/full instead? Or maybe dup2 over the
> descriptor of an fopen'ed stream with something that cannot be written
> to?
The test needs an unbuffered stream where the stdio buffers haven't been
set up yet.
@@ -420,14 +420,14 @@ _IO_wfile_overflow (FILE *f, wint_t wch)
{
_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);
if (f->_IO_write_base == NULL)
{
_IO_doallocbuf (f);
_IO_setg (f, f->_IO_buf_base, f->_IO_buf_base, f->_IO_buf_base);
}
+ _IO_wsetg (f, f->_wide_data->_IO_buf_base,
+ f->_wide_data->_IO_buf_base, f->_wide_data->_IO_buf_base);
}
else
{
@@ -958,7 +958,7 @@ _IO_wfile_xsputn (FILE *f, const void *data, size_t n)
const wchar_t *s = (const wchar_t *) data;
size_t to_do = n;
int must_flush = 0;
- size_t count;
+ size_t count = 0;
if (n <= 0)
return 0;
@@ -967,7 +967,6 @@ _IO_wfile_xsputn (FILE *f, const void *data, size_t n)
(or the filebuf is unbuffered), use sys_write directly. */
/* First figure out how much space is available in the buffer. */
- count = f->_wide_data->_IO_write_end - f->_wide_data->_IO_write_ptr;
if ((f->_flags & _IO_LINE_BUF) && (f->_flags & _IO_CURRENTLY_PUTTING))
{
count = f->_wide_data->_IO_buf_end - f->_wide_data->_IO_write_ptr;
@@ -985,6 +984,9 @@ _IO_wfile_xsputn (FILE *f, const void *data, size_t n)
}
}
}
+ else if (f->_wide_data->_IO_write_end > f->_wide_data->_IO_write_ptr)
+ count = f->_wide_data->_IO_write_end - f->_wide_data->_IO_write_ptr; /* Space available. */
+
/* Then fill the buffer. */
if (count > 0)
{
@@ -164,6 +164,7 @@ tests := \
test-mempcpy \
test-memrchr \
test-memset \
+ test-fputs-regression-20632 \
test-rawmemchr \
test-sig_np \
test-stpcpy \
new file mode 100644
@@ -0,0 +1,60 @@
+/* Regression test for 20632.
+ Copyright (C) 2024 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/>. */
+
+#define TEST_MAIN
+#ifndef WIDE
+# define TEST_NAME "fputs-regression-20632"
+#else
+# define TEST_NAME "fputws-regression-20632"
+#endif /* WIDE */
+#include "test-string.h"
+
+#ifndef WIDE
+# define CHAR char
+# define FPUTS fputs
+# define TEXT "0123456789ABCDEF"
+#else
+# include <wchar.h>
+# define CHAR wchar_t
+# define FPUTS fputws
+# define TEXT L"0123456789ABCDEF"
+#endif
+
+
+int main(void) {
+ /* Close stderr */
+ close(2);
+
+ /* Output long string */
+ const int sz = 4096;
+ CHAR *buff = calloc(sz+1, sizeof *buff);
+ for (int i=0; i < sz; i++) buff[i] = (CHAR)'x';
+ buff[sz] = (CHAR)'\0';
+ FPUTS(buff, stderr);
+
+ /* Output shorter string */
+ for (int i=0; i < 1024; i++) {
+ FPUTS(TEXT, stderr);
+
+ /* Call malloc, which should not crash.
+ However it will if malloc's function pointers
+ have been stomped. */
+ free(malloc(1));
+ }
+ return 0;
+}
@@ -146,6 +146,7 @@ tests := \
test-c8rtomb \
test-char-types \
test-mbrtoc8 \
+ test-fputws-regression-20632 \
test-wcpcpy \
test-wcpncpy \
test-wcscat \
new file mode 100644
@@ -0,0 +1,20 @@
+/* Regression test for 20632. Wide character output doesn't crash on a closed fd.
+ Copyright (C) 2024 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/>. */
+
+#define WIDE 1
+#include "../string/test-fputs-regression-20632.c"