stdlib: Make abort/_Exit AS-safe (BZ 26275)

The recursive lock used on abort does not synchronize with a new process
creation (either by fork-like interfaces or posix_spawn ones), nor it
is reinitialized after fork().

Also, the SIGABRT unblock before raise() shows another race condition,
where a fork or posix_spawn() call by another thread, just after the
recursive lock release and before the SIGABRT signal, might create
programs with a non-expected signal mask.  With the default option
(without POSIX_SPAWN_SETSIGDEF), the process can see SIG_DFL for
SIGABRT, where it should be SIG_IGN.

To fix the AS-safe, raise() does not change the process signal mask,
and an AS-safe lock is used if a SIGABRT is installed or the process
is blocked or ignored.  With the signal mask change removal,
there is no need to use a recursive loc.  The lock is also taken on
both _Fork() and posix_spawn(), to avoid the spawn process to see the
abort handler as SIG_DFL.

A read-write lock is used to avoid serialize _Fork and posix_spawn
execution.  Both sigaction (SIGABRT) and abort() requires to lock
as writer (since both change the disposition).

The fallback is also simplified: there is no need to use a loop of
ABORT_INSTRUCTION after _exit() (if the syscall does not terminate the
process, the system is broken).

The proposed fix changes how setjmp works on a SIGABRT handler, where
glibc does not save the signal mask.  So usage like the below will now
always abort.

  static volatile int chk_fail_ok;
  static jmp_buf chk_fail_buf;

  static void
  handler (int sig)
  {
    if (chk_fail_ok)
      {
        chk_fail_ok = 0;
        longjmp (chk_fail_buf, 1);
      }
    else
      _exit (127);
  }
  [...]
  signal (SIGABRT, handler);
  [....]
  chk_fail_ok = 1;
  if (! setjmp (chk_fail_buf))
    {
      // Something that can calls abort, like a failed fortify function.
      chk_fail_ok = 0;
      printf ("FAIL\n");
    }

Such cases will need to use sigsetjmp instead.

The _dl_start_profile calls sigaction through _profil, and to avoid
pulling abort() on loader the call is replaced with __libc_sigaction.

Checked on x86_64-linux-gnu and aarch64-linux-gnu.

Reviewed-by: DJ Delorie <dj@redhat.com>

NEWS

@@ -35,6 +35,11 @@ Deprecated and removed features, and other changes affecting compatibility:
 
 * The big-endian ARC port (arceb-linux-gnu) has been removed.
 
+* The abort is now async-signal-safe and its implementation makes longjmp
+  from the SIGABRT handler always abort if set up with setjmp.  Use sigsetjmp
+  to keep the old behavior, where the handler does not stop the process
+  execution.
+
 Changes to build and runtime requirements:
 
   [Add changes to build and runtime requirements here]

debug/test-strcpy_chk.c

@@ -59,7 +59,7 @@ static int test_main (void);
 #include <support/support.h>
 
 volatile int chk_fail_ok;
-jmp_buf chk_fail_buf;
+sigjmp_buf chk_fail_buf;
 
 static void
 handler (int sig)
