[PATCH] Always do locking when accessing streams

[Andreas Schwab <schwab at suse dot de>]

Trying to go through the open streams without locking is a futile
attempt.  Another thread may be calling fclose any time which may result
in _IO_cleanup to crash on dead territory.  POSIX has already
acknowleged that exit may block while closing the streams (see
<http://austingroupbugs.net/view.php?id=611>).  Similarily, abort is no
longer required (since POSIX.1-2004) to close all streams, which is
incompatible with being async-signal-safe.

Andreas.

	[BZ #15142]
	* libio/genops.c (_IO_list_all_stamp): Delete.  All uses removed.
	(_IO_flush_all_all_lockp): Delete.
	(_IO_flush_all): Replace with body of _IO_flush_all_all_lockp.
	Always do locking.
	(_IO_unbuffer_write): Always do locking.
	(_IO_cleanup): Call _IO_flush_all instead of _IO_flush_all_lockp.
	* libio/libioP.h (_IO_flush_all_all_lockp): Remove declaration.
	* stdlib/abort.c (abort): Don't call fflush and __fcloseall.
---
 libio/genops.c | 95 ++++++++++++++++------------------------------------------
 libio/libioP.h |  1 -
 stdlib/abort.c | 29 ++++--------------
 3 files changed, 31 insertions(+), 94 deletions(-)

diff --git a/libio/genops.c b/libio/genops.c
index 390d8d2..05d4a41 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -38,10 +38,6 @@
 static _IO_lock_t list_all_lock = _IO_lock_initializer;
 #endif
 
-/* Used to signal modifications to the list of FILE decriptors.  */
-static int _IO_list_all_stamp;
-
-
 static _IO_FILE *run_fp;
 
 #ifdef _IO_MTSAFE_IO
@@ -70,16 +66,12 @@ _IO_un_link (fp)
       if (_IO_list_all == NULL)
 	;
       else if (fp == _IO_list_all)
-	{
-	  _IO_list_all = (struct _IO_FILE_plus *) _IO_list_all->file._chain;
-	  ++_IO_list_all_stamp;
-	}
+	_IO_list_all = (struct _IO_FILE_plus *) _IO_list_all->file._chain;
       else
 	for (f = &_IO_list_all->file._chain; *f; f = &(*f)->_chain)
 	  if (*f == (_IO_FILE *) fp)
 	    {
 	      *f = fp->file._chain;
-	      ++_IO_list_all_stamp;
 	      break;
 	    }
       fp->file._flags &= ~_IO_LINKED;
@@ -108,7 +100,6 @@ _IO_link_in (fp)
 #endif
       fp->file._chain = (_IO_FILE *) _IO_list_all;
       _IO_list_all = fp;
-      ++_IO_list_all_stamp;
 #ifdef _IO_MTSAFE_IO
       _IO_funlockfile ((_IO_FILE *) fp);
       run_fp = NULL;
@@ -814,25 +805,20 @@ _IO_get_column (fp)
 
 
 int
-_IO_flush_all_lockp (int do_lock)
+_IO_flush_all (void)
 {
   int result = 0;
   struct _IO_FILE *fp;
-  int last_stamp;
 
 #ifdef _IO_MTSAFE_IO
-  __libc_cleanup_region_start (do_lock, flush_cleanup, NULL);
-  if (do_lock)
-    _IO_lock_lock (list_all_lock);
+  _IO_cleanup_region_start_noarg (flush_cleanup);
+  _IO_lock_lock (list_all_lock);
 #endif
 
-  last_stamp = _IO_list_all_stamp;
-  fp = (_IO_FILE *) _IO_list_all;
-  while (fp != NULL)
+  for (fp = (_IO_FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
-      if (do_lock)
-	_IO_flockfile (fp);
+      _IO_flockfile (fp);
 
       if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base)
 #if defined _LIBC || defined _GLIBCPP_USE_WCHAR_T
@@ -844,52 +830,30 @@ _IO_flush_all_lockp (int do_lock)
 	  && _IO_OVERFLOW (fp, EOF) == EOF)
 	result = EOF;
 
-      if (do_lock)
-	_IO_funlockfile (fp);
+      _IO_funlockfile (fp);
       run_fp = NULL;
-
-      if (last_stamp != _IO_list_all_stamp)
-	{
-	  /* Something was added to the list.  Start all over again.  */
-	  fp = (_IO_FILE *) _IO_list_all;
-	  last_stamp = _IO_list_all_stamp;
-	}
-      else
-	fp = fp->_chain;
     }
 
 #ifdef _IO_MTSAFE_IO
-  if (do_lock)
-    _IO_lock_unlock (list_all_lock);
-  __libc_cleanup_region_end (0);
+  _IO_lock_unlock (list_all_lock);
+  _IO_cleanup_region_end (0);
 #endif
 
   return result;
 }
-
-
-int
-_IO_flush_all ()
-{
-  /* We want locking.  */
-  return _IO_flush_all_lockp (1);
-}
 libc_hidden_def (_IO_flush_all)
 
 void
 _IO_flush_all_linebuffered ()
 {
   struct _IO_FILE *fp;
-  int last_stamp;
 
 #ifdef _IO_MTSAFE_IO
   _IO_cleanup_region_start_noarg (flush_cleanup);
   _IO_lock_lock (list_all_lock);
 #endif
 
-  last_stamp = _IO_list_all_stamp;
-  fp = (_IO_FILE *) _IO_list_all;
-  while (fp != NULL)
+  for (fp = (_IO_FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
       _IO_flockfile (fp);
@@ -899,15 +863,6 @@ _IO_flush_all_linebuffered ()
 
       _IO_funlockfile (fp);
       run_fp = NULL;
-
-      if (last_stamp != _IO_list_all_stamp)
-	{
-	  /* Something was added to the list.  Start all over again.  */
-	  fp = (_IO_FILE *) _IO_list_all;
-	  last_stamp = _IO_list_all_stamp;
-	}
-      else
-	fp = fp->_chain;
     }
 
 #ifdef _IO_MTSAFE_IO
@@ -943,6 +898,12 @@ static void
 _IO_unbuffer_write (void)
 {
   struct _IO_FILE *fp;
+
+#ifdef _IO_MTSAFE_IO
+  _IO_cleanup_region_start_noarg (flush_cleanup);
+  _IO_lock_lock (list_all_lock);
+#endif
+
   for (fp = (_IO_FILE *) _IO_list_all; fp; fp = fp->_chain)
     {
       if (! (fp->_flags & _IO_UNBUFFERED)
@@ -952,15 +913,8 @@ _IO_unbuffer_write (void)
 	  && fp->_mode != 0)
 	{
 #ifdef _IO_MTSAFE_IO
-	  int cnt;
-#define MAXTRIES 2
-	  for (cnt = 0; cnt < MAXTRIES; ++cnt)
-	    if (fp->_lock == NULL || _IO_lock_trylock (*fp->_lock) == 0)
-	      break;
-	    else
-	      /* Give the other thread time to finish up its use of the
-		 stream.  */
-	      __sched_yield ();
+	  run_fp = fp;
+	  _IO_flockfile (fp);
 #endif
 
 	  if (! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
@@ -976,8 +930,8 @@ _IO_unbuffer_write (void)
 	  _IO_SETBUF (fp, NULL, 0);
 
 #ifdef _IO_MTSAFE_IO
-	  if (cnt < MAXTRIES && fp->_lock != NULL)
-	    _IO_lock_unlock (*fp->_lock);
+	  _IO_funlockfile (fp);
+	  run_fp = NULL;
 #endif
 	}
 
@@ -985,6 +939,11 @@ _IO_unbuffer_write (void)
 	 used.  */
       fp->_mode = -1;
     }
+
+#ifdef _IO_MTSAFE_IO
+  _IO_lock_unlock (list_all_lock);
+  _IO_cleanup_region_end (0);
+#endif
 }
 
 
@@ -1004,9 +963,7 @@ libc_freeres_fn (buffer_free)
 int
 _IO_cleanup ()
 {
-  /* We do *not* want locking.  Some threads might use streams but
-     that is their problem, we flush them underneath them.  */
-  int result = _IO_flush_all_lockp (0);
+  int result = _IO_flush_all ();
 
   /* We currently don't have a reliable mechanism for making sure that
      C++ static destructors are executed in the correct order.
diff --git a/libio/libioP.h b/libio/libioP.h
index 911f649..4dd8bc0 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -487,7 +487,6 @@ extern int _IO_new_do_write (_IO_FILE *, const char *, _IO_size_t);
 extern int _IO_old_do_write (_IO_FILE *, const char *, _IO_size_t);
 extern int _IO_wdo_write (_IO_FILE *, const wchar_t *, _IO_size_t);
 libc_hidden_proto (_IO_wdo_write)
-extern int _IO_flush_all_lockp (int);
 extern int _IO_flush_all (void);
 libc_hidden_proto (_IO_flush_all)
 extern int _IO_cleanup (void);
diff --git a/stdlib/abort.c b/stdlib/abort.c
index 72b2d60..4b7df18 100644
--- a/stdlib/abort.c
+++ b/stdlib/abort.c
@@ -30,9 +30,6 @@
 # define ABORT_INSTRUCTION
 #endif
 
-#include <libio/libioP.h>
-#define fflush(s) _IO_flush_all_lockp (0)
-
 /* Exported variable to locate abort message in core files etc.  */
 struct abort_msg_s *__abort_msg __attribute__ ((nocommon));
 libc_hidden_def (__abort_msg)
@@ -66,16 +63,8 @@ abort (void)
 	__sigprocmask (SIG_UNBLOCK, &sigs, (sigset_t *) NULL);
     }
 
-  /* Flush all streams.  We cannot close them now because the user
-     might have registered a handler for SIGABRT.  */
-  if (stage == 1)
-    {
-      ++stage;
-      fflush (NULL);
-    }
-
   /* Send signal which possibly calls a user handler.  */
-  if (stage == 2)
+  if (stage == 1)
     {
       /* This stage is special: we must allow repeated calls of
 	 `abort' when a user defined handler for SIGABRT is installed.
@@ -93,7 +82,7 @@ abort (void)
     }
 
   /* There was a handler installed.  Now remove it.  */
-  if (stage == 3)
+  if (stage == 2)
     {
       ++stage;
       memset (&act, '\0', sizeof (struct sigaction));
@@ -103,30 +92,22 @@ abort (void)
       __sigaction (SIGABRT, &act, NULL);
     }
 
-  /* Now close the streams which also flushes the output the user
-     defined handler might has produced.  */
-  if (stage == 4)
-    {
-      ++stage;
-      __fcloseall ();
-    }
-
   /* Try again.  */
-  if (stage == 5)
+  if (stage == 3)
     {
       ++stage;
       raise (SIGABRT);
     }
 
   /* Now try to abort using the system specific command.  */
-  if (stage == 6)
+  if (stage == 4)
     {
       ++stage;
       ABORT_INSTRUCTION;
     }
 
   /* If we can't signal ourselves and the abort instruction failed, exit.  */
-  if (stage == 7)
+  if (stage == 5)
     {
       ++stage;
       _exit (127);
-- 
1.8.2.1

-- 
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."