From patchwork Thu Aug 17 13:35:07 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 22190 Received: (qmail 110801 invoked by alias); 17 Aug 2017 13:35:45 -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 109449 invoked by uid 89); 17 Aug 2017 13:35:18 -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=orientation, lx X-HELO: mx1.redhat.com DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 91C0A2EC6D3 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=fweimer@redhat.com Date: Thu, 17 Aug 2017 15:35:07 +0200 To: libc-alpha@sourceware.org Subject: [PATCH] abort: Only flush file-based stdio streams before termination User-Agent: Heirloom mailx 12.5 7/5/10 MIME-Version: 1.0 Message-Id: <20170817133507.CEA5341DB79B0@oldenburg.str.redhat.com> From: fweimer@redhat.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 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 -#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 + . */ + +#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