[PATCH] libio: do not cleanup wide buffers of legacy standard files [BZ #24228]

[PATCH] libio: do not cleanup wide buffers of legacy standard files [BZ #24228]

[Dmitry V. Levin]

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 that
are small statically allocated objects that do not have wide buffers.

Fix this by skipping _IO_wsetb() invocation for legacy standard files.

[BZ #24228]
* libio/genops.c (_IO_unbuffer_all)
[SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)]: Skip _IO_wsetb() invocation
for legacy standard files.
---
 ChangeLog      | 7 +++++++
 libio/genops.c | 7 +++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/libio/genops.c b/libio/genops.c
index 2a0d9b81df..c53696f2e0 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -816,8 +816,11 @@ _IO_unbuffer_all (void)
 
 	  _IO_SETBUF (fp, NULL, 0);
 
-	  if (fp->_mode > 0)
-	    _IO_wsetb (fp, NULL, NULL, 0);
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
+	  if (!_IO_legacy_file (fp))
+#endif
+	    if (fp->_mode > 0)
+	      _IO_wsetb (fp, NULL, NULL, 0);
 
 #ifdef _IO_MTSAFE_IO
 	  if (cnt < MAXTRIES && fp->_lock != NULL)
-- 
ldv

Re: [PATCH] libio: do not cleanup wide buffers of legacy standard files [BZ #24228]

[Florian Weimer]

* Dmitry V. Levin:

> 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 that
> are small statically allocated objects that do not have wide buffers.

Maybe at “and the _mode member”?

Does the crash reproduce under mtrace?  Then perhaps we can create a
test case by hiding the _IO_stdin_used symbol.

> diff --git a/libio/genops.c b/libio/genops.c
> index 2a0d9b81df..c53696f2e0 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -816,8 +816,11 @@ _IO_unbuffer_all (void)
>  
>  	  _IO_SETBUF (fp, NULL, 0);
>  
> -	  if (fp->_mode > 0)
> -	    _IO_wsetb (fp, NULL, NULL, 0);
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> +	  if (!_IO_legacy_file (fp))
> +#endif
> +	    if (fp->_mode > 0)
> +	      _IO_wsetb (fp, NULL, NULL, 0);

I can change my patch to always define _IO_legacy_file, for newer ABI
baselines as constantly return false.

Thanks,
Florian

Re: [PATCH] libio: do not cleanup wide buffers of legacy standard files [BZ #24228]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1806 bytes --]

On Mon, Feb 18, 2019 at 01:56:47PM +0100, Florian Weimer wrote:
> * Dmitry V. Levin:
> 
> > 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 that
> > are small statically allocated objects that do not have wide buffers.
> 
> Maybe at “and the _mode member”?

Yes, the _mode member is also not available.

> Does the crash reproduce under mtrace?  Then perhaps we can create a
> test case by hiding the _IO_stdin_used symbol.

Yes and no.  Apparently, this simple test crashes under mtrace:

$ cat tst-bz24228.c 
#include <mcheck.h>
int main() { mtrace(); return 0; }
$ cat tst-bz24228.map
{ local: _IO_stdin_used; };
$ gcc -Wall -O2 -Wl,--version-script,tst-bz24228.map tst-bz24228.c  
$ MALLOC_TRACE=/dev/null ./a.out 
Segmentation fault

But the crash is caused by a different issue that arises when
_IO_stdin_used == NULL.

> > diff --git a/libio/genops.c b/libio/genops.c
> > index 2a0d9b81df..c53696f2e0 100644
> > --- a/libio/genops.c
> > +++ b/libio/genops.c
> > @@ -816,8 +816,11 @@ _IO_unbuffer_all (void)
> >  
> >  	  _IO_SETBUF (fp, NULL, 0);
> >  
> > -	  if (fp->_mode > 0)
> > -	    _IO_wsetb (fp, NULL, NULL, 0);
> > +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> > +	  if (!_IO_legacy_file (fp))
> > +#endif
> > +	    if (fp->_mode > 0)
> > +	      _IO_wsetb (fp, NULL, NULL, 0);

Oops, this change makes misc/tst-error1-mem fail again for exactly
the same reason that led to commit glibc-2.23~693.

> I can change my patch to always define _IO_legacy_file, for newer ABI
> baselines as constantly return false.

Yes, that would be handy, but not necessary.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v2] libio: do not cleanup wide buffers of legacy standard files [BZ #24228]

[Dmitry V. Levin]

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 that
are small statically allocated objects that do not have wide buffers
and the _mode member.

Fix this by skipping _IO_wsetb() invocation for legacy standard files.

[BZ #24228]
* libio/genops.c (_IO_unbuffer_all)
[SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)]: Skip _IO_wsetb() invocation
for legacy standard files in compatibility mode.
---
 ChangeLog      | 7 +++++++
 libio/genops.c | 7 +++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/libio/genops.c b/libio/genops.c
index 2a0d9b81df..95d53595a2 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -816,8 +816,11 @@ _IO_unbuffer_all (void)
 
 	  _IO_SETBUF (fp, NULL, 0);
 
-	  if (fp->_mode > 0)
-	    _IO_wsetb (fp, NULL, NULL, 0);
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
+	  if (__glibc_likely (&_IO_stdin_used != NULL) || !_IO_legacy_file (fp))
+#endif
+	    if (fp->_mode > 0)
+	      _IO_wsetb (fp, NULL, NULL, 0);
 
 #ifdef _IO_MTSAFE_IO
 	  if (cnt < MAXTRIES && fp->_lock != NULL)
-- 
ldv

Re: [PATCH] libio: do not cleanup wide buffers of legacy standard files [BZ #24228]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

On Mon, Feb 18, 2019 at 10:10:21PM +0300, Dmitry V. Levin wrote:
> On Mon, Feb 18, 2019 at 01:56:47PM +0100, Florian Weimer wrote:
> > * Dmitry V. Levin:
> > 
> > > 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 that
> > > are small statically allocated objects that do not have wide buffers.
> > 
> > Maybe at “and the _mode member”?
> 
> Yes, the _mode member is also not available.
> 
> > Does the crash reproduce under mtrace?  Then perhaps we can create a
> > test case by hiding the _IO_stdin_used symbol.
> 
> Yes and no.  Apparently, this simple test crashes under mtrace:
> 
> $ cat tst-bz24228.c 
> #include <mcheck.h>
> int main() { mtrace(); return 0; }
> $ cat tst-bz24228.map
> { local: _IO_stdin_used; };
> $ gcc -Wall -O2 -Wl,--version-script,tst-bz24228.map tst-bz24228.c  
> $ MALLOC_TRACE=/dev/null ./a.out 
> Segmentation fault

This memory corruption is caused by "fp->_mode = -1;" statement in
_IO_unbuffer_all().  I think we should avoid touching legacy standard
files in compatibility mode.  A fix with a test follows.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Dmitry V. Levin]

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

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

Re: [PATCH v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]

[Florian Weimer]

* 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

Re: [PATCH v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]

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.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]

[Florian Weimer]

* 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

Re: [PATCH v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 2047 bytes --]

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.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]

[Florian Weimer]

* 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

Re: [PATCH v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

On Wed, Jun 19, 2019 at 03:10:14PM +0200, Florian Weimer wrote:
> * 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.

Yes, this approach seems to work, too:

diff --git a/libio/genops.c b/libio/genops.c
index 2a0d9b81df..575f0e6584 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_vtable_offset (fp) != 0))
+	continue;
+#endif
       if (! (fp->_flags & _IO_UNBUFFERED)
 	  /* Iff stream is un-orientated, it wasn't used. */
 	  && fp->_mode != 0)

> 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.

Sorry, I couldn't think of a more reliable test than that.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]

[Florian Weimer]

* Dmitry V. Levin:

> On Wed, Jun 19, 2019 at 03:10:14PM +0200, Florian Weimer wrote:
>> * 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.
>
> Yes, this approach seems to work, too:
>
> diff --git a/libio/genops.c b/libio/genops.c
> index 2a0d9b81df..575f0e6584 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_vtable_offset (fp) != 0))
> +	continue;
> +#endif
>        if (! (fp->_flags & _IO_UNBUFFERED)
>  	  /* Iff stream is un-orientated, it wasn't used. */
>  	  && fp->_mode != 0)

