Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Florian Weimer]

Do we have to revert the fix fpr bug 1190 (which introduces sticky EOF 
for stdio streams)?

We received a bug report that cups-filters is quite broken due to this 
change, and while reviewing the libio sources, I found evidence that 
what cups-filters does (dup2-ing another descriptor after EOF) was once 
considered supported (in _IO_new_file_underflow):

   fp->_IO_read_end += count;
   if (count == 0)
     {
       /* If a stream is read to EOF, the calling application may switch 
active
	 handles.  As a result, our offset cache would no longer be valid, so
	 unset it.  */
       fp->_offset = _IO_pos_BAD;
       return EOF;
     }

Usually, when our own testing finds bugs, that's that's just a small 
subset of what the larger user community (many of whom cannot recompile 
their applications) will experience once they upgrade, so I'm quite 
worried about the implications of the fix for bug 1190.

Thanks,
Florian

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Rich Felker]

On Mon, Sep 17, 2018 at 03:37:03PM +0200, Florian Weimer wrote:
> Do we have to revert the fix fpr bug 1190 (which introduces sticky
> EOF for stdio streams)?
> 
> We received a bug report that cups-filters is quite broken due to
> this change, and while reviewing the libio sources, I found evidence
> that what cups-filters does (dup2-ing another descriptor after EOF)
> was once considered supported (in _IO_new_file_underflow):
> 
>   fp->_IO_read_end += count;
>   if (count == 0)
>     {
>       /* If a stream is read to EOF, the calling application may
> switch active
> 	 handles.  As a result, our offset cache would no longer be valid, so
> 	 unset it.  */
>       fp->_offset = _IO_pos_BAD;
>       return EOF;
>     }
> 
> Usually, when our own testing finds bugs, that's that's just a small
> subset of what the larger user community (many of whom cannot
> recompile their applications) will experience once they upgrade, so
> I'm quite worried about the implications of the fix for bug 1190.

I've always considered dup2'ing to temporarily or permanently switch
the fd underlying a FILE to be reasonable usage, since it's something
that needs to be doable (especially for the standard streams, which
can't be replaced) and freopen has multiple fatal flaws that make it
unsuitable to the task. However this does not absolve the appliction
of a requirement to clear the EOF status if the stream might be at EOF
when switching. Failure to do so is an application bug that was masked
by the glibc bug.

Rich

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Florian Weimer]

Rich Felker <dalias@libc.org> writes:

> On Mon, Sep 17, 2018 at 03:37:03PM +0200, Florian Weimer wrote:
>> Do we have to revert the fix fpr bug 1190 (which introduces sticky
>> EOF for stdio streams)?
>> 
>> We received a bug report that cups-filters is quite broken due to
>> this change, and while reviewing the libio sources, I found evidence
>> that what cups-filters does (dup2-ing another descriptor after EOF)
>> was once considered supported (in _IO_new_file_underflow):
>> 
>>   fp->_IO_read_end += count;
>>   if (count == 0)
>>     {
>>       /* If a stream is read to EOF, the calling application may
>> switch active
>> 	 handles.  As a result, our offset cache would no longer be valid, so
>> 	 unset it.  */
>>       fp->_offset = _IO_pos_BAD;
>>       return EOF;
>>     }
>> 
>> Usually, when our own testing finds bugs, that's that's just a small
>> subset of what the larger user community (many of whom cannot
>> recompile their applications) will experience once they upgrade, so
>> I'm quite worried about the implications of the fix for bug 1190.
>
> I've always considered dup2'ing to temporarily or permanently switch
> the fd underlying a FILE to be reasonable usage, since it's something
> that needs to be doable (especially for the standard streams, which
> can't be replaced) and freopen has multiple fatal flaws that make it
> unsuitable to the task. However this does not absolve the appliction
> of a requirement to clear the EOF status if the stream might be at EOF
> when switching. Failure to do so is an application bug that was masked
> by the glibc bug.

What worries me that we have had code for a long time, explicitly
enabling applications which do *not* seek, beyond the fact that we
simply did not implement sticky EOF.  This could perhaps be considered a
glibc extension.

Thanks,
Florian

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Rich Felker]

