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]
On 30/08/2017 12:52, Florian Weimer wrote:
> Here's an alternative patch which removes flushing completely. This is
> what Andreas suggested.
>
> I've added a short NEWS entry.
So the idea is still partial flush, but without internal locking? Regarding
partial flushing, I still think we can remove malloc altogether in
open_memstream overflow, so there is no need to make is only for file
operations (I have it my backlog to send a patch for it).
Also I think you missed the NEWS entry.
>
> Thanks,
> Florian
>
>
> abort-no-flush.patch
>
>
> 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>
>