Hmm, right there's an early access to fp->_mode that I had missed.
Should we still flush buffers in old binaries?

I think we could do this instead:

  int mode;
#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
  if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
    mode = 1; /* Old streams are never wide.  */
  else
    mode = fp->_mode;
#else
  mode  = fp->_mode;
#endif

And then use mode instead of fp->_mode below.  Does this make sense?

>> 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.
>
> Sorry, I couldn't think of a more reliable test than that.

I came up with this (also with the linker script).  But curiously
enough, the padding is not actually needed.  The test is only valid with
GLIBC_2.0 targets, it crashes for newer targets which do not define
_IO_stdout_.

#include <stdio.h>
#include <stdlib.h>

int pad1[] = {1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 };
int _IO_stdout_[20] __attribute__ ((nocommon));
int pad2[] = {1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 };

int
main (void)
{
  /* Simulate old-style printf.  */
  fprintf ((FILE *) &_IO_stdout_, "info: testing old printing\n");
  return 0;
}

This could be improved by using internal headers when the test is built
within the glibc tree, so that hard-coding the size of _IO_stdout_ is
not required.

I guess we could add both tests, just in case.

Thanks,
Florian

Re: [PATCH v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 3894 bytes --]

On Wed, Jun 19, 2019 at 06:15:06PM +0200, Florian Weimer wrote:
> * Dmitry V. Levin:
> > On Wed, Jun 19, 2019 at 03:10:14PM +0200, Florian Weimer wrote:
> >> * 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.
> >
> > Yes, this approach seems to work, too:
> >
> > diff --git a/libio/genops.c b/libio/genops.c
> > index 2a0d9b81df..575f0e6584 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_vtable_offset (fp) != 0))
> > +	continue;
> > +#endif
> >        if (! (fp->_flags & _IO_UNBUFFERED)
> >  	  /* Iff stream is un-orientated, it wasn't used. */
> >  	  && fp->_mode != 0)
> 
> Hmm, right there's an early access to fp->_mode that I had missed.
> Should we still flush buffers in old binaries?
> 
> I think we could do this instead:
> 
>   int mode;
> #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
>   if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
>     mode = 1; /* Old streams are never wide.  */
>   else
>     mode = fp->_mode;
> #else
>   mode  = fp->_mode;
> #endif
> 
> And then use mode instead of fp->_mode below.  Does this make sense?