@@ -86,7 +86,7 @@ do_one_test (impl_t *impl, char *dst, const char *src,
 	return;
 
       chk_fail_ok = 1;
-      if (setjmp (chk_fail_buf) == 0)
+      if (sigsetjmp (chk_fail_buf, 1) == 0)
 	{
 	  res = CALL (impl, dst, src, dlen);
 	  printf ("*** Function %s (%zd; %zd) did not __chk_fail\n",
@@ -214,7 +214,7 @@ do_random_tests (void)
 	      if (impl->test != 1)
 		{
 		  chk_fail_ok = 1;
-		  if (setjmp (chk_fail_buf) == 0)
+		  if (sigsetjmp (chk_fail_buf, 1) == 0)
 		    {
 		      res = (unsigned char *)
 			    CALL (impl, (char *) p2 + align2,

debug/tst-fortify-wide.c

@@ -26,7 +26,7 @@
 
 static volatile int chk_fail_ok;
 static volatile int ret;
-static jmp_buf chk_fail_buf;
+static sigjmp_buf chk_fail_buf;
 
 static void
 handler (int sig)
@@ -49,7 +49,7 @@ static wchar_t wbuf2[20] = L"%ls";
   do { wprintf (L"Failure on line %d\n", __LINE__); ret = 1; } while (0)
 #define CHK_FAIL_START \
   chk_fail_ok = 1;				\
-  if (! setjmp (chk_fail_buf))			\
+  if (! sigsetjmp (chk_fail_buf, 1))		\
     {
 #define CHK_FAIL_END \
       chk_fail_ok = 0;				\

debug/tst-fortify.c

@@ -90,7 +90,7 @@ do_prepare (int argc, char *argv[])
 
 static volatile int chk_fail_ok;
 static volatile int ret;
-static jmp_buf chk_fail_buf;
+static sigjmp_buf chk_fail_buf;
 
 static void
 handler (int sig)
@@ -133,7 +133,7 @@ static int num2 = 987654;
   do { printf ("Failure on line %d\n", __LINE__); ret = 1; } while (0)
 #define CHK_FAIL_START \
   chk_fail_ok = 1;				\
-  if (! setjmp (chk_fail_buf))			\
+  if (! sigsetjmp (chk_fail_buf, 1))		\
     {
 #define CHK_FAIL_END \
       chk_fail_ok = 0;				\

include/bits/unistd_ext.h

@@ -3,4 +3,7 @@
 #ifndef _ISOMAC
 extern int __close_range (unsigned int lowfd, unsigned int highfd, int flags);
 libc_hidden_proto (__close_range);
+
+extern pid_t __gettid (void);
+libc_hidden_proto (__gettid);
 #endif

include/stdlib.h

@@ -20,6 +20,7 @@
 # include <sys/stat.h>
 
 # include <rtld-malloc.h>
+# include <internal-sigset.h>
 
 extern __typeof (strtol_l) __strtol_l;
 extern __typeof (strtoul_l) __strtoul_l;
@@ -77,6 +78,12 @@ libc_hidden_proto (__isoc23_strtoull_l)
 # define strtoull_l __isoc23_strtoull_l
 #endif
 
+extern void __abort_fork_reset_child (void) attribute_hidden;
+extern void __abort_lock_rdlock (internal_sigset_t *set) attribute_hidden;
+extern void __abort_lock_wrlock (internal_sigset_t *set) attribute_hidden;
+extern void __abort_lock_unlock (const internal_sigset_t *set)
+     attribute_hidden;
+
 libc_hidden_proto (exit)
 libc_hidden_proto (abort)
 libc_hidden_proto (getenv)

manual/startup.texi

@@ -1014,10 +1014,7 @@ for this function is in @file{stdlib.h}.
 
 @deftypefun void abort (void)
 @standards{ISO, stdlib.h}
-@safety{@prelim{}@mtsafe{}@asunsafe{@asucorrupt{}}@acunsafe{@aculock{} @acucorrupt{}}}
+@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
-@c The implementation takes a recursive lock and attempts to support
-@c calls from signal handlers, but if we're in the middle of flushing or
-@c using streams, we may encounter them in inconsistent states.
 The @code{abort} function causes abnormal program termination.  This
 does not execute cleanup functions registered with @code{atexit} or
 @code{on_exit}.
@@ -1025,6 +1022,10 @@ does not execute cleanup functions registered with @code{atexit} or
 This function actually terminates the process by raising a
 @code{SIGABRT} signal, and your program can include a handler to
 intercept this signal; see @ref{Signal Handling}.
+
+If either the signal handler does not terminate the process, or if the
+signal is blocked, @code{abort} will reset the signal disposition to the
+default @code{SIG_DFL} action and raise the signal again.
 @end deftypefun
 
 @node Termination Internals

nptl/pthread_kill.c

@@ -69,6 +69,17 @@ __pthread_kill_implementation (pthread_t threadid, int signo, int no_tid)
   return ret;
 }
 
+/* Send the signal SIGNO to the caller.  Used by abort and called where the
+   signals are being already blocked and there is no need to synchronize with
+   exit_lock.  */
+int
+__pthread_raise_internal (int signo)
+{
+  /* Use the gettid syscall so it works after vfork.  */
+  int ret = INTERNAL_SYSCALL_CALL (tgkill, __getpid (), __gettid(), signo);
+  return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0;
+}
+
 int
 __pthread_kill_internal (pthread_t threadid, int signo)
 {

posix/fork.c

@@ -84,6 +84,8 @@ __libc_fork (void)
 
 	  fork_system_setup_after_fork ();
 
+	  call_function_static_weak (__abort_fork_reset_child);
+
 	  /* Release malloc locks.  */
 	  call_function_static_weak (__malloc_fork_unlock_child);
 

signal/sigaction.c

@@ -16,8 +16,9 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <errno.h>
-#include <signal.h>
 #include <internal-signals.h>
+#include <libc-lock.h>
+#include <signal.h>
 
 /* If ACT is not NULL, change the action for SIG to *ACT.
    If OACT is not NULL, put the old action for SIG in *OACT.  */
@@ -30,7 +31,17 @@ __sigaction (int sig, const struct sigaction *act, struct sigaction *oact)
       return -1;
     }
 
-  return __libc_sigaction (sig, act, oact);
+  internal_sigset_t set;
+
+  if (sig == SIGABRT)
+    __abort_lock_wrlock (&set);
+
+  int r = __libc_sigaction (sig, act, oact);
+
+  if (sig == SIGABRT)
+    __abort_lock_unlock (&set);
+
+  return r;
 }
 libc_hidden_def (__sigaction)
 weak_alias (__sigaction, sigaction)

stdlib/abort.c

@@ -15,13 +15,11 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <libc-lock.h>
 #include <signal.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
 #include <internal-signals.h>
+#include <libc-lock.h>
+#include <pthreadP.h>
+#include <unistd.h>
 
 /* Try to get a machine dependent instruction which will make the
    program crash.  This is used in case everything else fails.  */
@@ -35,89 +33,63 @@
 struct abort_msg_s *__abort_msg;
 libc_hidden_def (__abort_msg)
 
-/* We must avoid to run in circles.  Therefore we remember how far we
+/* The lock is used to prevent multiple thread to change the SIGABRT
-   already got.  */
+   to SIG_IGN while abort tries to change to SIG_DFL, and to avoid
-static int stage;
+   a new process to see a wrong disposition if there is a SIGABRT
+   handler installed.  */
+__libc_rwlock_define_initialized (static, lock);
 
-/* We should be prepared for multiple threads trying to run abort.  */
+void
-__libc_lock_define_initialized_recursive (static, lock);
+__abort_fork_reset_child (void)
+{
+  __libc_rwlock_init (lock);
+}
 
+void
+__abort_lock_rdlock (internal_sigset_t *set)
+{
+  internal_signal_block_all (set);
+  __libc_rwlock_rdlock (lock);
+}
+
+void
+__abort_lock_wrlock (internal_sigset_t *set)
+{
+  internal_signal_block_all (set);
+  __libc_rwlock_wrlock (lock);
+}
+
+void
+__abort_lock_unlock (const internal_sigset_t *set)
+{
+  __libc_rwlock_unlock (lock);
+  internal_signal_restore_set (set);
+}
 
 /* Cause an abnormal program termination with core-dump.  */
-void
+_Noreturn void
 abort (void)
 {
-  struct sigaction act;
-
-  /* First acquire the lock.  */
-  __libc_lock_lock_recursive (lock);
-
-  /* Now it's for sure we are alone.  But recursive calls are possible.  */
-
-  /* Unblock SIGABRT.  */
-  if (stage == 0)
-    {
-      ++stage;
-      internal_sigset_t sigs;
-      internal_sigemptyset (&sigs);
-      internal_sigaddset (&sigs, SIGABRT);
-      internal_sigprocmask (SIG_UNBLOCK, &sigs, NULL);
-    }
-
-  /* Send signal which possibly calls a user handler.  */
-  if (stage == 1)
-    {
-      /* This stage is special: we must allow repeated calls of
-	 `abort' when a user defined handler for SIGABRT is installed.
-	 This is risky since the `raise' implementation might also
-	 fail but I don't see another possibility.  */
-      int save_stage = stage;
-
-      stage = 0;
-      __libc_lock_unlock_recursive (lock);
-
   raise (SIGABRT);
 
-      __libc_lock_lock_recursive (lock);
+  /* There is a SIGABRT handle installed and it returned, or SIGABRT was
-      stage = save_stage + 1;
+     blocked or ignored.  In this case use a AS-safe lock to prevent sigaction
-    }
+     to change the signal disposition again, set the handle to default
+     disposition, and re-raise the signal.  Even if POSIX state this step is
+     optional, this a QoI by forcing the process termination through the
+     signal handler.  */
+  __abort_lock_wrlock (NULL);
 
-  /* There was a handler installed.  Now remove it.  */
+  struct sigaction act = {.sa_handler = SIG_DFL, .sa_flags = 0 };
-  if (stage == 2)
-    {
-      ++stage;
-      memset (&act, '\0', sizeof (struct sigaction));
-      act.sa_handler = SIG_DFL;
   __sigfillset (&act.sa_mask);
-      act.sa_flags = 0;
+  __libc_sigaction (SIGABRT, &act, NULL);
-      __sigaction (SIGABRT, &act, NULL);
+  __pthread_raise_internal (SIGABRT);
-    }
+  internal_signal_unblock_signal (SIGABRT);
 
-  /* Try again.  */
+  /* This code should be unreachable, try the arch-specific code and the
-  if (stage == 3)
+     syscall fallback.  */
-    {
-      ++stage;
-      raise (SIGABRT);
-    }
-
-  /* Now try to abort using the system specific command.  */
-  if (stage == 4)
-    {
-      ++stage;
   ABORT_INSTRUCTION;
-    }
 
-  /* If we can't signal ourselves and the abort instruction failed, exit.  */
-  if (stage == 5)
-    {
-      ++stage;
   _exit (127);
-    }
-
-  /* If even this fails try to use the provided instruction to crash
-     or otherwise make sure we never return.  */
-  while (1)
-    /* Try for ever and ever.  */
-    ABORT_INSTRUCTION;
 }
 libc_hidden_def (abort)

sysdeps/generic/internal-signals.h

@@ -20,6 +20,7 @@
 # define __INTERNAL_SIGNALS_H
 
 #include <signal.h>
+#include <internal-sigset.h>
 #include <sigsetops.h>
 #include <stdbool.h>
 #include <stddef.h>
@@ -39,10 +40,32 @@ clear_internal_signals (sigset_t *set)
 {
 }
 
-typedef sigset_t internal_sigset_t;
-
 #define internal_sigemptyset(__s)            __sigemptyset (__s)
+#define internal_sigfillset(__s)             __sigfillset (__s)
 #define internal_sigaddset(__s, __i)         __sigaddset (__s, __i)
 #define internal_sigprocmask(__h, __s, __o)  __sigprocmask (__h, __s, __o)
 
+static inline void
+internal_signal_block_all (internal_sigset_t *oset)
+{
+  internal_sigset_t set;
+  internal_sigfillset (&set);
+  internal_sigprocmask (SIG_BLOCK, &set, oset);
+}
+
+static inline void
+internal_signal_restore_set (const internal_sigset_t *set)
+{
+  internal_sigprocmask (SIG_SETMASK, set, NULL);
+}
+
+static inline void
+internal_signal_unblock_signal (int sig)
+{
+  internal_sigset_t set;
+  internal_sigemptyset (&set);
+  internal_sigaddset (&set, sig);
+  internal_sigprocmask (SIG_UNBLOCK, &set, NULL);
+}
+
 #endif /* __INTERNAL_SIGNALS_H  */

sysdeps/generic/internal-sigset.h

@@ -0,0 +1,26 @@
+/* Internal sigset_t definition.
+   Copyright (C) 2022-2023 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
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _INTERNAL_SIGSET_H
+#define _INTERNAL_SIGSET_H
+
+#include <signal.h>
+
+typedef sigset_t internal_sigset_t;
+
+#endif

sysdeps/htl/pthreadP.h

@@ -92,6 +92,8 @@ int __pthread_attr_setstack (pthread_attr_t *__attr, void *__stackaddr,
 int __pthread_attr_getstack (const pthread_attr_t *, void **, size_t *);
 void __pthread_testcancel (void);
 
+#define __pthread_raise_internal(__sig) raise (__sig)
+
 libc_hidden_proto (__pthread_self)
 
 #if IS_IN (libpthread)

sysdeps/nptl/_Fork.c

@@ -17,15 +17,17 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <arch-fork.h>
+#include <libc-lock.h>
 #include <pthreadP.h>
 
 pid_t
 _Fork (void)
 {
   /* Block all signals to avoid revealing the inconsistent TCB state
-     to a signal handler after fork.  */
+     to a signal handler after fork.  The abort lock should AS-safe
+     to avoid deadlock if _Fork is called from a signal handler.  */
   internal_sigset_t original_sigmask;
-  internal_signal_block_all (&original_sigmask);
+  __abort_lock_rdlock (&original_sigmask);
 
   pid_t pid = arch_fork (&THREAD_SELF->tid);
   if (pid == 0)
@@ -50,7 +52,7 @@ _Fork (void)
 			     sizeof (struct robust_list_head));
     }
 
-  internal_signal_restore_set (&original_sigmask);
+  __abort_lock_unlock (&original_sigmask);
   return pid;
 }
 libc_hidden_def (_Fork)

sysdeps/nptl/pthreadP.h

@@ -517,6 +517,7 @@ libc_hidden_proto (__pthread_kill)
 extern int __pthread_cancel (pthread_t th);
 extern int __pthread_kill_internal (pthread_t threadid, int signo)
   attribute_hidden;
+extern int __pthread_raise_internal (int signo) attribute_hidden;
 extern void __pthread_exit (void *value) __attribute__ ((__noreturn__));
 libc_hidden_proto (__pthread_exit)
 extern int __pthread_join (pthread_t threadid, void **thread_return);

sysdeps/posix/profil.c

@@ -82,7 +82,7 @@ __profil (u_short *sample_buffer, size_t size, size_t offset, u_int scale)
       if (__setitimer (ITIMER_PROF, &otimer, NULL) < 0)
 	return -1;
       samples = NULL;
-      return __sigaction (SIGPROF, &oact, NULL);
+      return __libc_sigaction (SIGPROF, &oact, NULL);
     }
 
  if (samples)
@@ -90,7 +90,7 @@ __profil (u_short *sample_buffer, size_t size, size_t offset, u_int scale)
       /* Was already turned on.  Restore old timer and signal handler
 	 first.  */
       if (__setitimer (ITIMER_PROF, &otimer, NULL) < 0
-	  || __sigaction (SIGPROF, &oact, NULL) < 0)
+	  || __libc_sigaction (SIGPROF, &oact, NULL) < 0)
 	return -1;
     }
 #else
@@ -114,7 +114,7 @@ __profil (u_short *sample_buffer, size_t size, size_t offset, u_int scale)
 #endif
   act.sa_flags |= SA_RESTART;
   __sigfillset (&act.sa_mask);
-  if (__sigaction (SIGPROF, &act, oact_ptr) < 0)
+  if (__libc_sigaction (SIGPROF, &act, oact_ptr) < 0)
     return -1;
 
   timer.it_value.tv_sec = 0;

sysdeps/unix/sysv/linux/internal-signals.h

@@ -90,6 +90,15 @@ internal_signal_restore_set (const internal_sigset_t *set)
 			 __NSIG_BYTES);
 }
 
