Re: [PATCH] abort: Do not flush stdio streams [BZ #15436]

[Florian Weimer <fweimer at redhat dot com>]

On 08/30/2017 09:17 PM, Adhemerval Zanella wrote:
> 
> 
> 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.

No, I posted a patch generated from the wrong branch.

Thanks,
Florian

Do not flush stdio streams on abort [BZ #15436]

2017-08-30  Florian Weimer  <fweimer@redhat.com>

	[BZ #15436]
	Do not flush stdio streams on abort.
	* stdlib/abort.c (fflush): Remove macro definition.
	(abort): Remove stages related to stdio flushing.

diff --git a/NEWS b/NEWS
index 625bcc60b6..221015bd4b 100644
--- a/NEWS
+++ b/NEWS
@@ -20,6 +20,10 @@ Major new features:
   leads to lower overall process restart latency, so there is benefit both
   from a security and performance perspective.
 
+* The abort function terminates the process immediately, without flushing
+  stdio streams.  Previous glibc versions used to flush streams, resulting
+  in deadlocks and further data corruption.
+
 Deprecated and removed features, and other changes affecting compatibility:
 
 * On GNU/Linux, the obsolete Linux constant PTRACE_SEIZE_DEVEL is no longer
diff --git a/stdlib/abort.c b/stdlib/abort.c
index 19882f3e3d..117a507ff8 100644
--- a/stdlib/abort.c
+++ b/stdlib/abort.c
@@ -31,9 +31,6 @@
 # define ABORT_INSTRUCTION
 #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));
 libc_hidden_def (__abort_msg)
@@ -67,16 +64,8 @@ abort (void)
       __sigprocmask (SIG_UNBLOCK, &sigs, 0);
     }
 
-  /* Flush all streams.  We cannot close them now because the user
-     might have registered a handler for SIGABRT.  */
-  if (stage == 1)
-    {
-      ++stage;
-      fflush (NULL);
-    }
-
   /* Send signal which possibly calls a user handler.  */
-  if (stage == 2)
+  if (stage == 1)
     {
       /* This stage is special: we must allow repeated calls of
 	 `abort' when a user defined handler for SIGABRT is installed.
@@ -94,7 +83,7 @@ abort (void)
     }
 
   /* There was a handler installed.  Now remove it.  */
-  if (stage == 3)
+  if (stage == 2)
     {
       ++stage;
       memset (&act, '\0', sizeof (struct sigaction));
@@ -104,30 +93,22 @@ abort (void)
       __sigaction (SIGABRT, &act, NULL);
     }
 
-  /* Now close the streams which also flushes the output the user
-     defined handler might has produced.  */
-  if (stage == 4)
-    {
-      ++stage;
-      __fcloseall ();
-    }
-
   /* Try again.  */
-  if (stage == 5)
+  if (stage == 3)
     {
       ++stage;
       raise (SIGABRT);
     }
 
   /* Now try to abort using the system specific command.  */
-  if (stage == 6)
+  if (stage == 4)
     {
       ++stage;
       ABORT_INSTRUCTION;
     }
 
   /* If we can't signal ourselves and the abort instruction failed, exit.  */
-  if (stage == 7)
+  if (stage == 5)
     {
       ++stage;
       _exit (127);