_mode is not the only member that is not available in legacy libio,
_freeres_list and _freeres_buf also should be avoided.
Would you prefer the following approach?

diff --git a/libio/genops.c b/libio/genops.c
index 2a0d9b81df..11a15549e8 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -789,9 +789,16 @@ _IO_unbuffer_all (void)
 
   for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain)
     {
+      int legacy = 0;
+
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
+      if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
+	legacy = 1;
+#endif
+
       if (! (fp->_flags & _IO_UNBUFFERED)
 	  /* Iff stream is un-orientated, it wasn't used. */
-	  && fp->_mode != 0)
+	  && (legacy || fp->_mode != 0))
 	{
 #ifdef _IO_MTSAFE_IO
 	  int cnt;
@@ -805,7 +812,7 @@ _IO_unbuffer_all (void)
 	      __sched_yield ();
 #endif
 
-	  if (! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
+	  if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
 	    {
 	      fp->_flags |= _IO_USER_BUF;
 
@@ -816,7 +823,7 @@ _IO_unbuffer_all (void)
 
 	  _IO_SETBUF (fp, NULL, 0);
 
-	  if (fp->_mode > 0)
+	  if (! legacy && fp->_mode > 0)
 	    _IO_wsetb (fp, NULL, NULL, 0);
 
 #ifdef _IO_MTSAFE_IO
@@ -827,7 +834,8 @@ _IO_unbuffer_all (void)
 
       /* Make sure that never again the wide char functions can be
 	 used.  */
-      fp->_mode = -1;
+      if (! legacy)
+	fp->_mode = -1;
     }
 
 #ifdef _IO_MTSAFE_IO


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]

[Florian Weimer]

* Dmitry V. Levin:

> _mode is not the only member that is not available in legacy libio,
> _freeres_list and _freeres_buf also should be avoided.
> Would you prefer the following approach?
>
> diff --git a/libio/genops.c b/libio/genops.c
> index 2a0d9b81df..11a15549e8 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -789,9 +789,16 @@ _IO_unbuffer_all (void)
>  
>    for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain)
>      {
> +      int legacy = 0;
> +
> +#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
> +      if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
> +	legacy = 1;
> +#endif

This approach looks reasonable to me.  It's certainly much better than
what we have today.  If you post a full patch, I'll test it against
one of the problematic OpenJDK 8 builds.

[PATCH v4] libio: do not attempt to free wide buffers of legacy streams [BZ #24228]

[Dmitry V. Levin]

Commit a601b74d31ca086de38441d316a3dee24c866305 aka glibc-2.23~693
("In preparation for fixing BZ#16734, fix failure in misc/tst-error1-mem
when _G_HAVE_MMAP is turned off.") 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.

[BZ #24228]
* libio/genops.c (_IO_unbuffer_all)
[SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)]: Do not attempt to free wide
buffers and access _IO_FILE_complete members of legacy libio streams.
* 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        | 11 ++++++++++-
 libio/genops.c        | 16 ++++++++++++----
 libio/tst-bz24228.c   | 29 +++++++++++++++++++++++++++++
 libio/tst-bz24228.map |  5 +++++
 5 files changed, 70 insertions(+), 5 deletions(-)
 create mode 100644 libio/tst-bz24228.c
 create mode 100644 libio/tst-bz24228.map

diff --git a/libio/Makefile b/libio/Makefile
index 7d53cb06b0..b81e8bf5e2 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 \
 	tst-wfile-sync tst-wfile-gconv
 
 # This test tests interaction with the gconv cache.  Setting
@@ -162,16 +162,20 @@ 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
 tst-wfile-gconv-ENV = MALLOC_TRACE=$(objpfx)tst-wfile-gconv.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
 generated += tst-wfile-gconv.mtrace tst-wfile-gconv.check
 
 aux	:= fileops genops stdfiles stdio strops
@@ -188,6 +192,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-bz24228-mem.out \
 		 $(objpfx)tst-wfile-gconv-mem.out
 ifeq (yes,$(build-shared))
 # Run tst-fopenloc-cmp.out and tst-openloc-mem.out only if shared
@@ -246,6 +251,10 @@ $(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)
+
 $(objpfx)tst-wfile-gconv-mem.out: $(objpfx)tst-wfile-gconv.out
 	$(common-objpfx)malloc/mtrace $(objpfx)tst-wfile-gconv.mtrace > $@; \
 	$(evaluate-test)
diff --git a/libio/genops.c b/libio/genops.c
index 2a0d9b81df..11a15549e8 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -789,9 +789,16 @@ _IO_unbuffer_all (void)
 
   for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain)
     {
+      int legacy = 0;
+
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
+      if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
+	legacy = 1;
+#endif
+
       if (! (fp->_flags & _IO_UNBUFFERED)
 	  /* Iff stream is un-orientated, it wasn't used. */
-	  && fp->_mode != 0)
+	  && (legacy || fp->_mode != 0))
 	{
 #ifdef _IO_MTSAFE_IO
 	  int cnt;
@@ -805,7 +812,7 @@ _IO_unbuffer_all (void)
 	      __sched_yield ();
 #endif
 
-	  if (! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
+	  if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
 	    {
 	      fp->_flags |= _IO_USER_BUF;
 
@@ -816,7 +823,7 @@ _IO_unbuffer_all (void)
 
 	  _IO_SETBUF (fp, NULL, 0);
 
-	  if (fp->_mode > 0)
+	  if (! legacy && fp->_mode > 0)
 	    _IO_wsetb (fp, NULL, NULL, 0);
 
 #ifdef _IO_MTSAFE_IO
@@ -827,7 +834,8 @@ _IO_unbuffer_all (void)
 
       /* Make sure that never again the wide char functions can be
 	 used.  */
-      fp->_mode = -1;
+      if (! legacy)
+	fp->_mode = -1;
     }
 
 #ifdef _IO_MTSAFE_IO
diff --git a/libio/tst-bz24228.c b/libio/tst-bz24228.c
new file mode 100644
index 0000000000..6a74500d47
--- /dev/null
+++ b/libio/tst-bz24228.c
@@ -0,0 +1,29 @@
+/* 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..4383e0817d
--- /dev/null
+++ b/libio/tst-bz24228.map
@@ -0,0 +1,5 @@
+# Hide the symbol from libc.so.6 to switch to the libio/oldfileops.c
+# implementation when it is available for the architecture.
+{
+  local: _IO_stdin_used;
+};
-- 
ldv

Re: [PATCH v4] libio: do not attempt to free wide buffers of legacy streams [BZ #24228]

[Florian Weimer]

* Dmitry V. Levin:

> Commit a601b74d31ca086de38441d316a3dee24c866305 aka glibc-2.23~693
> ("In preparation for fixing BZ#16734, fix failure in misc/tst-error1-mem
> when _G_HAVE_MMAP is turned off.") 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.

GNU style calls for not putting () after function names.  (Sorry, I must
sound like a broken record).

> diff --git a/libio/Makefile b/libio/Makefile
> index 7d53cb06b0..b81e8bf5e2 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 \
>  	tst-wfile-sync tst-wfile-gconv

I suggest to qualify the new test with “ifeq ($(build-shared),yes)”.  It
can never meet its test objective with static linking because the
relevant code just is not there.

Rest looks good to me, thanks.

I can confirm that it fixes the OpenJDK failure we saw.

Thanks,
Florian

[PATCH v5] libio: do not attempt to free wide buffers of legacy streams [BZ #24228]

[Dmitry V. Levin]

Commit a601b74d31ca086de38441d316a3dee24c866305 aka glibc-2.23~693
("In preparation for fixing BZ#16734, fix failure in misc/tst-error1-mem
when _G_HAVE_MMAP is turned off.") 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.

[BZ #24228]
* libio/genops.c (_IO_unbuffer_all)
[SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)]: Do not attempt to free wide
buffers and access _IO_FILE_complete members of legacy libio streams.
* libio/tst-bz24228.c: New file.
* libio/tst-bz24228.map: Likewise.
* libio/Makefile [build-shared] (tests): Add tst-bz24228.
[build-shared] (generated): Add tst-bz24228.mtrace and
tst-bz24228.check.
[run-built-tests && build-shared] (tests-special): Add
$(objpfx)tst-bz24228-mem.out.
(LDFLAGS-tst-bz24228, tst-bz24228-ENV): New variables.
($(objpfx)tst-bz24228-mem.out): New rule.
---
 ChangeLog             | 17 +++++++++++++++++
 libio/Makefile        | 14 +++++++++++++-
 libio/genops.c        | 16 ++++++++++++----
 libio/tst-bz24228.c   | 29 +++++++++++++++++++++++++++++
 libio/tst-bz24228.map |  5 +++++
 5 files changed, 76 insertions(+), 5 deletions(-)
 create mode 100644 libio/tst-bz24228.c
 create mode 100644 libio/tst-bz24228.map

diff --git a/ChangeLog b/ChangeLog
index 729301f75e..a795d6ac77 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+2019-06-19  Dmitry V. Levin  <ldv@altlinux.org>
+	    Florian Weimer  <fweimer@redhat.com>
+
+	[BZ #24228]
+	* libio/genops.c (_IO_unbuffer_all)
+	[SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)]: Do not attempt to free wide
+	buffers and access _IO_FILE_complete members of legacy libio streams.
+	* libio/tst-bz24228.c: New file.
+	* libio/tst-bz24228.map: Likewise.
+	* libio/Makefile [build-shared] (tests): Add tst-bz24228.
+	[build-shared] (generated): Add tst-bz24228.mtrace and
+	tst-bz24228.check.
+	[run-built-tests && build-shared] (tests-special): Add
+	$(objpfx)tst-bz24228-mem.out.
+	(LDFLAGS-tst-bz24228, tst-bz24228-ENV): New variables.
+	($(objpfx)tst-bz24228-mem.out): New rule.
+
 2019-06-19  Rafal Luzynski  <digitalfreak@lingonborough.com>
 
 	[BZ #24614]
diff --git a/libio/Makefile b/libio/Makefile
index 7d53cb06b0..6e594b8ec5 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -78,6 +78,9 @@ ifeq (yes,$(build-shared))
 # Add test-fopenloc only if shared library is enabled since it depends on
 # shared localedata objects.
 tests += tst-fopenloc
+# Add tst-bz24228 only if shared library is enabled since it can never meet its
+# objective with static linking because the relevant code just is not there.
+tests += tst-bz24228
 endif
 test-srcs = test-freopen
 
@@ -162,11 +165,14 @@ 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
 tst-wfile-gconv-ENV = MALLOC_TRACE=$(objpfx)tst-wfile-gconv.mtrace
 
 generated += test-fmemopen.mtrace test-fmemopen.check
@@ -177,6 +183,7 @@ generated += tst-wfile-gconv.mtrace tst-wfile-gconv.check
 aux	:= fileops genops stdfiles stdio strops
 
 ifeq ($(build-shared),yes)
+generated += tst-bz24228.mtrace tst-bz24228.check
 aux	+= oldfileops oldstdfiles
 endif
 
@@ -192,7 +199,8 @@ tests-special += $(objpfx)test-freopen.out $(objpfx)test-fmemopen-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.
-tests-special += $(objpfx)tst-fopenloc-cmp.out $(objpfx)tst-fopenloc-mem.out
+tests-special += $(objpfx)tst-fopenloc-cmp.out $(objpfx)tst-fopenloc-mem.out \
+		 $(objpfx)tst-bz24228-mem.out
 endif
 endif
 
@@ -246,6 +254,10 @@ $(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)
+
 $(objpfx)tst-wfile-gconv-mem.out: $(objpfx)tst-wfile-gconv.out
 	$(common-objpfx)malloc/mtrace $(objpfx)tst-wfile-gconv.mtrace > $@; \
 	$(evaluate-test)
diff --git a/libio/genops.c b/libio/genops.c
index 2a0d9b81df..11a15549e8 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -789,9 +789,16 @@ _IO_unbuffer_all (void)
 
   for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain)
     {
+      int legacy = 0;
+
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
+      if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
+	legacy = 1;
+#endif
+
       if (! (fp->_flags & _IO_UNBUFFERED)
 	  /* Iff stream is un-orientated, it wasn't used. */
-	  && fp->_mode != 0)
+	  && (legacy || fp->_mode != 0))
 	{
 #ifdef _IO_MTSAFE_IO
 	  int cnt;
@@ -805,7 +812,7 @@ _IO_unbuffer_all (void)
 	      __sched_yield ();
 #endif
 
-	  if (! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
+	  if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
 	    {
 	      fp->_flags |= _IO_USER_BUF;
 
@@ -816,7 +823,7 @@ _IO_unbuffer_all (void)
 
 	  _IO_SETBUF (fp, NULL, 0);
 
-	  if (fp->_mode > 0)
+	  if (! legacy && fp->_mode > 0)
 	    _IO_wsetb (fp, NULL, NULL, 0);
 
 #ifdef _IO_MTSAFE_IO
@@ -827,7 +834,8 @@ _IO_unbuffer_all (void)
 
       /* Make sure that never again the wide char functions can be
 	 used.  */
-      fp->_mode = -1;
+      if (! legacy)
+	fp->_mode = -1;
     }
 
 #ifdef _IO_MTSAFE_IO
diff --git a/libio/tst-bz24228.c b/libio/tst-bz24228.c
new file mode 100644
index 0000000000..6a74500d47
--- /dev/null
+++ b/libio/tst-bz24228.c
@@ -0,0 +1,29 @@
+/* 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..4383e0817d
--- /dev/null
+++ b/libio/tst-bz24228.map
@@ -0,0 +1,5 @@
+# Hide the symbol from libc.so.6 to switch to the libio/oldfileops.c
+# implementation when it is available for the architecture.
+{
+  local: _IO_stdin_used;
+};
-- 
ldv

Re: [PATCH v5] libio: do not attempt to free wide buffers of legacy streams [BZ #24228]

[Florian Weimer]

* Dmitry V. Levin:

> Commit a601b74d31ca086de38441d316a3dee24c866305 aka glibc-2.23~693
> ("In preparation for fixing BZ#16734, fix failure in misc/tst-error1-mem
> when _G_HAVE_MMAP is turned off.") 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.
>
> [BZ #24228]
> * libio/genops.c (_IO_unbuffer_all)
> [SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)]: Do not attempt to free wide
> buffers and access _IO_FILE_complete members of legacy libio streams.
> * libio/tst-bz24228.c: New file.
> * libio/tst-bz24228.map: Likewise.
> * libio/Makefile [build-shared] (tests): Add tst-bz24228.
> [build-shared] (generated): Add tst-bz24228.mtrace and
> tst-bz24228.check.
> [run-built-tests && build-shared] (tests-special): Add
> $(objpfx)tst-bz24228-mem.out.
> (LDFLAGS-tst-bz24228, tst-bz24228-ENV): New variables.
> ($(objpfx)tst-bz24228-mem.out): New rule.

This version looks fine to me.  Thanks.

Sorry, it took me a while to really understand this issue.

Florian

Re: [PATCH v5] libio: do not attempt to free wide buffers of legacy streams [BZ #24228]

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

On Thu, Jun 20, 2019 at 10:59:28AM +0200, Florian Weimer wrote:
> * Dmitry V. Levin:
> 
> > Commit a601b74d31ca086de38441d316a3dee24c866305 aka glibc-2.23~693
> > ("In preparation for fixing BZ#16734, fix failure in misc/tst-error1-mem
> > when _G_HAVE_MMAP is turned off.") 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.
> >
> > [BZ #24228]
> > * libio/genops.c (_IO_unbuffer_all)
> > [SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)]: Do not attempt to free wide
> > buffers and access _IO_FILE_complete members of legacy libio streams.
> > * libio/tst-bz24228.c: New file.
> > * libio/tst-bz24228.map: Likewise.
> > * libio/Makefile [build-shared] (tests): Add tst-bz24228.
> > [build-shared] (generated): Add tst-bz24228.mtrace and
> > tst-bz24228.check.
> > [run-built-tests && build-shared] (tests-special): Add
> > $(objpfx)tst-bz24228-mem.out.
> > (LDFLAGS-tst-bz24228, tst-bz24228-ENV): New variables.
> > ($(objpfx)tst-bz24228-mem.out): New rule.
> 
> This version looks fine to me.  Thanks.

Pushed.  Thanks.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]