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
  

On 08/30/2017 10:52 AM, Florian Weimer wrote:
> Here's an alternative patch which removes flushing completely.  This is
> what Andreas suggested.
> 
> I've added a short NEWS entry.

Could you please also add a wiki note for 
https://sourceware.org/glibc/wiki/Release/2.27#Packaging_Changes
which explains what happened and why?

If we get enough queries from users we can add to the text and include some
code to get back some of the old flushing behaviour.
  

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>
>
  

On 08/30/2017 06:22 PM, Carlos O'Donell wrote:
> On 08/30/2017 10:52 AM, Florian Weimer wrote:
>> Here's an alternative patch which removes flushing completely.  This is
>> what Andreas suggested.
>>
>> I've added a short NEWS entry.
> 
> Could you please also add a wiki note for
> https://sourceware.org/glibc/wiki/Release/2.27#Packaging_Changes
> which explains what happened and why?

Do we have consensus for this patch?  I haven't pushed it yet.

I originally had thought of it as more of a glibc-3.0-type project.

Thanks,
Florian
  

On 09/18/2017 08:33 AM, Florian Weimer wrote:
> On 08/30/2017 06:22 PM, Carlos O'Donell wrote:
>> On 08/30/2017 10:52 AM, Florian Weimer wrote:
>>> Here's an alternative patch which removes flushing completely.  This is
>>> what Andreas suggested.
>>>
>>> I've added a short NEWS entry.
>>
>> Could you please also add a wiki note for
>> https://sourceware.org/glibc/wiki/Release/2.27#Packaging_Changes
>> which explains what happened and why?
> 
> Do we have consensus for this patch?  I haven't pushed it yet.

I thought we did :-)

However, if you, the author of the patch, aren't sure, then it is certainly
important to repost your patch and gather another round of review.

The key point is that we don't have to do this flushing if we don't have to
and it's hard to see anyone claiming they needed these exact semantics given
that abort() is an abnormal termination scenario.
 
> I originally had thought of it as more of a glibc-3.0-type project.

Sure, we should probably start to put together a wiki page with glibc-3.0
kinds of changes?
  

On 09/20/2017 11:30 PM, Carlos O'Donell wrote:
> On 09/18/2017 08:33 AM, Florian Weimer wrote:
>> On 08/30/2017 06:22 PM, Carlos O'Donell wrote:
>>> On 08/30/2017 10:52 AM, Florian Weimer wrote:
>>>> Here's an alternative patch which removes flushing completely.  This is
>>>> what Andreas suggested.
>>>>
>>>> I've added a short NEWS entry.
>>>
>>> Could you please also add a wiki note for
>>> https://sourceware.org/glibc/wiki/Release/2.27#Packaging_Changes
>>> which explains what happened and why?
>>
>> Do we have consensus for this patch?  I haven't pushed it yet.
> 
> I thought we did :-)
> 
> However, if you, the author of the patch, aren't sure, then it is certainly
> important to repost your patch and gather another round of review.
> 
> The key point is that we don't have to do this flushing if we don't have to
> and it's hard to see anyone claiming they needed these exact semantics given
> that abort() is an abnormal termination scenario.

There were no objections to the patch:

   <https://sourceware.org/ml/libc-alpha/2017-08/msg01304.html>

I'm going to commit it shortly and update the wiki as well.

Thanks,
Florian
  

On 05/10/2017 07:48, Florian Weimer wrote:
> On 09/20/2017 11:30 PM, Carlos O'Donell wrote:
>> On 09/18/2017 08:33 AM, Florian Weimer wrote:
>>> On 08/30/2017 06:22 PM, Carlos O'Donell wrote:
>>>> On 08/30/2017 10:52 AM, Florian Weimer wrote:
>>>>> Here's an alternative patch which removes flushing completely.  This is
>>>>> what Andreas suggested.
>>>>>
>>>>> I've added a short NEWS entry.
>>>>
>>>> Could you please also add a wiki note for
>>>> https://sourceware.org/glibc/wiki/Release/2.27#Packaging_Changes
>>>> which explains what happened and why?
>>>
>>> Do we have consensus for this patch?  I haven't pushed it yet.
>>
>> I thought we did :-)
>>
>> However, if you, the author of the patch, aren't sure, then it is certainly
>> important to repost your patch and gather another round of review.
>>
>> The key point is that we don't have to do this flushing if we don't have to
>> and it's hard to see anyone claiming they needed these exact semantics given
>> that abort() is an abnormal termination scenario.
> 
> There were no objections to the patch:
> 
>   <https://sourceware.org/ml/libc-alpha/2017-08/msg01304.html>
> 
> I'm going to commit it shortly and update the wiki as well.

LGTM thanks.