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

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

-- 
ldv

Comments

Florian Weimer March 13, 2019, 3:08 p.m. | #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. | #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.


-- 
ldv
Florian Weimer March 13, 2019, 3:49 p.m. | #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. | #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.


-- 
ldv
Florian Weimer June 19, 2019, 1:10 p.m. | #5
* 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
Dmitry V. Levin June 19, 2019, 4:03 p.m. | #6
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
Florian Weimer June 19, 2019, 4:15 p.m. | #7
* 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
Dmitry V. Levin June 19, 2019, 5:46 p.m. | #8
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
Florian Weimer June 19, 2019, 7:04 p.m. | #9
* 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

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