From patchwork Wed Aug 30 15:52:37 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 22422 Received: (qmail 127954 invoked by alias); 30 Aug 2017 15:52:47 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 127905 invoked by uid 89); 30 Aug 2017 15:52:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=termination, lx, flushing X-HELO: mx1.redhat.com DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 690327EA94 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=fweimer@redhat.com Subject: Re: [PATCH] abort: Do not flush stdio streams [BZ #15436] To: Carlos O'Donell Cc: libc-alpha@sourceware.org, Andreas Schwab References: <20170817133507.CEA5341DB79B0@oldenburg.str.redhat.com> <5453e6d9-11dd-5dce-f38f-4c8793864ddc@redhat.com> <1ded54c6-5796-b844-3cc5-9dbf21671c2d@redhat.com> <5e2d5837-3344-8710-f2c8-265d404c2205@redhat.com> From: Florian Weimer Message-ID: <0f8e9f6b-18bb-f045-c662-ea6c468c78d6@redhat.com> Date: Wed, 30 Aug 2017 17:52:37 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: 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 [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 -#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 + . */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* 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