+static inline void
+internal_signal_unblock_signal (int sig)
+{
+  internal_sigset_t set;
+  internal_sigemptyset (&set);
+  internal_sigaddset (&set, sig);
+  INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &set, NULL,
+			 __NSIG_BYTES);
+}
 
 /* It is used on timer_create code directly on sigwaitinfo call, so it can not
    use the internal_sigset_t definitions.  */

sysdeps/unix/sysv/linux/internal-sigset.h

@@ -21,7 +21,7 @@
 
 #include <sigsetops.h>
 
-typedef struct
+typedef struct _internal_sigset_t
 {
   unsigned long int __val[__NSIG_WORDS];
 } internal_sigset_t;

sysdeps/unix/sysv/linux/spawni.c

@@ -383,7 +383,11 @@ __spawnix (int *pid, const char *file,
   args.pidfd = 0;
   args.xflags = xflags;
 
-  internal_signal_block_all (&args.oldmask);
+  /* Avoid the potential issues if caller sets a SIG_IGN for SIGABRT, calls
+     abort, and another thread issues posix_spawn just after the sigaction
+     returns.  With default options (not setting POSIX_SPAWN_SETSIGDEF), the
+     process can still see SIG_DFL for SIGABRT, where it should be SIG_IGN.  */
+  __abort_lock_rdlock (&args.oldmask);
 
   /* The clone flags used will create a new child that will run in the same
      memory space (CLONE_VM) and the execution of calling thread will be
@@ -474,7 +478,7 @@ __spawnix (int *pid, const char *file,
   if ((ec == 0) && (pid != NULL))
     *pid = use_pidfd ? args.pidfd : new_pid;
 
-  internal_signal_restore_set (&args.oldmask);
+  __abort_lock_unlock (&args.oldmask);
 
   __pthread_setcancelstate (state, NULL);