Message ID | d432961e08bf2c452c12644aeaeffbfe961a0ed3.1671221440.git.fweimer@redhat.com |
---|---|
State | New |
Headers | show |
Series | vfprintf refactor | expand |
On 16/12/22 17:15, Florian Weimer via Libc-alpha wrote: > Always null-terminate the buffer and set E2BIG if the buffer is too > small. This fixes bug 27857. Good, another vtable removed. LGTM, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > --- > elf/Makefile | 1 - > include/printf_buffer.h | 1 + > libio/tst_swprintf.c | 31 ++++++++- > libio/vswprintf.c | 100 +++++----------------------- > manual/stdio.texi | 7 +- > stdio-common/wprintf_buffer_flush.c | 6 ++ > 6 files changed, 57 insertions(+), 89 deletions(-) > > diff --git a/elf/Makefile b/elf/Makefile > index 36901ac3ab..0ecbcde962 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -609,7 +609,6 @@ $(objpfx)tst-relro-libc.out: tst-relro-symbols.py $(..)/scripts/glibcelf.py \ > --required=_IO_wmem_jumps \ > --required=_IO_wprintf_buffer_as_file_jumps \ > --required=_IO_wstr_jumps \ > - --required=_IO_wstrn_jumps \ > --optional=_IO_old_cookie_jumps \ > --optional=_IO_old_file_jumps \ > --optional=_IO_old_proc_jumps \ > diff --git a/include/printf_buffer.h b/include/printf_buffer.h > index 168555d8d3..e7a45fb45f 100644 > --- a/include/printf_buffer.h > +++ b/include/printf_buffer.h > @@ -196,6 +196,7 @@ bool __printf_buffer_flush (struct __printf_buffer *buf) attribute_hidden; > enum __wprintf_buffer_mode > { > __wprintf_buffer_mode_failed, > + __wprintf_buffer_mode_swprintf, > __wprintf_buffer_mode_to_file, > }; > > diff --git a/libio/tst_swprintf.c b/libio/tst_swprintf.c > index 1f997b9393..169951de4b 100644 > --- a/libio/tst_swprintf.c > +++ b/libio/tst_swprintf.c > @@ -1,4 +1,5 @@ > #include <array_length.h> > +#include <errno.h> > #include <stdio.h> > #include <support/check.h> > #include <sys/types.h> > @@ -37,6 +38,8 @@ do_test (void) > > for (n = 0; n < array_length (tests); ++n) > { > + wmemset (buf, 0xabcd, array_length (buf)); > + errno = 0; > ssize_t res = swprintf (buf, tests[n].n, L"%s", tests[n].str); > > if (tests[n].exp < 0 && res >= 0) > @@ -52,9 +55,31 @@ do_test (void) > swprintf (buf, %zu, L\"%%s\", \"%s\") expected to return %zd, but got %zd\n", > tests[n].n, tests[n].str, tests[n].exp, res); > } > - else > - printf ("swprintf (buf, %zu, L\"%%s\", \"%s\") OK\n", > - tests[n].n, tests[n].str); > + else if (res < 0 > + && tests[n].n > 0 > + && wcsnlen (buf, array_length (buf)) == array_length (buf)) > + { > + support_record_failure (); > + printf ("\ > +error: swprintf (buf, %zu, L\"%%s\", \"%s\") missing null terminator\n", > + tests[n].n, tests[n].str); > + } > + else if (res < 0 > + && tests[n].n > 0 > + && wcsnlen (buf, array_length (buf)) < array_length (buf) > + && buf[wcsnlen (buf, array_length (buf)) + 1] != 0xabcd) > + { > + support_record_failure (); > + printf ("\ > +error: swprintf (buf, %zu, L\"%%s\", \"%s\") out of bounds write\n", > + tests[n].n, tests[n].str); > + } > + > + if (res < 0 && tests[n].n < 0) > + TEST_COMPARE (errno, E2BIG); > + > + printf ("swprintf (buf, %zu, L\"%%s\", \"%s\") OK\n", > + tests[n].n, tests[n].str); > } > > TEST_COMPARE (swprintf (buf, array_length (buf), L"%.0s", "foo"), 0); > diff --git a/libio/vswprintf.c b/libio/vswprintf.c > index 5a9ccdff3e..f53545c806 100644 > --- a/libio/vswprintf.c > +++ b/libio/vswprintf.c > @@ -24,101 +24,37 @@ > This exception applies to code released by its copyright holders > in files containing the exception. */ > > -#include "libioP.h" > -#include "strfile.h" > - > - > -static wint_t _IO_wstrn_overflow (FILE *fp, wint_t c) __THROW; > - > -static wint_t > -_IO_wstrn_overflow (FILE *fp, wint_t c) > -{ > - /* When we come to here this means the user supplied buffer is > - filled. But since we must return the number of characters which > - would have been written in total we must provide a buffer for > - further use. We can do this by writing on and on in the overflow > - buffer in the _IO_wstrnfile structure. */ > - _IO_wstrnfile *snf = (_IO_wstrnfile *) fp; > - > - if (fp->_wide_data->_IO_buf_base != snf->overflow_buf) > - { > - _IO_wsetb (fp, snf->overflow_buf, > - snf->overflow_buf + (sizeof (snf->overflow_buf) > - / sizeof (wchar_t)), 0); > - > - fp->_wide_data->_IO_write_base = snf->overflow_buf; > - fp->_wide_data->_IO_read_base = snf->overflow_buf; > - fp->_wide_data->_IO_read_ptr = snf->overflow_buf; > - fp->_wide_data->_IO_read_end = (snf->overflow_buf > - + (sizeof (snf->overflow_buf) > - / sizeof (wchar_t))); > - } > - > - fp->_wide_data->_IO_write_ptr = snf->overflow_buf; > - fp->_wide_data->_IO_write_end = snf->overflow_buf; > - > - /* Since we are not really interested in storing the characters > - which do not fit in the buffer we simply ignore it. */ > - return c; > -} > - > - > -const struct _IO_jump_t _IO_wstrn_jumps libio_vtable attribute_hidden = > -{ > - JUMP_INIT_DUMMY, > - JUMP_INIT(finish, _IO_wstr_finish), > - JUMP_INIT(overflow, (_IO_overflow_t) _IO_wstrn_overflow), > - JUMP_INIT(underflow, (_IO_underflow_t) _IO_wstr_underflow), > - JUMP_INIT(uflow, (_IO_underflow_t) _IO_wdefault_uflow), > - JUMP_INIT(pbackfail, (_IO_pbackfail_t) _IO_wstr_pbackfail), > - JUMP_INIT(xsputn, _IO_wdefault_xsputn), > - JUMP_INIT(xsgetn, _IO_wdefault_xsgetn), > - JUMP_INIT(seekoff, _IO_wstr_seekoff), > - JUMP_INIT(seekpos, _IO_default_seekpos), > - JUMP_INIT(setbuf, _IO_default_setbuf), > - JUMP_INIT(sync, _IO_default_sync), > - JUMP_INIT(doallocate, _IO_wdefault_doallocate), > - JUMP_INIT(read, _IO_default_read), > - JUMP_INIT(write, _IO_default_write), > - JUMP_INIT(seek, _IO_default_seek), > - JUMP_INIT(close, _IO_default_close), > - JUMP_INIT(stat, _IO_default_stat), > - JUMP_INIT(showmanyc, _IO_default_showmanyc), > - JUMP_INIT(imbue, _IO_default_imbue) > -}; > - > +#include <errno.h> > +#include <math_ldbl_opt.h> > +#include <printf.h> > +#include <printf_buffer.h> > > int > __vswprintf_internal (wchar_t *string, size_t maxlen, const wchar_t *format, > va_list args, unsigned int mode_flags) > { > - _IO_wstrnfile sf; > - int ret; > - struct _IO_wide_data wd; > -#ifdef _IO_MTSAFE_IO > - sf.f._sbf._f._lock = NULL; > -#endif > - > if (maxlen == 0) > /* Since we have to write at least the terminating L'\0' a buffer > length of zero always makes the function fail. */ > return -1; > > - _IO_no_init (&sf.f._sbf._f, _IO_USER_LOCK, 0, &wd, &_IO_wstrn_jumps); > - _IO_fwide (&sf.f._sbf._f, 1); > - string[0] = L'\0'; > - _IO_wstr_init_static (&sf.f._sbf._f, string, maxlen - 1, string); > - ret = __vfwprintf_internal ((FILE *) &sf.f._sbf, format, args, mode_flags); > + struct __wprintf_buffer buf; > + __wprintf_buffer_init (&buf, string, maxlen, __wprintf_buffer_mode_swprintf); > > - if (sf.f._sbf._f._wide_data->_IO_buf_base == sf.overflow_buf) > - /* ISO C99 requires swprintf/vswprintf to return an error if the > - output does not fit in the provided buffer. */ > - return -1; > + __wprintf_buffer (&buf, format, args, mode_flags); > + > + if (buf.write_ptr == buf.write_end) > + { > + /* Buffer has been filled exactly, excluding the null wide > + character. This is an error because the null wide character > + is required. */ > + buf.write_end[-1] = L'\0'; > + return -1; > + } > > - /* Terminate the string. */ > - *sf.f._sbf._f._wide_data->_IO_write_ptr = '\0'; > + buf.write_ptr[0] = L'\0'; > > - return ret; > + return __wprintf_buffer_done (&buf); > } > > int > diff --git a/manual/stdio.texi b/manual/stdio.texi > index 4aa1a2bc2d..f6319a4b8a 100644 > --- a/manual/stdio.texi > +++ b/manual/stdio.texi > @@ -2412,9 +2412,10 @@ allocate at least @var{size} wide characters for the string @var{ws}. > > The return value is the number of characters generated for the given > input, excluding the trailing null. If not all output fits into the > -provided buffer a negative value is returned. You should try again with > -a bigger output string. @emph{Note:} this is different from how > -@code{snprintf} handles this situation. > +provided buffer a negative value is returned, and @code{errno} is set to > +@code{E2BIG}. (The setting of @code{errno} is a GNU extension.) You > +should try again with a bigger output string. @emph{Note:} this is > +different from how @code{snprintf} handles this situation. > > Note that the corresponding narrow stream function takes fewer > parameters. @code{swprintf} in fact corresponds to the @code{snprintf} > diff --git a/stdio-common/wprintf_buffer_flush.c b/stdio-common/wprintf_buffer_flush.c > index 2d91095cca..46af0f348f 100644 > --- a/stdio-common/wprintf_buffer_flush.c > +++ b/stdio-common/wprintf_buffer_flush.c > @@ -16,6 +16,7 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > +#include <errno.h> > #include <printf_buffer.h> > > #include "printf_buffer-wchar_t.h" > @@ -31,6 +32,11 @@ __wprintf_buffer_do_flush (struct __wprintf_buffer *buf) > case __wprintf_buffer_mode_to_file: > __wprintf_buffer_flush_to_file ((struct __wprintf_buffer_to_file *) buf); > return; > + case __wprintf_buffer_mode_swprintf: > + buf->write_end[-1] = L'\0'; > + __set_errno (E2BIG); > + __wprintf_buffer_mark_failed (buf); > + return; > } > __builtin_trap (); > }
diff --git a/elf/Makefile b/elf/Makefile index 36901ac3ab..0ecbcde962 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -609,7 +609,6 @@ $(objpfx)tst-relro-libc.out: tst-relro-symbols.py $(..)/scripts/glibcelf.py \ --required=_IO_wmem_jumps \ --required=_IO_wprintf_buffer_as_file_jumps \ --required=_IO_wstr_jumps \ - --required=_IO_wstrn_jumps \ --optional=_IO_old_cookie_jumps \ --optional=_IO_old_file_jumps \ --optional=_IO_old_proc_jumps \ diff --git a/include/printf_buffer.h b/include/printf_buffer.h index 168555d8d3..e7a45fb45f 100644 --- a/include/printf_buffer.h +++ b/include/printf_buffer.h @@ -196,6 +196,7 @@ bool __printf_buffer_flush (struct __printf_buffer *buf) attribute_hidden; enum __wprintf_buffer_mode { __wprintf_buffer_mode_failed, + __wprintf_buffer_mode_swprintf, __wprintf_buffer_mode_to_file, }; diff --git a/libio/tst_swprintf.c b/libio/tst_swprintf.c index 1f997b9393..169951de4b 100644 --- a/libio/tst_swprintf.c +++ b/libio/tst_swprintf.c @@ -1,4 +1,5 @@ #include <array_length.h> +#include <errno.h> #include <stdio.h> #include <support/check.h> #include <sys/types.h> @@ -37,6 +38,8 @@ do_test (void) for (n = 0; n < array_length (tests); ++n) { + wmemset (buf, 0xabcd, array_length (buf)); + errno = 0; ssize_t res = swprintf (buf, tests[n].n, L"%s", tests[n].str); if (tests[n].exp < 0 && res >= 0) @@ -52,9 +55,31 @@ do_test (void) swprintf (buf, %zu, L\"%%s\", \"%s\") expected to return %zd, but got %zd\n", tests[n].n, tests[n].str, tests[n].exp, res); } - else - printf ("swprintf (buf, %zu, L\"%%s\", \"%s\") OK\n", - tests[n].n, tests[n].str); + else if (res < 0 + && tests[n].n > 0 + && wcsnlen (buf, array_length (buf)) == array_length (buf)) + { + support_record_failure (); + printf ("\ +error: swprintf (buf, %zu, L\"%%s\", \"%s\") missing null terminator\n", + tests[n].n, tests[n].str); + } + else if (res < 0 + && tests[n].n > 0 + && wcsnlen (buf, array_length (buf)) < array_length (buf) + && buf[wcsnlen (buf, array_length (buf)) + 1] != 0xabcd) + { + support_record_failure (); + printf ("\ +error: swprintf (buf, %zu, L\"%%s\", \"%s\") out of bounds write\n", + tests[n].n, tests[n].str); + } + + if (res < 0 && tests[n].n < 0) + TEST_COMPARE (errno, E2BIG); + + printf ("swprintf (buf, %zu, L\"%%s\", \"%s\") OK\n", + tests[n].n, tests[n].str); } TEST_COMPARE (swprintf (buf, array_length (buf), L"%.0s", "foo"), 0); diff --git a/libio/vswprintf.c b/libio/vswprintf.c index 5a9ccdff3e..f53545c806 100644 --- a/libio/vswprintf.c +++ b/libio/vswprintf.c @@ -24,101 +24,37 @@ This exception applies to code released by its copyright holders in files containing the exception. */ -#include "libioP.h" -#include "strfile.h" - - -static wint_t _IO_wstrn_overflow (FILE *fp, wint_t c) __THROW; - -static wint_t -_IO_wstrn_overflow (FILE *fp, wint_t c) -{ - /* When we come to here this means the user supplied buffer is - filled. But since we must return the number of characters which - would have been written in total we must provide a buffer for - further use. We can do this by writing on and on in the overflow - buffer in the _IO_wstrnfile structure. */ - _IO_wstrnfile *snf = (_IO_wstrnfile *) fp; - - if (fp->_wide_data->_IO_buf_base != snf->overflow_buf) - { - _IO_wsetb (fp, snf->overflow_buf, - snf->overflow_buf + (sizeof (snf->overflow_buf) - / sizeof (wchar_t)), 0); - - fp->_wide_data->_IO_write_base = snf->overflow_buf; - fp->_wide_data->_IO_read_base = snf->overflow_buf; - fp->_wide_data->_IO_read_ptr = snf->overflow_buf; - fp->_wide_data->_IO_read_end = (snf->overflow_buf - + (sizeof (snf->overflow_buf) - / sizeof (wchar_t))); - } - - fp->_wide_data->_IO_write_ptr = snf->overflow_buf; - fp->_wide_data->_IO_write_end = snf->overflow_buf; - - /* Since we are not really interested in storing the characters - which do not fit in the buffer we simply ignore it. */ - return c; -} - - -const struct _IO_jump_t _IO_wstrn_jumps libio_vtable attribute_hidden = -{ - JUMP_INIT_DUMMY, - JUMP_INIT(finish, _IO_wstr_finish), - JUMP_INIT(overflow, (_IO_overflow_t) _IO_wstrn_overflow), - JUMP_INIT(underflow, (_IO_underflow_t) _IO_wstr_underflow), - JUMP_INIT(uflow, (_IO_underflow_t) _IO_wdefault_uflow), - JUMP_INIT(pbackfail, (_IO_pbackfail_t) _IO_wstr_pbackfail), - JUMP_INIT(xsputn, _IO_wdefault_xsputn), - JUMP_INIT(xsgetn, _IO_wdefault_xsgetn), - JUMP_INIT(seekoff, _IO_wstr_seekoff), - JUMP_INIT(seekpos, _IO_default_seekpos), - JUMP_INIT(setbuf, _IO_default_setbuf), - JUMP_INIT(sync, _IO_default_sync), - JUMP_INIT(doallocate, _IO_wdefault_doallocate), - JUMP_INIT(read, _IO_default_read), - JUMP_INIT(write, _IO_default_write), - JUMP_INIT(seek, _IO_default_seek), - JUMP_INIT(close, _IO_default_close), - JUMP_INIT(stat, _IO_default_stat), - JUMP_INIT(showmanyc, _IO_default_showmanyc), - JUMP_INIT(imbue, _IO_default_imbue) -}; - +#include <errno.h> +#include <math_ldbl_opt.h> +#include <printf.h> +#include <printf_buffer.h> int __vswprintf_internal (wchar_t *string, size_t maxlen, const wchar_t *format, va_list args, unsigned int mode_flags) { - _IO_wstrnfile sf; - int ret; - struct _IO_wide_data wd; -#ifdef _IO_MTSAFE_IO - sf.f._sbf._f._lock = NULL; -#endif - if (maxlen == 0) /* Since we have to write at least the terminating L'\0' a buffer length of zero always makes the function fail. */ return -1; - _IO_no_init (&sf.f._sbf._f, _IO_USER_LOCK, 0, &wd, &_IO_wstrn_jumps); - _IO_fwide (&sf.f._sbf._f, 1); - string[0] = L'\0'; - _IO_wstr_init_static (&sf.f._sbf._f, string, maxlen - 1, string); - ret = __vfwprintf_internal ((FILE *) &sf.f._sbf, format, args, mode_flags); + struct __wprintf_buffer buf; + __wprintf_buffer_init (&buf, string, maxlen, __wprintf_buffer_mode_swprintf); - if (sf.f._sbf._f._wide_data->_IO_buf_base == sf.overflow_buf) - /* ISO C99 requires swprintf/vswprintf to return an error if the - output does not fit in the provided buffer. */ - return -1; + __wprintf_buffer (&buf, format, args, mode_flags); + + if (buf.write_ptr == buf.write_end) + { + /* Buffer has been filled exactly, excluding the null wide + character. This is an error because the null wide character + is required. */ + buf.write_end[-1] = L'\0'; + return -1; + } - /* Terminate the string. */ - *sf.f._sbf._f._wide_data->_IO_write_ptr = '\0'; + buf.write_ptr[0] = L'\0'; - return ret; + return __wprintf_buffer_done (&buf); } int diff --git a/manual/stdio.texi b/manual/stdio.texi index 4aa1a2bc2d..f6319a4b8a 100644 --- a/manual/stdio.texi +++ b/manual/stdio.texi @@ -2412,9 +2412,10 @@ allocate at least @var{size} wide characters for the string @var{ws}. The return value is the number of characters generated for the given input, excluding the trailing null. If not all output fits into the -provided buffer a negative value is returned. You should try again with -a bigger output string. @emph{Note:} this is different from how -@code{snprintf} handles this situation. +provided buffer a negative value is returned, and @code{errno} is set to +@code{E2BIG}. (The setting of @code{errno} is a GNU extension.) You +should try again with a bigger output string. @emph{Note:} this is +different from how @code{snprintf} handles this situation. Note that the corresponding narrow stream function takes fewer parameters. @code{swprintf} in fact corresponds to the @code{snprintf} diff --git a/stdio-common/wprintf_buffer_flush.c b/stdio-common/wprintf_buffer_flush.c index 2d91095cca..46af0f348f 100644 --- a/stdio-common/wprintf_buffer_flush.c +++ b/stdio-common/wprintf_buffer_flush.c @@ -16,6 +16,7 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ +#include <errno.h> #include <printf_buffer.h> #include "printf_buffer-wchar_t.h" @@ -31,6 +32,11 @@ __wprintf_buffer_do_flush (struct __wprintf_buffer *buf) case __wprintf_buffer_mode_to_file: __wprintf_buffer_flush_to_file ((struct __wprintf_buffer_to_file *) buf); return; + case __wprintf_buffer_mode_swprintf: + buf->write_end[-1] = L'\0'; + __set_errno (E2BIG); + __wprintf_buffer_mark_failed (buf); + return; } __builtin_trap (); }