[PATCH] abort: Only flush file-based stdio streams before termination

[fweimer at redhat dot com (Florian Weimer)]

Historically, glibc flushes streams on abort, which is not
required by POSIX.  This can trigger additional work
(including callbacks through function pointers) in processes
which are known to be in a bad state.  After this change,
only streams which are backed by the standard descriptor-based
implementation are flushed.

2017-08-17  Florian Weimer  <fweimer@redhat.com>

	Only flush file-based stdio streams on process abort.
	* libio/genops.c (_IO_flush_all_lockp): Add restrict_vtables
	parameter.  Do not call __overflow on vtable mismatch.
	(_IO_flush_all): Adjust call to _IO_flush_all_lockp.
	(_IO_cleanup): Likewise.
	* libio/libioP.h (_IO_JUMPS_FUNC_NOVALIDATE): New macro.
	(_IO_JUMPS_FUNC): Use it.
	(_IO_flush_all_lockp): Add restrict_vtables parameter.  Make
	hidden.
	* stdlib/abort.c (fflush): Remove macro definition.
	(abort): Call _IO_flush_all_lockp directly, with vtable
	restriction.
	* stdlib/tst-abort-fflush.c: Newfile.
	* stdlib/Makefile (tests): Add it.

diff --git a/libio/genops.c b/libio/genops.c
index 6ad7346cae..d97196ebef 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -790,7 +790,7 @@ _IO_get_column (_IO_FILE *fp)
 
 
 int
-_IO_flush_all_lockp (int do_lock)
+_IO_flush_all_lockp (int do_lock, int restrict_vtables)
 {
   int result = 0;
   struct _IO_FILE *fp;
@@ -816,9 +816,32 @@ _IO_flush_all_lockp (int do_lock)
 	       && fp->_mode > 0 && (fp->_wide_data->_IO_write_ptr
 				    > fp->_wide_data->_IO_write_base))
 #endif
-	   )
-	  && _IO_OVERFLOW (fp, EOF) == EOF)
-	result = EOF;
+	   ))
+	{
+	  /* Only invoke __overflow if the vtable refers to an actual
+	     file.  (mmap'ed files are read-only and do not need
+	     flushing in this mode.)  */
+	  const struct _IO_jump_t *vtable = NULL;
+	  if (restrict_vtables)
+	    {
+	      vtable = _IO_JUMPS_FUNC_NOVALIDATE (fp);
+	      if (!(vtable == &_IO_file_jumps
+		    || vtable == &_IO_wfile_jumps
+#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
+		    || vtable == &_IO_old_file_jumps
+#endif
+		    ))
+		vtable = NULL;
+	    }
+	  else
+	    vtable = _IO_JUMPS_FUNC (fp);
+
+	  if (vtable != NULL)
+	    {
+	      if (vtable->__overflow (fp, EOF))
+		result = EOF;
+	    }
+	}
 
       if (do_lock)
 	_IO_funlockfile (fp);
@@ -848,7 +871,7 @@ int
 _IO_flush_all (void)
 {
   /* We want locking.  */
-  return _IO_flush_all_lockp (1);
+  return _IO_flush_all_lockp (1, 0);
 }
 libc_hidden_def (_IO_flush_all)
 
