unbuffered fread() deadlock

[Jeff Johnston]

I believe the source of it is from C99 7.19.3.  Note the furthermore clause:

"When a stream is unbuffered, characters are intended to appear from the 
source or at the destination
as soon as possible.  Otherwise characters may be accumulated and 
transmitted to or from the host
environment as a block.  when a sgtream is fully buffered, characters 
are intended to be transmitted to
or from the host environment as a block when a buffer is filled.  When a 
stream is line buffered, characters
are intended to be transmitted to or from the host environment as a 
block when a new-line character is
encountered.  Furthermore, characters are intended to be transmitted as 
a block to the host environment
when a buffer is filled, when input is requested on an unbuffered 
stream, or when input is requested on
a line buffered stream that requires the transmission of characters from 
the host environment.  Support
for these characteristics is implementation-defined, and may be affected 
via the setbuf and setvbuf functions."

The removal of the current stream from the list of fps to walk will 
solve Andre's problem.  This will occur if we just remove the 
fp-lock/fp-unlock from fwalk since the lflush function being called for 
each fp won't call fflush for the current stream that is reading 
anyway.  However, we still have the sfp-lock vs fp-lock problem looming. 

A read will lock the fp and then possibly need to acquire the sfp lock.  
If something else is doing an fwalk as well (e.g. fflush(null) at same 
time), the 2nd fwalk may wait for the read fp to be unlocked and never 
give up the sfp lock that the read fp sits waiting for.

-- Jeff J.

Howland Craig D (Craig) wrote:
> I scanned the C99 standard and POSIX, and did not find any requirement
> that matches the comment from refill.c that Andre mentions about
> flushing
> all line-buffered output files.  (I've wondered about that comment
> before,
> but hadn't chased it down.)  I think that the comment is in error
> and therefore the associated code which performs the operation is
> extraneous and undesirable.  If it could be done away with, it would be
> a
> nice efficiency improvement.  To repeat it for ease of reading:
>   /*
>    * Before reading from a line buffered or unbuffered file,
>    * flush all line buffered output files, per the ANSI C
>    * standard.
>    */
> The only thing that I know of even remotely approaching this is that
> when
> a file is opened for update mode (RW on same fd; "+" in mode flags--see
> C99 section 7.19.5.3 or Opengroup fopen) output may not be followed
> directly by input unless fflush, fseek, fsetpos, or rewind are called
> first.
> (The comment really makes no sense.  Why would completely different
> streams be coupled in such a manner?  The only time all things get
> flushed at the same time is abort or exit, which are unrelated to a
> normal read.)
>  
> Does anybody know the source of the statement, or of another reason not
> to do away with the mentioned behavior?  (Eliminating this behavior
> naturally warrants adequate consideration beforehand.)
>  
> For additional information, FreeBSD has added code in that section to
> deal with a deadlock problem, and netBSD a similar variation
> (pseudo-diff w.r.t. current Newlib, with '+' being FreeBSD and '!' being
> netBSD):
>   /*
>    * Before reading from a line buffered or unbuffered file,
>    * flush all line buffered output files, per the ANSI C
>    * standard.
>    */
> -  if (fp->_flags & (__SLBF | __SNBF))
> -   _CAST_VOID _fwalk (_GLOBAL_REENT, lflush);
> +  if (fp->_flags & (__SLBF|__SNBF)) {
> +          /* Ignore this file in _fwalk to avoid potential deadlock. */
> +          fp->_flags |= __SIGN;
> +          (void) _fwalk(lflush);
> +          fp->_flags &= ~__SIGN;
> +
> +          /* Now flush this file without locking it. */
> +          if ((fp->_flags & (__SLBF|__SWR)) == (__SLBF|__SWR))
> +                  __sflush(fp);
> +  }
> ! if (fp->_flags & (__SLBF|__SNBF)) {
> !		rwlock_rdlock(&__sfp_lock);
> !		(void) _fwalk(lflush);
> !		rwlock_unlock(&__sfp_lock);
> !	}
>   fp->_p = fp->_bf._base;
>
> These could not be used directly as is since the _fwalk() call is
> obviously a little different, nor does Newlib currently have a __SIGN
> flag nor a rwlock_rdlock function.  (Note that both other fwalks
> actually flush more than what the comment calls for, just as Newlib
> presently does.)
>  
> Thus, a counter-proposal to Jeff's solution is to get rid of the
> flush at that point in the code entirely.  (The standards put it on
> the users to properly handle the direction change for R&W.)  A small
> backoff from that is to flush only the fd being refilled if that fd
> is bi-di.  This would still be an "extension" to the standard, but
> would run less risk of applications behaving in a noticeably-different
> fashion than doing no flushing at all.
>  
> Craig Howland
>
> -----Original Message-----
> From: newlib-owner@sourceware.org [mailto:newlib-owner@sourceware.org]
> On Behalf Of Jeff Johnston
> Sent: Monday, January 12, 2009 3:11 PM
> To: Andre Heider
> Cc: newlib@sourceware.org
> Subject: Re: unbuffered fread() deadlock
>
> Andre,
>
>   It's actually more complicated.
>
>   The problem is that the read operation is allowing an fp lock before 
> an sfp lock.  The sfp lock is meant to occur before any fp locks and in 
> all other cases, this is true (e.g. fclose, fopen, fflush(null), 
> fcloseall).  In the read case, we are allowing the fp lock first which 
> is recipe for a deadlock.  Even if we kludge this particular situation 
> you can see that if a read operation starts while doing an fcloseall(), 
> we are back in the same boat.
>
>   Thus said, the answer is to sfp_lock any read operation before locking
>
> it's fp and to sfp_unlock the moment we know there is no refill 
> required.  As an aside, the fwalk code should not be locking any fp and 
> letting the operation determine if locking is required (e.g. the 
> operation might be a read-only operation that doesn't affect the fp
> itself).
>
> -- Jeff J.
>