[v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228]
Commit Message
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
* 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
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.
* 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
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.
* 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
@@ -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)
@@ -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)
new file mode 100644
@@ -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>
new file mode 100644
@@ -0,0 +1,3 @@
+{
+ local: _IO_stdin_used;
+};