Message ID | 20190619160340.GA23394@altlinux.org |
---|---|
State | Superseded |
Headers |
Received: (qmail 2326 invoked by alias); 19 Jun 2019 16:03:49 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 2174 invoked by uid 89); 19 Jun 2019 16:03:48 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: vmicros1.altlinux.org Date: Wed, 19 Jun 2019 19:03:40 +0300 From: "Dmitry V. Levin" <ldv@altlinux.org> To: Florian Weimer <fweimer@redhat.com> Cc: libc-alpha@sourceware.org Subject: Re: [PATCH v3] libio: do not unbuffer legacy standard files in compatibility mode [BZ #24228] Message-ID: <20190619160340.GA23394@altlinux.org> References: <20190218124438.GB20127@altlinux.org> <87mumtcl0w.fsf@oldenburg2.str.redhat.com> <20190218191021.GA25527@altlinux.org> <20190219005741.GA29070@altlinux.org> <20190219012913.GB29070@altlinux.org> <87lfxx4t6x.fsf@oldenburg2.str.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gKMricLos+KVdGMg" Content-Disposition: inline In-Reply-To: <87lfxx4t6x.fsf@oldenburg2.str.redhat.com> |
Commit Message
Dmitry V. Levin
June 19, 2019, 4:03 p.m. UTC
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: > 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.
Comments
* 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
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)