@@ -982,7 +1005,7 @@ _IO_cleanup (void)
 {
   /* 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_lockp (0, 0);
 
   /* 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 1832b44cc7..bfdcb8e949 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -123,16 +123,21 @@ extern "C" {
 #define _IO_CHECK_WIDE(THIS) \
   (_IO_CAST_FIELD_ACCESS ((THIS), struct _IO_FILE, _wide_data) != NULL)
 
+/* _IO_JUMPS_FUNC_NOVALIDATE (THIS) expands to a vtable pointer for THIS,
+   possibly adjusted by the vtable offset, _IO_vtable_offset (THIS).  */
 #if _IO_JUMPS_OFFSET
-# define _IO_JUMPS_FUNC(THIS) \
-  (IO_validate_vtable                                                   \
-   (*(struct _IO_jump_t **) ((void *) &_IO_JUMPS_FILE_plus (THIS)	\
-			     + (THIS)->_vtable_offset)))
+# define _IO_JUMPS_FUNC_NOVALIDATE(THIS)				\
+  (*(struct _IO_jump_t **) ((void *) &_IO_JUMPS_FILE_plus (THIS)	\
+			    + (THIS)->_vtable_offset))
 # define _IO_vtable_offset(THIS) (THIS)->_vtable_offset
 #else
-# define _IO_JUMPS_FUNC(THIS) (IO_validate_vtable (_IO_JUMPS_FILE_plus (THIS)))
+# define _IO_JUMPS_FUNC_NOVALIDATE(THIS) (_IO_JUMPS_FILE_plus (THIS))
 # define _IO_vtable_offset(THIS) 0
-#endif
+#endif /* _IO_JUMPS_OFFSET */
+
+/* Like _IO_JUMPS_FUNC_NOVALIDATE, but perform vtable validation.  */
+#define _IO_JUMPS_FUNC(THIS) \
+  (IO_validate_vtable (_IO_JUMPS_FUNC_NOVALIDATE (THIS)))
 #define _IO_WIDE_JUMPS_FUNC(THIS) _IO_WIDE_JUMPS(THIS)
 #define JUMP_FIELD(TYPE, NAME) TYPE NAME
 #define JUMP0(FUNC, THIS) (_IO_JUMPS_FUNC(THIS)->FUNC) (THIS)
@@ -507,7 +512,13 @@ 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);
+
+/* If DO_LOCK, perform locking.  If RESTRICT_VTABLES, only flush
+   streams which refer to real files (based on their vtable); this is
+   used for restricted flushing on abort.  */
+extern int _IO_flush_all_lockp (int do_lock, int restrict_vtables)
+  attribute_hidden;
+
 extern int _IO_flush_all (void);
 libc_hidden_proto (_IO_flush_all)
 extern int _IO_cleanup (void);
diff --git a/stdlib/Makefile b/stdlib/Makefile
index e4b36ca118..e1e428cf41 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -80,7 +80,7 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-strtol-locale tst-strtod-nan-locale tst-strfmon_l    \
 		   tst-quick_exit tst-thread-quick_exit tst-width	    \
 		   tst-width-stdint tst-strfrom tst-strfrom-locale	    \
-		   tst-getrandom
+		   tst-getrandom tst-abort-fflush
 tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
 		   tst-tls-atexit tst-tls-atexit-nodelete
 tests-static	:= tst-secure-getenv
diff --git a/stdlib/abort.c b/stdlib/abort.c
index 19882f3e3d..79ec2a9cd4 100644
--- a/stdlib/abort.c
+++ b/stdlib/abort.c
@@ -32,7 +32,6 @@
 #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));
@@ -72,7 +71,8 @@ abort (void)
   if (stage == 1)
     {
       ++stage;
-      fflush (NULL);
+      /* Flush streams without locking, but only file streams.  */
+      _IO_flush_all_lockp (0, 1);
     }
 
   /* Send signal which possibly calls a user handler.  */
