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

  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(-)
  

* 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
  

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.
  

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.