| Message ID | 20190219012913.GB29070@altlinux.org |
|---|---|
| State | Superseded |
| Headers | show |
* Dmitry V. Levin: > Commit glibc-2.23~693 (a601b74d31ca086de38441d316a3dee24c866305) Please quote the commit hash and commit subject, kernel-style. (How did you determine this reference, anyway?) > diff --git a/libio/genops.c b/libio/genops.c > index 2a0d9b81df..aa92d61b6b 100644 > --- a/libio/genops.c > +++ b/libio/genops.c > @@ -789,6 +789,10 @@ _IO_unbuffer_all (void) > > for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain) > { > +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) > + if (__glibc_unlikely (&_IO_stdin_used == NULL) && _IO_legacy_file (fp)) > + continue; > +#endif I wonder if we should check _IO_legacy_file only here. This is related to the previous discussion. > diff --git a/libio/tst-bz24228.c b/libio/tst-bz24228.c > new file mode 100644 > index 0000000000..3692f14b71 > --- /dev/null > +++ b/libio/tst-bz24228.c > @@ -0,0 +1,30 @@ > +/* BZ #24228 check for memory corruption in legacy libio > + > + Copyright (C) 2019 Free Software Foundation, Inc. > + This file is part of the GNU C Library. We do not generally have a blank line for the “Copyright” line, I think. > +#include <mcheck.h> > +#include <support/test-driver.h> > + > +static int > +do_test (void) > +{ > + mtrace (); > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/libio/tst-bz24228.map b/libio/tst-bz24228.map > new file mode 100644 > index 0000000000..ecb8c058f5 > --- /dev/null > +++ b/libio/tst-bz24228.map > @@ -0,0 +1,3 @@ > +{ > + local: _IO_stdin_used; > +}; Please add a comment to the file why you are doing this, something like “hide the symbol from libc.so.6 to switch to the libio/oldfileops.c implementation (if it is available for the architecture)”. Thanks, Florian
On Wed, Mar 13, 2019 at 04:08:16PM +0100, Florian Weimer wrote: > * Dmitry V. Levin: > > > Commit glibc-2.23~693 (a601b74d31ca086de38441d316a3dee24c866305) > > Please quote the commit hash and commit subject, kernel-style. The kernel-style reference would look this way: Commit a601b74d31ca ("In preparation for fixing BZ#16734, fix failure in misc/tst-error1-mem when _G_HAVE_MMAP is turned off.") I'd like to add a reference to glibc-2.23~693 somewhere because I find it useful, but I don't see any suitable place for it in this long kernel-style form. > (How did you determine this reference, anyway?) Sorry? > > diff --git a/libio/genops.c b/libio/genops.c > > index 2a0d9b81df..aa92d61b6b 100644 > > --- a/libio/genops.c > > +++ b/libio/genops.c > > @@ -789,6 +789,10 @@ _IO_unbuffer_all (void) > > > > for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain) > > { > > +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) > > + if (__glibc_unlikely (&_IO_stdin_used == NULL) && _IO_legacy_file (fp)) > > + continue; > > +#endif > > I wonder if we should check _IO_legacy_file only here. This is related > to the previous discussion. If we omitted the check for _IO_stdin_used, then standard files would be skipped and misc/tst-error1-mem would complain. > > diff --git a/libio/tst-bz24228.c b/libio/tst-bz24228.c > > new file mode 100644 > > index 0000000000..3692f14b71 > > --- /dev/null > > +++ b/libio/tst-bz24228.c > > @@ -0,0 +1,30 @@ > > +/* BZ #24228 check for memory corruption in legacy libio > > + > > + Copyright (C) 2019 Free Software Foundation, Inc. > > + This file is part of the GNU C Library. > > We do not generally have a blank line for the “Copyright” line, I think. No problem, I can remove it. I don't think we are quite consistent about it, though. > > +#include <mcheck.h> > > +#include <support/test-driver.h> > > + > > +static int > > +do_test (void) > > +{ > > + mtrace (); > > + return 0; > > +} > > + > > +#include <support/test-driver.c> > > diff --git a/libio/tst-bz24228.map b/libio/tst-bz24228.map > > new file mode 100644 > > index 0000000000..ecb8c058f5 > > --- /dev/null > > +++ b/libio/tst-bz24228.map > > @@ -0,0 +1,3 @@ > > +{ > > + local: _IO_stdin_used; > > +}; > > Please add a comment to the file why you are doing this, something like > “hide the symbol from libc.so.6 to switch to the libio/oldfileops.c > implementation (if it is available for the architecture)”. Indeed, thanks for the suggestion.
* Dmitry V. Levin: > On Wed, Mar 13, 2019 at 04:08:16PM +0100, Florian Weimer wrote: >> * Dmitry V. Levin: >> >> > Commit glibc-2.23~693 (a601b74d31ca086de38441d316a3dee24c866305) >> >> Please quote the commit hash and commit subject, kernel-style. > > The kernel-style reference would look this way: > > Commit a601b74d31ca ("In preparation for fixing BZ#16734, fix failure in > misc/tst-error1-mem when _G_HAVE_MMAP is turned off.") > > I'd like to add a reference to glibc-2.23~693 somewhere because > I find it useful, but I don't see any suitable place for it > in this long kernel-style form. Hmm. It's not strictly speaking unique because we do not reject branches, unfortunately. >> (How did you determine this reference, anyway?) > > Sorry? glibc-2.23~693 looks nice, but I can't get “git describe” to produce. >> > diff --git a/libio/genops.c b/libio/genops.c >> > index 2a0d9b81df..aa92d61b6b 100644 >> > --- a/libio/genops.c >> > +++ b/libio/genops.c >> > @@ -789,6 +789,10 @@ _IO_unbuffer_all (void) >> > >> > for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain) >> > { >> > +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) >> > + if (__glibc_unlikely (&_IO_stdin_used == NULL) && _IO_legacy_file (fp)) >> > + continue; >> > +#endif >> >> I wonder if we should check _IO_legacy_file only here. This is related >> to the previous discussion. > > If we omitted the check for _IO_stdin_used, then standard files > would be skipped and misc/tst-error1-mem would complain. Really? Why would _IO_legacy_file be true for those? That's definitely not what I intended. Thanks, Florian
On Wed, Mar 13, 2019 at 04:49:59PM +0100, Florian Weimer wrote: > * Dmitry V. Levin: > > > On Wed, Mar 13, 2019 at 04:08:16PM +0100, Florian Weimer wrote: > >> * Dmitry V. Levin: > >> > >> > Commit glibc-2.23~693 (a601b74d31ca086de38441d316a3dee24c866305) > >> > >> Please quote the commit hash and commit subject, kernel-style. > > > > The kernel-style reference would look this way: > > > > Commit a601b74d31ca ("In preparation for fixing BZ#16734, fix failure in > > misc/tst-error1-mem when _G_HAVE_MMAP is turned off.") > > > > I'd like to add a reference to glibc-2.23~693 somewhere because > > I find it useful, but I don't see any suitable place for it > > in this long kernel-style form. > > Hmm. It's not strictly speaking unique because we do not reject > branches, unfortunately. No, it's not unique so I also add the commit hash, but it's handy. > >> (How did you determine this reference, anyway?) > > > > Sorry? > > glibc-2.23~693 looks nice, but I can't get “git describe” to produce. git describe --contains a601b74d31ca > >> > diff --git a/libio/genops.c b/libio/genops.c > >> > index 2a0d9b81df..aa92d61b6b 100644 > >> > --- a/libio/genops.c > >> > +++ b/libio/genops.c > >> > @@ -789,6 +789,10 @@ _IO_unbuffer_all (void) > >> > > >> > for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain) > >> > { > >> > +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) > >> > + if (__glibc_unlikely (&_IO_stdin_used == NULL) && _IO_legacy_file (fp)) > >> > + continue; > >> > +#endif > >> > >> I wonder if we should check _IO_legacy_file only here. This is related > >> to the previous discussion. > > > > If we omitted the check for _IO_stdin_used, then standard files > > would be skipped and misc/tst-error1-mem would complain. > > Really? Why would _IO_legacy_file be true for those? That's definitely > not what I intended. I don't see why it shouldn't be the case, but it was a month ago when I looked at the code, so I'll re-check.
* Dmitry V. Levin: > diff --git a/libio/genops.c b/libio/genops.c > index 2a0d9b81df..aa92d61b6b 100644 > --- a/libio/genops.c > +++ b/libio/genops.c > @@ -789,6 +789,10 @@ _IO_unbuffer_all (void) > > for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain) > { > +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) > + if (__glibc_unlikely (&_IO_stdin_used == NULL) && _IO_legacy_file (fp)) > + continue; > +#endif > if (! (fp->_flags & _IO_UNBUFFERED) > /* Iff stream is un-orientated, it wasn't used. */ > && fp->_mode != 0) I believe a better fix would be this, in case an old-style file showed up for a different reason: #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) bool potentially_wide_stream = _IO_vtable_offset (fp) != 0; #else bool potentially_wide_stream = true; #endif if (potentially_wide_stream && fp->_mode > 0) _IO_wsetb (fp, NULL, NULL, 0); This is _IO_new_fclose handles this situation. I fear the test is unreliable because it depends on what fp->_mode evaluates to (which is not actually present in the struct with old files). But the test is definitely better than nothing. Thanks, Florian
diff --git a/libio/Makefile b/libio/Makefile index 95b91084dd..d6cabe5ef3 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -65,7 +65,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \ 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-bz22415 tst-fgetc-after-eof \ - tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 + tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 tst-bz24228 tests-internal = tst-vtables tst-vtables-interposed tst-readline @@ -157,15 +157,19 @@ CFLAGS-tst_putwc.c += -DOBJPFX=\"$(objpfx)\" CFLAGS-tst-sprintf-ub.c += -Wno-restrict CFLAGS-tst-sprintf-chk-ub.c += -Wno-restrict +LDFLAGS-tst-bz24228 = -Wl,--version-script=tst-bz24228.map + 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 +tst-bz24228-ENV = MALLOC_TRACE=$(objpfx)tst-bz24228.mtrace generated += test-fmemopen.mtrace test-fmemopen.check generated += tst-fopenloc.mtrace tst-fopenloc.check generated += tst-bz22415.mtrace tst-bz22415.check +generated += tst-bz24228.mtrace tst-bz24228.check aux := fileops genops stdfiles stdio strops @@ -180,7 +184,7 @@ shared-only-routines = oldiofopen oldiofdopen oldiofclose oldfileops \ ifeq ($(run-built-tests),yes) tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-mem.out \ - $(objpfx)tst-bz22415-mem.out + $(objpfx)tst-bz22415-mem.out $(objpfx)tst-bz24228-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. @@ -235,3 +239,7 @@ $(objpfx)tst-fopenloc-mem.out: $(objpfx)tst-fopenloc.out $(objpfx)tst-bz22415-mem.out: $(objpfx)tst-bz22415.out $(common-objpfx)malloc/mtrace $(objpfx)tst-bz22415.mtrace > $@; \ $(evaluate-test) + +$(objpfx)tst-bz24228-mem.out: $(objpfx)tst-bz24228.out + $(common-objpfx)malloc/mtrace $(objpfx)tst-bz24228.mtrace > $@; \ + $(evaluate-test) diff --git a/libio/genops.c b/libio/genops.c index 2a0d9b81df..aa92d61b6b 100644 --- a/libio/genops.c +++ b/libio/genops.c @@ -789,6 +789,10 @@ _IO_unbuffer_all (void) for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain) { +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) + if (__glibc_unlikely (&_IO_stdin_used == NULL) && _IO_legacy_file (fp)) + continue; +#endif if (! (fp->_flags & _IO_UNBUFFERED) /* Iff stream is un-orientated, it wasn't used. */ && fp->_mode != 0) diff --git a/libio/tst-bz24228.c b/libio/tst-bz24228.c new file mode 100644 index 0000000000..3692f14b71 --- /dev/null +++ b/libio/tst-bz24228.c @@ -0,0 +1,30 @@ +/* BZ #24228 check for memory corruption in legacy libio + + Copyright (C) 2019 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 <mcheck.h> +#include <support/test-driver.h> + +static int +do_test (void) +{ + mtrace (); + return 0; +} + +#include <support/test-driver.c> diff --git a/libio/tst-bz24228.map b/libio/tst-bz24228.map new file mode 100644 index 0000000000..ecb8c058f5 --- /dev/null +++ b/libio/tst-bz24228.map @@ -0,0 +1,3 @@ +{ + local: _IO_stdin_used; +};