From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: jobol@nonadev.net, libc-alpha@sourceware.org
Subject: Re: [PATCH v3] libio: Ensure output buffer for wchars (bug 28828)
Date: Fri, 4 Mar 2022 16:39:36 -0300 [thread overview]
Message-ID: <ded339bc-cb70-1cf9-7f31-c3d32a23a0c2@linaro.org> (raw)
In-Reply-To: <20220222205550.34537-1-jobol@nonadev.net>
On 22/02/2022 17:55, jobol@nonadev.net wrote:
> From: "jobol@nonadev.net" <jobol@nonadev.net>
>
> When fileops.c checks for nullity of the write pointer,
> in order to ensure its allocation, before that patch,
> wfileops didn't. This was leading to crashes on some cases,
> as described by bug 28828.
>
> The minimal sequence to produce the crash was:
>
> #include <stdio.h>
> #include <wchar.h>
> int main(int ac, char **av)
> {
> setvbuf(stdout, NULL, _IOLBF, 0);
> fgetwc(stdin);
> fputwc(10, stdout); /*CRASH HERE!*/
> return 0;
> }
>
> The line "fgetwc(stdin);" is necessary. It introduces the
> bug by setting the flag _IO_CURRENTLY_PUTTING of stdout
> indirectly (file wfileops.c, function _IO_wfile_underflow, line 213).
>
> Signed-off-by: Jose Bollo <jobol@nonadev.net>
> ---
> libio/Makefile | 2 +-
> libio/tst-bz28828.c | 10 ++++++++++
> libio/tst-bz28828.input | 1 +
> libio/wfileops.c | 3 ++-
> 4 files changed, 14 insertions(+), 2 deletions(-)
> create mode 100644 libio/tst-bz28828.c
> create mode 100644 libio/tst-bz28828.input
>
> diff --git a/libio/Makefile b/libio/Makefile
> index 0e5f348bea..e97387743f 100644
> --- a/libio/Makefile
> +++ b/libio/Makefile
> @@ -66,7 +66,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \
> tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \
> tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \
> tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 \
> - tst-wfile-sync
> + tst-wfile-sync tst-bz28828
>
> tests-internal = tst-vtables tst-vtables-interposed
>
> diff --git a/libio/tst-bz28828.c b/libio/tst-bz28828.c
> new file mode 100644
> index 0000000000..f5849d2ca6
> --- /dev/null
> +++ b/libio/tst-bz28828.c
> @@ -0,0 +1,10 @@
> +#include <stdio.h>
> +#include <wchar.h>
> +int main(int ac, char **av)
> +{
> + setvbuf(stdout, NULL, _IOLBF, 0);
> + fgetwc(stdin);
> + fputwc(10, stdout); /*SHOUDN'T CRASH HERE!*/
> + return 0;
> +}
> +
The tests requires a proper Copyright header and to use libsupport (so it handles
test timeout, segfault, or any other error in test itself). Also, use the proper
indentation. A skeleton will be something like below:
---
/* Unit test for BZ#28828.
Copyright (C) 2022 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 <support/xstdio.h>
#include <support/check.h>
#include <wchar.h>
static int
do_test (void)
{
/* Add the test here. */
return 0;
}
#include <support/test-driver.c>
---
Keep in mind that using fgetwc without a method to actually input some data
will make the test timeout and fail.
> diff --git a/libio/tst-bz28828.input b/libio/tst-bz28828.input
> new file mode 100644
> index 0000000000..ce01362503
> --- /dev/null
> +++ b/libio/tst-bz28828.input
> @@ -0,0 +1 @@
> +hello
> diff --git a/libio/wfileops.c b/libio/wfileops.c
> index fb9d45b677..b59a98881f 100644
> --- a/libio/wfileops.c
> +++ b/libio/wfileops.c
> @@ -412,7 +412,8 @@ _IO_wfile_overflow (FILE *f, wint_t wch)
> return WEOF;
> }
> /* If currently reading or no buffer allocated. */
> - if ((f->_flags & _IO_CURRENTLY_PUTTING) == 0)
> + if ((f->_flags & _IO_CURRENTLY_PUTTING) == 0
> + || f->_wide_data->_IO_write_base == NULL)
> {
> /* Allocate a buffer if needed. */
> if (f->_wide_data->_IO_write_base == 0)
Based on your test I was intrigued on why you need a fgetwc(stdio) to actually
trigger the issue. The fgetwc(stdio) in fact changed the stdout internal flags
somewhat, which then triggered the issue.
Digging a little bit deeper it seems this snippet is triggering it:
libio/wfileops.c
201 /* FIXME This can/should be moved to genops ?? */
202 if (fp->_flags & (_IO_LINE_BUF | _IO_UNBUFFERED))
203 {
204 /* We used to flush all line-buffered stream. This really isn't
205 required by any standard. My recollection is that
206 traditional Unix systems did this for stdout. stderr better
207 not be line buffered. So we do just that here
208 explicitly. --drepper */
209 _IO_acquire_lock (stdout);
210
211 if ((stdout->_flags & (_IO_LINKED | _IO_NO_WRITES | _IO_LINE_BUF))
212 == (_IO_LINKED | _IO_LINE_BUF))
213 _IO_OVERFLOW (stdout, EOF);
214
215 _IO_release_lock (stdout);
216 }
And the FIXME really makes me wonder if this is really ok, the libio/fileops.c
does not contain such snippet. Removing this code does not trigger any regression
and also fixes the issue.
next prev parent reply other threads:[~2022-03-04 19:39 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-22 20:55 jobol
2022-03-04 19:39 ` Adhemerval Zanella [this message]
2022-03-04 19:53 ` Andreas Schwab
2022-03-07 11:03 ` Adhemerval Zanella
2022-03-07 11:29 ` Andreas Schwab
2022-03-07 11:33 ` Adhemerval Zanella
2022-03-07 11:37 ` Andreas Schwab
2022-03-08 8:44 ` José Bollo
2022-03-08 8:54 ` [PATCH] " jobol
2022-03-08 8:58 ` [PATCH v4] " jobol
2022-03-08 17:12 ` Adhemerval Zanella
2022-03-08 17:27 ` José Bollo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ded339bc-cb70-1cf9-7f31-c3d32a23a0c2@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=jobol@nonadev.net \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).