[v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]
diff mbox series

Message ID 20190219012913.GB29070@altlinux.org
State New
Headers show
Series
  • [v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]
Related show

Commit Message

Dmitry V. Levin Feb. 19, 2019, 1:29 a.m. UTC
Commit glibc-2.23~693 (a601b74d31ca086de38441d316a3dee24c866305)
introduced a regression: _IO_unbuffer_all() now invokes _IO_wsetb() to
free wide buffers of all files, including legacy standard files which
are small statically allocated objects that do not have wide buffers
and the _mode member, causing memory corruption.

Another memory corruption in _IO_unbuffer_all() happens when -1
is assigned to the _mode member of legacy standard files that do not
have it.

Fix both issues by changing _IO_unbuffer_all() to skip legacy standard
files in compatibility mode.

[BZ #24228]
* libio/genops.c (_IO_unbuffer_all)
[SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)]: Skip legacy standard files
in compatibility mode.
* libio/tst-bz24228.c: New file.
* libio/tst-bz24228.map: Likewise.
* libio/Makefile (tests): Add tst-bz24228.
(generated): Add tst-bz24228.mtrace and tst-bz24228.check.
(tests-special): Add $(objpfx)tst-bz24228-mem.out.
(LDFLAGS-tst-bz24228, tst-bz24228-ENV): New variables.
($(objpfx)tst-bz24228-mem.out): New rule.
---
 ChangeLog             | 14 ++++++++++++++
 libio/Makefile        | 12 ++++++++++--
 libio/genops.c        |  4 ++++
 libio/tst-bz24228.c   | 30 ++++++++++++++++++++++++++++++
 libio/tst-bz24228.map |  3 +++
 5 files changed, 61 insertions(+), 2 deletions(-)
 create mode 100644 libio/tst-bz24228.c
 create mode 100644 libio/tst-bz24228.map

Comments

Florian Weimer March 13, 2019, 3:08 p.m. UTC | #1
* 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
Dmitry V. Levin March 13, 2019, 3:46 p.m. UTC | #2
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.
Florian Weimer March 13, 2019, 3:49 p.m. UTC | #3
* 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
Dmitry V. Levin March 13, 2019, 3:59 p.m. UTC | #4
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.

Patch
diff mbox series

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;
+};