On Mon, Sep 17, 2018 at 04:37:55PM +0200, Florian Weimer wrote:
> Rich Felker <dalias@libc.org> writes:
> 
> > On Mon, Sep 17, 2018 at 03:37:03PM +0200, Florian Weimer wrote:
> >> Do we have to revert the fix fpr bug 1190 (which introduces sticky
> >> EOF for stdio streams)?
> >> 
> >> We received a bug report that cups-filters is quite broken due to
> >> this change, and while reviewing the libio sources, I found evidence
> >> that what cups-filters does (dup2-ing another descriptor after EOF)
> >> was once considered supported (in _IO_new_file_underflow):
> >> 
> >>   fp->_IO_read_end += count;
> >>   if (count == 0)
> >>     {
> >>       /* If a stream is read to EOF, the calling application may
> >> switch active
> >> 	 handles.  As a result, our offset cache would no longer be valid, so
> >> 	 unset it.  */
> >>       fp->_offset = _IO_pos_BAD;
> >>       return EOF;
> >>     }
> >> 
> >> Usually, when our own testing finds bugs, that's that's just a small
> >> subset of what the larger user community (many of whom cannot
> >> recompile their applications) will experience once they upgrade, so
> >> I'm quite worried about the implications of the fix for bug 1190.
> >
> > I've always considered dup2'ing to temporarily or permanently switch
> > the fd underlying a FILE to be reasonable usage, since it's something
> > that needs to be doable (especially for the standard streams, which
> > can't be replaced) and freopen has multiple fatal flaws that make it
> > unsuitable to the task. However this does not absolve the appliction
> > of a requirement to clear the EOF status if the stream might be at EOF
> > when switching. Failure to do so is an application bug that was masked
> > by the glibc bug.
> 
> What worries me that we have had code for a long time, explicitly
> enabling applications which do *not* seek, beyond the fact that we
> simply did not implement sticky EOF.

You don't need to seek to clear it; clearerr(f) is the easiest way.

> This could perhaps be considered a glibc extension.

All bugs are extensions. :-)

If it's really a problem maybe there's some mechanism for versioning
the fix or otherwise maintaining compatibility with existing
applications (e.g. when hitting sticky EOF, probing whether the
underlying open file is still the same), but I really hope this
doesn't derail the fix.

Getting back to the particular software broken, cups-filter, how did
it work on non-glibc systems without this bug? Was the code being used
glibc-specific? Or was it just untested elsewhere?

Rich

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Andreas Schwab]

On Sep 17 2018, Florian Weimer <fweimer@redhat.com> wrote:

> What worries me that we have had code for a long time, explicitly
> enabling applications which do *not* seek, beyond the fact that we
> simply did not implement sticky EOF.  This could perhaps be considered a
> glibc extension.

Is there any indication that there are more packages that depend on this
extension?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Florian Weimer]

Rich Felker <dalias@libc.org> writes:

>> What worries me that we have had code for a long time, explicitly
>> enabling applications which do *not* seek, beyond the fact that we
>> simply did not implement sticky EOF.
>
> You don't need to seek to clear it; clearerr(f) is the easiest way.

True.

>> This could perhaps be considered a glibc extension.
>
> All bugs are extensions. :-)
>
> If it's really a problem maybe there's some mechanism for versioning
> the fix or otherwise maintaining compatibility with existing
> applications (e.g. when hitting sticky EOF, probing whether the
> underlying open file is still the same), but I really hope this
> doesn't derail the fix.

We already do an fstat, so we could capture the device/inode information
and capture that to be reused at a later EOF.

To version the fix, symbol versioning would have to be introduced for
all functions which create descriptors, and the fix would have to be
reverted in 2.28 because no new symbol versions can be added after a
release.

> Getting back to the particular software broken, cups-filter, how did
> it work on non-glibc systems without this bug? Was the code being used
> glibc-specific? Or was it just untested elsewhere?

The current dependencies of cups-filters look sufficiently heavy that
it's possible that only glibc targets were used with it.

Thanks,
Florian

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Florian Weimer]

Andreas Schwab <schwab@suse.de> writes:

> On Sep 17 2018, Florian Weimer <fweimer@redhat.com> wrote:
>
>> What worries me that we have had code for a long time, explicitly
>> enabling applications which do *not* seek, beyond the fact that we
>> simply did not implement sticky EOF.  This could perhaps be considered a
>> glibc extension.
>
> Is there any indication that there are more packages that depend on this
> extension?

Not yet, no.

Thanks,
Florian

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Carlos O'Donell]

On 09/17/2018 10:58 AM, Florian Weimer wrote:
> Andreas Schwab <schwab@suse.de> writes:
> 
>> On Sep 17 2018, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> What worries me that we have had code for a long time, explicitly
>>> enabling applications which do *not* seek, beyond the fact that we
>>> simply did not implement sticky EOF.  This could perhaps be considered a
>>> glibc extension.
>>
>> Is there any indication that there are more packages that depend on this
>> extension?
> 
> Not yet, no.

However, as Florian notes, when we see one failure like this in the corpus
of packages we ship it has historically been an indicator that there exists
code which also depends on it. This is just correlation, not causation though,
it might be that cups-filter is the only thing broken by this fix.

