From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from erza.lautre.net (erza.lautre.net [80.67.160.89]) by sourceware.org (Postfix) with ESMTPS id 5A4F2385783B for ; Tue, 8 Mar 2022 08:44:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5A4F2385783B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=nonadev.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=nonadev.net Received: from d-jobol.iot.bzh (por56-1-78-229-206-229.fbx.proxad.net [78.229.206.229]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by erza.lautre.net (Postfix) with ESMTPSA id 624CDE4376; Tue, 8 Mar 2022 09:44:34 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=nonadev.net; s=alternc; t=1646729074; bh=CGxEUe4Gr3VcDtvErtTAlgbeidY5Nf4WcViIevpuLDA=; h=Date:From:To:Cc:Subject:In-Reply-To:References; b=JxuBpF/ZMJWG18I04RmH3OvufrgqS/XWqwSjDP4iRdhiOeescCZ0+0XbHK9nsEd6K u31Sgu53bizdLQOzNLopDQzHwO9oaQyOjDo6wUBTrR0UJG6QBnGwrKlsdGOcDn1ZMi 3I5AOKzZxYIKadr7mcrYVyh8D5LLBd+Mk5yFZFE0= Date: Tue, 8 Mar 2022 09:44:26 +0100 From: =?UTF-8?B?Sm9zw6k=?= Bollo To: Adhemerval Zanella Cc: libc-alpha@sourceware.org Subject: Re: [PATCH v3] libio: Ensure output buffer for wchars (bug 28828) Message-ID: <20220308094426.10343707@d-jobol.iot.bzh> In-Reply-To: References: <20220222205550.34537-1-jobol@nonadev.net> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Mar 2022 08:44:38 -0000 On Fri, 4 Mar 2022 16:39:36 -0300 Adhemerval Zanella wrote: Hi Adhemerval, Hi Andreas, Hi all, I'm going to submit a new version taking your skeleton as a basis. I added the file named libio/tst-bz28828.input with the understanding that given its name, the test framework will automatically use it as input. Can you tell me if it is correct or if I have to add something some where. My though about the interaction between stdin and stdout is that it is sometime needed to get an input to flush the output. Best regards Jos=C3=A9 > On 22/02/2022 17:55, jobol@nonadev.net wrote: > > From: "jobol@nonadev.net" > >=20 > > 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. > >=20 > > The minimal sequence to produce the crash was: > >=20 > > #include > > #include > > int main(int ac, char **av) > > { > > setvbuf(stdout, NULL, _IOLBF, 0); > > fgetwc(stdin); > > fputwc(10, stdout); /*CRASH HERE!*/ > > return 0; > > } > >=20 > > 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). > >=20 > > Signed-off-by: Jose Bollo > > --- > > 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 > >=20 > > diff --git a/libio/Makefile b/libio/Makefile > > index 0e5f348bea..e97387743f 100644 > > --- a/libio/Makefile > > +++ b/libio/Makefile > > @@ -66,7 +66,7 @@ tests =3D 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 > > =20 > > tests-internal =3D tst-vtables tst-vtables-interposed > > =20 > > 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 > > +#include > > +int main(int ac, char **av) > > +{ > > + setvbuf(stdout, NULL, _IOLBF, 0); > > + fgetwc(stdin); > > + fputwc(10, stdout); /*SHOUDN'T CRASH HERE!*/ > > + return 0; > > +} > > + =20 >=20 > 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: >=20 > --- > /* Unit test for BZ#28828. > Copyright (C) 2022 Free Software Foundation, Inc. > This file is part of the GNU C Library. >=20 > 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. >=20 > 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. >=20 > You should have received a copy of the GNU Lesser General Public > License along with the GNU C Library; if not, see > . */ >=20 > #include > #include > #include >=20 > static int > do_test (void) > {=20 > /* Add the test here. */ >=20 > return 0; > } >=20 > #include > --- >=20 > Keep in mind that using fgetwc without a method to actually input > some data will make the test timeout and fail. >=20 >=20 > > 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) =3D=3D 0) > > + if ((f->_flags & _IO_CURRENTLY_PUTTING) =3D=3D 0 > > + || f->_wide_data->_IO_write_base =3D=3D NULL) > > { > > /* Allocate a buffer if needed. */ > > if (f->_wide_data->_IO_write_base =3D=3D 0) =20 >=20 > 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. >=20 > Digging a little bit deeper it seems this snippet is triggering it: >=20 > libio/wfileops.c >=20 > 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 =20 > 211 if ((stdout->_flags & (_IO_LINKED | _IO_NO_WRITES | > _IO_LINE_BUF)) 212 =3D=3D (_IO_LINKED | _IO_LINE_BUF)) > 213 _IO_OVERFLOW (stdout, EOF); > 214 =20 > 215 _IO_release_lock (stdout); > 216 } >=20 > 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.