This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] abort: Do not flush stdio streams [BZ #15436]
Here's an alternative patch which removes flushing completely. This is
what Andreas suggested.
I've added a short NEWS entry.
Thanks,
Florian
abort: Only flush file-based stdio streams before termination [BZ #15436]
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>
[BZ #15436]
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. Call it for the second round of flushing, instead of
__fcloseall.
* 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 2da39e067c..e85e919fa0 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -81,7 +81,7 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \
tst-quick_exit tst-thread-quick_exit tst-width \
tst-width-stdint tst-strfrom tst-strfrom-locale \
tst-getrandom tst-atexit tst-at_quick_exit \
- tst-cxa_atexit tst-on_exit
+ tst-cxa_atexit tst-on_exit tst-abort-fflush \
tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
tst-tls-atexit tst-tls-atexit-nodelete
diff --git a/stdlib/abort.c b/stdlib/abort.c
index 19882f3e3d..9bb39e0a45 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. */
@@ -104,12 +104,12 @@ abort (void)
__sigaction (SIGABRT, &act, NULL);
}
- /* Now close the streams which also flushes the output the user
- defined handler might has produced. */
+ /* Now flush the output the user defined handler might has
+ produced. */
if (stage == 4)
{
++stage;
- __fcloseall ();
+ _IO_flush_all_lockp (0, 1);
}
/* Try again. */
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>