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

Message ID 20190218124438.GB20127@altlinux.org
State Superseded
Headers show
Series
  • libio: do not cleanup wide buffers of legacy standard files [BZ #24228]
Related show

Commit Message

Dmitry V. Levin Feb. 18, 2019, 12:44 p.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 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(-)

-- 
ldv

Comments

Florian Weimer Feb. 18, 2019, 12:56 p.m. | #1
* 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
Dmitry V. Levin Feb. 18, 2019, 7:10 p.m. | #2
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
Dmitry V. Levin Feb. 19, 2019, 12:57 a.m. | #3
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

Patch

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)