RE: unbuffered fread() deadlock

["Howland Craig D (Craig)" <howland at LGSInnovations dot com>]

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.