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

Message ID 20190619220834.GA28646@altlinux.org
State New
Headers show
Series
  • [v5] libio: do not attempt to free wide buffers of legacy streams [BZ #24228]
Related show

Commit Message

Dmitry V. Levin June 19, 2019, 10:08 p.m.
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

-- 
ldv

Comments

Florian Weimer June 20, 2019, 8:59 a.m. | #1
* 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
Dmitry V. Levin June 20, 2019, 5:42 p.m. | #2
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

Patch

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