I'm inclined to side with Rich that "All bugs are extensions."

Is cups-filter fixed by inserting a clearerr(f)?

Or does glibc's current implementation disallow concat via dup2?

-- 
Cheers,
Carlos.

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Adam Sampson]

Florian Weimer <fweimer@redhat.com> writes:

>> Getting back to the particular software broken, cups-filter, how did
>> it work on non-glibc systems without this bug? Was the code being used
>> glibc-specific? Or was it just untested elsewhere?
> The current dependencies of cups-filters look sufficiently heavy that
> it's possible that only glibc targets were used with it.

cups-filters is included in the FreeBSD ports collection, but they've
been carrying a patch since 2015 that adds clearerr(stdin); in the
appropriate place:

https://svnweb.freebsd.org/ports/head/print/cups-filters/files/patch-filter_foomatic-rip_foomaticrip.c?revision=411031&view=markup

-- 
Adam Sampson <ats@offog.org>                         <http://offog.org/>

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Carlos O'Donell]

On 09/17/2018 11:14 AM, Adam Sampson wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>>> Getting back to the particular software broken, cups-filter, how did
>>> it work on non-glibc systems without this bug? Was the code being used
>>> glibc-specific? Or was it just untested elsewhere?
>> The current dependencies of cups-filters look sufficiently heavy that
>> it's possible that only glibc targets were used with it.
> 
> cups-filters is included in the FreeBSD ports collection, but they've
> been carrying a patch since 2015 that adds clearerr(stdin); in the
> appropriate place:
> 
> https://svnweb.freebsd.org/ports/head/print/cups-filters/files/patch-filter_foomatic-rip_foomaticrip.c?revision=411031&view=markup
 
Adam,

Thank you *very* much for chiming in. That data point is indicative
that we may have just a few packages that fail like this.

Do you have any visibility into the ports tree to see how many
other patches add clearerr(.*);?

-- 
Cheers,
Carlos.

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Florian Weimer]

Carlos O'Donell <carlos@redhat.com> writes:

> Is cups-filter fixed by inserting a clearerr(f)?

The proposed fix uses rewind, which is equivalent in this case and
presumably more portable.

Thanks,
Florian

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Rich Felker]

On Mon, Sep 17, 2018 at 11:17:20AM -0400, Carlos O'Donell wrote:
> On 09/17/2018 11:14 AM, Adam Sampson wrote:
> > Florian Weimer <fweimer@redhat.com> writes:
> > 
> >>> Getting back to the particular software broken, cups-filter, how did
> >>> it work on non-glibc systems without this bug? Was the code being used
> >>> glibc-specific? Or was it just untested elsewhere?
> >> The current dependencies of cups-filters look sufficiently heavy that
> >> it's possible that only glibc targets were used with it.
> > 
> > cups-filters is included in the FreeBSD ports collection, but they've
> > been carrying a patch since 2015 that adds clearerr(stdin); in the
> > appropriate place:
> > 
> > https://svnweb.freebsd.org/ports/head/print/cups-filters/files/patch-filter_foomatic-rip_foomaticrip.c?revision=411031&view=markup
>  
> Adam,
> 
> Thank you *very* much for chiming in. That data point is indicative
> that we may have just a few packages that fail like this.
> 
> Do you have any visibility into the ports tree to see how many
> other patches add clearerr(.*);?

Maybe getting a bit off-topic, but something is really wrong with FOSS
processes of getting fixes upstream if FreeBSD ports had this trivial
fix since 2015 and it didn't make it upstream.

Rich

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Rich Felker]

On Mon, Sep 17, 2018 at 05:18:48PM +0200, Florian Weimer wrote:
> Carlos O'Donell <carlos@redhat.com> writes:
> 
> > Is cups-filter fixed by inserting a clearerr(f)?
> 
> The proposed fix uses rewind, which is equivalent in this case and
> presumably more portable.

I think clearerr is better. rewind is essentially specified as
clearerr(f)+fseek(f,0,SEEK_SET), and in general the extra seek may be
undesirable. Both interfaces are plain C and have been since C89 or
earlier.

Rich

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Adam Sampson]

Rich Felker <dalias@libc.org> writes:

>> Do you have any visibility into the ports tree to see how many
>> other patches add clearerr(.*);?

Looking at:
https://github.com/freebsd/freebsd-ports/search?q=clearerr

The only other patch adding a call to clearerr is in colortail, and
looking at the original code, that doesn't seem to be the same kind of
problem since it's not replacing the fd:
https://github.com/joakim666/colortail/blob/master/TailFile.cc#L385