diff --git a/stdlib/tst-abort-fflush.c b/stdlib/tst-abort-fflush.c
new file mode 100644
index 0000000000..9d2eedce1d
--- /dev/null
+++ b/stdlib/tst-abort-fflush.c
@@ -0,0 +1,219 @@
+/* Check stream-flushing behavior on abort.
+   Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <signal.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <support/capture_subprocess.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <wchar.h>
+
+/* Test streams and their file names.  */
+static char *name_fdopen;
+static FILE *stream_fdopen;
+static char *name_fopen;
+static FILE *stream_fopen;
+static char *name_fopen_wide;
+static FILE *stream_fopen_wide;
+static FILE *stream_fopencookie;
+
+/* Shared data with subprocess, to detect fopencookie flushing.  */
+static struct
+{
+  unsigned int write_called;
+  bool allow_close;
+} *shared;
+
+static void
+check_size (const char *name, const char *path, off64_t expected)
+{
+  struct stat64 st;
+  xstat (path, &st);
+  if (st.st_size != expected)
+    {
+      support_record_failure ();
+      printf ("error: file for %s (%s) does not have size %llu, got %llu\n",
+              name, path, (unsigned long long) expected,
+              (unsigned long long) st.st_size);
+    }
+}
+
+static void
+check_all_empty (void)
+{
+  check_size ("fdopen", name_fdopen, 0);
+  check_size ("fopen", name_fopen, 0);
+  check_size ("fopen (wide orientation)", name_fopen_wide, 0);
+  TEST_VERIFY (shared->write_called == 0);
+}
+
+static void
+check_all_written (void)
+{
+  check_size ("fdopen", name_fdopen, 1);
+  check_size ("fopen (wide orientation)", name_fopen_wide, 1);
+  TEST_VERIFY (shared->write_called == 1);
+}
+
+static ssize_t
+cookieread (void *cookie, char *buf, size_t count)
+{
+  FAIL_EXIT1 ("cookieread called");
+}
+
+static ssize_t
+cookiewrite (void *cookie, const char *buf, size_t count)
+{
+  ++shared->write_called;
+  return count;
+}
+
+static int
+cookieseek (void *cookie, off64_t *offset, int whence)
+{
+  return 0;
+}
+
+static int
+cookieclose (void *cookie)
+{
+  if (!shared->allow_close)
+    FAIL_EXIT1 ("cookieclose called");
+  return 0;
+}
+
+static void
+pretest (void)
+{
+  shared = support_shared_allocate (sizeof (*shared));
+
+  /* Create the streams.  */
+  {
+    int fd = create_temp_file ("tst-abort-fflush", &name_fdopen);
+    TEST_VERIFY_EXIT (fd >= 0);
+    stream_fdopen = fdopen (fd, "w");
+    TEST_VERIFY_EXIT (stream_fdopen != NULL);
+  }
+  {
+    int fd = create_temp_file ("tst-abort-fflush", &name_fopen);
+    TEST_VERIFY_EXIT (fd >= 0);
+    xclose (fd);
+    stream_fopen = xfopen (name_fopen, "w");
+  }
+  {
+    int fd = create_temp_file ("tst-abort-fflush", &name_fopen_wide);
+    TEST_VERIFY_EXIT (fd >= 0);
+    xclose (fd);
+    stream_fopen_wide = xfopen (name_fopen_wide, "w,ccs=utf-8");
+  }
+
+  stream_fopencookie = fopencookie (NULL, "w", (cookie_io_functions_t)
+                                    {
+                                      .read = cookieread,
+                                      .write = cookiewrite,
+                                      .seek = cookieseek,
+                                      .close = cookieclose
+                                    });
+  TEST_VERIFY_EXIT (stream_fopencookie != NULL);
+  shared->write_called = 0;
+  shared->allow_close = false;
+
+  TEST_VERIFY (fputc ('X', stream_fdopen) == 'X');
+  TEST_VERIFY (fputc ('X', stream_fopen) == 'X');
+  TEST_VERIFY (fputwc (L'X', stream_fopen_wide) == L'X');
+  TEST_VERIFY (fputc ('X', stream_fopencookie) == 'X');
+
+  /* No flushing must have happened at this point.  */
+  check_all_empty ();
+}
+
+static void
+posttest (void)
+{
+  xfclose (stream_fdopen);
+  xfclose (stream_fopen);
+  xfclose (stream_fopen_wide);
+  shared->allow_close = true;
+  xfclose (stream_fopencookie);
+  free (name_fdopen);
+  free (name_fopen);
+  free (name_fopen_wide);
+  support_shared_free (shared);
+  shared = NULL;
+}
+
+static void
+run_exit (void *unused)
+{
+  exit (0);
+}
+
+static void
+run_abort (void *unused)
+{
+  TEST_VERIFY (fputc ('X', stdout) == 'X');
+  TEST_VERIFY (fputc ('Y', stderr) == 'Y');
+  abort ();
+}
+
+static int
+do_test (void)
+{
+  /* Check that fflush flushes all streams.  */
+  pretest ();
+  fflush (NULL);
+  check_all_written ();
+  posttest ();
+
+  /* Check that exit flushes all streams.  */
+  pretest ();
+  support_isolate_in_subprocess (run_exit, NULL);
+  check_all_written ();
+  posttest ();
+
+  /* Check that abort only flushes file streams.  */
+  pretest ();
+  {
+    struct support_capture_subprocess proc
+      = support_capture_subprocess (run_abort, NULL);
+    TEST_VERIFY (proc.out.length == 1);
+    TEST_VERIFY (proc.out.buffer[0] == 'X');
+    TEST_VERIFY (proc.err.length == 1);
+    TEST_VERIFY (proc.err.buffer[0] == 'Y');
+    TEST_VERIFY (WIFSIGNALED (proc.status));
+    TEST_VERIFY (WTERMSIG (proc.status) == SIGABRT);
+    check_size ("fdopen", name_fdopen, 1);
+    check_size ("fopen", name_fopen, 1);
+    check_size ("fopen (wide)", name_fopen_wide, 1);
+    TEST_VERIFY (shared->write_called == 0);
+    support_capture_subprocess_free (&proc);
+  }
+  posttest ();
+
+  return 0;
+}
+
+#include <support/test-driver.c>