[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.
Florian Weimer June 19, 2019, 1:10 p.m. UTC | #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. UTC | #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.
Florian Weimer June 19, 2019, 4:15 p.m. UTC | #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. UTC | #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
Florian Weimer June 19, 2019, 7:04 p.m. UTC | #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 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;
+};