> Maybe getting a bit off-topic, but something is really wrong with FOSS
> processes of getting fixes upstream if FreeBSD ports had this trivial
> fix since 2015 and it didn't make it upstream.

Well, maybe, but it's quite possible that the port author didn't realise
it was a general bugfix (rather than a FreeBSD-specific thing); I've
certainly made the same mistake when writing ports in the past!

Cheers,

-- 
Adam Sampson <ats@offog.org>                         <http://offog.org/>

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Rich Felker]

On Mon, Sep 17, 2018 at 04:38:24PM +0100, Adam Sampson wrote:
> Rich Felker <dalias@libc.org> writes:
> 
> >> Do you have any visibility into the ports tree to see how many
> >> other patches add clearerr(.*);?
> 
> Looking at:
> https://github.com/freebsd/freebsd-ports/search?q=clearerr
> 
> The only other patch adding a call to clearerr is in colortail, and
> looking at the original code, that doesn't seem to be the same kind of
> problem since it's not replacing the fd:
> https://github.com/joakim666/colortail/blob/master/TailFile.cc#L385
> 
> > Maybe getting a bit off-topic, but something is really wrong with FOSS
> > processes of getting fixes upstream if FreeBSD ports had this trivial
> > fix since 2015 and it didn't make it upstream.
> 
> Well, maybe, but it's quite possible that the port author didn't realise
> it was a general bugfix (rather than a FreeBSD-specific thing); I've
> certainly made the same mistake when writing ports in the past!

Yes, it's not my intent to assign blame here; rather, I think there's
a lack of publicized education and communication on distinguishing the
two and on getting things like this seen by the eyes who are already
able to distinguish. I'm not sure how we best address that.

Rich

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Florian Weimer]

* Florian Weimer:

> Do we have to revert the fix fpr bug 1190 (which introduces sticky EOF
> for stdio streams)?
>
> We received a bug report that cups-filters is quite broken due to this

The upstream bug: <https://github.com/OpenPrinting/cups-filters/issues/58>

> change, and while reviewing the libio sources, I found evidence that
> what cups-filters does (dup2-ing another descriptor after EOF) was
> once considered supported (in _IO_new_file_underflow):
>
>   fp->_IO_read_end += count;
>   if (count == 0)
>     {
>       /* If a stream is read to EOF, the calling application may
> switch active
> 	 handles.  As a result, our offset cache would no longer be valid, so
> 	 unset it.  */
>       fp->_offset = _IO_pos_BAD;
>       return EOF;
>     }
>
> Usually, when our own testing finds bugs, that's that's just a small
> subset of what the larger user community (many of whom cannot
> recompile their applications) will experience once they upgrade, so
> I'm quite worried about the implications of the fix for bug 1190.

Folks,

do I read the emerging consensus correctly as the following?

glibc should not change (e.g., revert the fix for bug 1190) to paper
over application bugs such as the bug in cups-filters.  Applications
need to be fixed instead.  End users whose applications cannot be fixed
should keep with previous glibc versions.

Thanks,
Florian

Re: Sticky EOF breaks stream concatenation via dup2 [BZ #23636]

[Adhemerval Zanella]

On 20/09/2018 05:28, Florian Weimer wrote:
> * Florian Weimer:
> 
>> Do we have to revert the fix fpr bug 1190 (which introduces sticky EOF
>> for stdio streams)?
>>
>> We received a bug report that cups-filters is quite broken due to this
> 
> The upstream bug: <https://github.com/OpenPrinting/cups-filters/issues/58>
> 
>> change, and while reviewing the libio sources, I found evidence that
>> what cups-filters does (dup2-ing another descriptor after EOF) was
>> once considered supported (in _IO_new_file_underflow):
>>
>>   fp->_IO_read_end += count;
>>   if (count == 0)
>>     {
>>       /* If a stream is read to EOF, the calling application may
>> switch active
>> 	 handles.  As a result, our offset cache would no longer be valid, so
>> 	 unset it.  */
>>       fp->_offset = _IO_pos_BAD;
>>       return EOF;
>>     }
>>
>> Usually, when our own testing finds bugs, that's that's just a small
>> subset of what the larger user community (many of whom cannot
>> recompile their applications) will experience once they upgrade, so
>> I'm quite worried about the implications of the fix for bug 1190.
> 
> Folks,
> 
> do I read the emerging consensus correctly as the following?
> 
> glibc should not change (e.g., revert the fix for bug 1190) to paper
> over application bugs such as the bug in cups-filters.  Applications
> need to be fixed instead.  End users whose applications cannot be fixed
> should keep with previous glibc versions.

I agree with you on this specific issue, even more after the FreeBSD
ports required path indicated it is indeed an application problem.