[PATCH 0/2] Make abort AS-safe

[PATCH 0/2] Make abort AS-safe

[Adhemerval Zanella]

Besides POSIX stating abort should be AS-safe, Rust also had an
open PR about it [1] (it was closed with a different fix). 

The main issue is the recursive lock used on abort does not
synchronize with 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 raising SIGABRT might create
a new process with a non-expected signal mask.

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

The clone is also subjected to this issue, but since glibc does not
do any internal metadata setup (as for fork-like function), this patch
does not handle it for the symbol.

I have not added a regression tests because, from previous Carlos's
patch [2], hitting the code path to trigger the potential issue
(fork just after abort has acquired the lock and reset SIGABRT handler)
is not deterministic and it would generate a lot of development
overhead.

[1] https://github.com/rust-lang/rust/issues/73894#issuecomment-673478761
[2] https://sourceware.org/pipermail/libc-alpha/2020-September/117934.html

Adhemerval Zanella (2):
  setjmp: Use BSD sematic as default for setjmp
  stdlib: Make abort AS-safe (BZ 26275)

 include/stdlib.h                           |   4 +
 manual/setjmp.texi                         |  14 +--
 manual/startup.texi                        |   3 -
 nptl/pthread_create.c                      |   3 +-
 nptl/pthread_kill.c                        |  11 ++
 posix/fork.c                               |   2 +
 setjmp/setjmp.h                            |   5 -
 signal/sigaction.c                         |  21 +++-
 stdlib/abort.c                             | 128 ++++++++-------------
 sysdeps/generic/internal-signals.h         |  24 ++++
 sysdeps/htl/pthreadP.h                     |   2 +
 sysdeps/nptl/_Fork.c                       |  12 ++
 sysdeps/nptl/libc_start_call_main.h        |   3 +-
 sysdeps/nptl/pthreadP.h                    |   1 +
 sysdeps/unix/sysv/linux/internal-signals.h |   9 ++
 sysdeps/unix/sysv/linux/spawni.c           |   3 +
 16 files changed, 140 insertions(+), 105 deletions(-)

-- 
2.34.1

[PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Adhemerval Zanella]

POSIX relaxed the relation of setjmp/longjmp and the signal mask
save/restore, meaning that setjmp does not require to be routed to
_setjmp to be standard compliant.

This is done to avoid breakage of SIGABRT handlers, since to fully
make abort AS-safe, it is required to remove the recurisve lock
used to unblock SIGABRT prior raised the signal.

Also, it allows caller to actually use setjmp, since from
7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally
routed to _setjmp.

Checked on x86_64-linux-gnu.
---
 manual/setjmp.texi                  | 14 ++++----------
 nptl/pthread_create.c               |  3 ++-
 setjmp/setjmp.h                     |  5 -----
 sysdeps/nptl/libc_start_call_main.h |  3 ++-
 4 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/manual/setjmp.texi b/manual/setjmp.texi
index 7092a0dde2..f2d82a2f33 100644
--- a/manual/setjmp.texi
+++ b/manual/setjmp.texi
@@ -189,16 +189,10 @@ them @code{volatile}.
 @section Non-Local Exits and Signals
 
 In BSD Unix systems, @code{setjmp} and @code{longjmp} also save and
-restore the set of blocked signals; see @ref{Blocking Signals}.  However,
-the POSIX.1 standard requires @code{setjmp} and @code{longjmp} not to
-change the set of blocked signals, and provides an additional pair of
-functions (@code{sigsetjmp} and @code{siglongjmp}) to get the BSD
-behavior.
-
-The behavior of @code{setjmp} and @code{longjmp} in @theglibc{} is
-controlled by feature test macros; see @ref{Feature Test Macros}.  The
-default in @theglibc{} is the POSIX.1 behavior rather than the BSD
-behavior.
+restore the set of blocked signals; see @ref{Blocking Signals}, while
+on @w{System V} they will not.  POSIX does not specify the relation of
+@code{setjmp} and @code{longjmp} to signal mask.  The default in
+@theglibc{} is the BSD behavior.
 
 The facilities in this section are declared in the header file
 @file{setjmp.h}.
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 1ac8862ed2..3d7dfac198 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -399,7 +399,8 @@ start_thread (void *arg)
      the saved signal mask), so that is a false positive.  */
   DIAG_IGNORE_NEEDS_COMMENT (11, "-Wstringop-overflow=");
 #endif
-  not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf);
+  not_first_call = __sigsetjmp (
+    (struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf, 0);
   DIAG_POP_NEEDS_COMMENT;
 
   /* No previous handlers.  NB: This must be done after setjmp since the
diff --git a/setjmp/setjmp.h b/setjmp/setjmp.h
index 3cdc2dfcfb..53edbba92b 100644
--- a/setjmp/setjmp.h
+++ b/setjmp/setjmp.h
@@ -44,11 +44,6 @@ extern int __sigsetjmp (struct __jmp_buf_tag __env[1], int __savemask) __THROWNL
    Return 0.  */
 extern int _setjmp (struct __jmp_buf_tag __env[1]) __THROWNL;
 
-/* Do not save the signal mask.  This is equivalent to the `_setjmp'
-   BSD function.  */
-#define setjmp(env)	_setjmp (env)
-
-
 /* Jump to the environment saved in ENV, making the
    `setjmp' call there return VAL, or 1 if VAL is 0.  */
 extern void longjmp (struct __jmp_buf_tag __env[1], int __val)
diff --git a/sysdeps/nptl/libc_start_call_main.h b/sysdeps/nptl/libc_start_call_main.h
index a55e3df013..984e859550 100644
--- a/sysdeps/nptl/libc_start_call_main.h
+++ b/sysdeps/nptl/libc_start_call_main.h
@@ -41,7 +41,8 @@ __libc_start_call_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
      the saved signal mask), so that is a false positive.  */
   DIAG_IGNORE_NEEDS_COMMENT (11, "-Wstringop-overflow=");
 #endif
-  not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf);
+  not_first_call = __sigsetjmp (
+     (struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf, 0);
   DIAG_POP_NEEDS_COMMENT;
   if (__glibc_likely (! not_first_call))
     {
-- 
2.34.1

[PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Adhemerval Zanella]

The recursive lock used on abort does not synchronize with 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 raise might create
programs with a non-expected signal mask.

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

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

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 include/stdlib.h                           |   4 +
 manual/startup.texi                        |   3 -
 nptl/pthread_kill.c                        |  11 ++
 posix/fork.c                               |   2 +
 signal/sigaction.c                         |  21 +++-
 stdlib/abort.c                             | 128 ++++++++-------------
 sysdeps/generic/internal-signals.h         |  24 ++++
 sysdeps/htl/pthreadP.h                     |   2 +
 sysdeps/nptl/_Fork.c                       |  12 ++
 sysdeps/nptl/pthreadP.h                    |   1 +
 sysdeps/unix/sysv/linux/internal-signals.h |   9 ++
 sysdeps/unix/sysv/linux/spawni.c           |   3 +
 12 files changed, 132 insertions(+), 88 deletions(-)

diff --git a/include/stdlib.h b/include/stdlib.h
index 7deb8193d7..b87a37e7e2 100644
--- a/include/stdlib.h
+++ b/include/stdlib.h
@@ -75,6 +75,10 @@ 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_lock (void) attribute_hidden;
+extern void __abort_lock_unlock (void) attribute_hidden;
+
 libc_hidden_proto (exit)
 libc_hidden_proto (abort)
 libc_hidden_proto (getenv)
diff --git a/manual/startup.texi b/manual/startup.texi
index 9bf24123f5..236a480a74 100644
--- a/manual/startup.texi
+++ b/manual/startup.texi
@@ -993,9 +993,6 @@ for this function is in @file{stdlib.h}.
 @deftypefun void abort (void)
 @standards{ISO, stdlib.h}
 @safety{@prelim{}@mtsafe{}@asunsafe{@asucorrupt{}}@acunsafe{@aculock{} @acucorrupt{}}}
-@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}.
diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
index 44e45a4e23..e3364fb5d1 100644
--- a/nptl/pthread_kill.c
+++ b/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)
+{
+  struct pthread *pd = THREAD_SELF;
+  int ret = INTERNAL_SYSCALL_CALL (tgkill, __getpid (), pd->tid, signo);
+  return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0;
+}
+
 int
 __pthread_kill_internal (pthread_t threadid, int signo)
 {
diff --git a/posix/fork.c b/posix/fork.c
index b4aaa9fa6d..1640d0750c 100644
--- a/posix/fork.c
+++ b/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);
 
diff --git a/signal/sigaction.c b/signal/sigaction.c
index 3a49597c61..0d1358d720 100644
--- a/signal/sigaction.c
+++ b/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,23 @@ __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)
+    {
+      internal_signal_block_all (&set);
+      __abort_lock_lock ();
+    }
+
+  int r = __libc_sigaction (sig, act, oact);
+
+  if (sig == SIGABRT)
+    {
+      __abort_lock_unlock ();
+      internal_signal_restore_set (&set);
+    }
+
+  return r;
 }
 libc_hidden_def (__sigaction)
 weak_alias (__sigaction, sigaction)
diff --git a/stdlib/abort.c b/stdlib/abort.c
index 16a453459c..e19ad730cd 100644
--- a/stdlib/abort.c
+++ b/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,53 @@
 struct abort_msg_s *__abort_msg;
 libc_hidden_def (__abort_msg)
 
-/* We must avoid to run in circles.  Therefore we remember how far we
-   already got.  */
-static int stage;
+/* The lock is used to prevent multiple thread to change the SIGABRT
+   to SIG_IGN while abort tries to change to SIG_DFL, and to avoid
+   a new process to see a wrong disposition if there is a SIGABRT
+   handler installed.  */
+__libc_lock_define_initialized (static, lock);
 
-/* We should be prepared for multiple threads trying to run abort.  */
-__libc_lock_define_initialized_recursive (static, lock);
-
-
-/* Cause an abnormal program termination with core-dump.  */
 void
-abort (void)
+__abort_fork_reset_child (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);
-      stage = save_stage + 1;
-    }
-
-  /* There was a handler installed.  Now remove it.  */
-  if (stage == 2)
-    {
-      ++stage;
-      memset (&act, '\0', sizeof (struct sigaction));
-      act.sa_handler = SIG_DFL;
-      __sigfillset (&act.sa_mask);
-      act.sa_flags = 0;
-      __sigaction (SIGABRT, &act, NULL);
-    }
-
-  /* Try again.  */
-  if (stage == 3)
-    {
-      ++stage;
-      raise (SIGABRT);
-    }
+  __libc_lock_init (lock);
+}
 
-  /* Now try to abort using the system specific command.  */
-  if (stage == 4)
-    {
-      ++stage;
-      ABORT_INSTRUCTION;
-    }
+void
+__abort_lock_lock (void)
+{
+  __libc_lock_lock (lock);
+}
 
-  /* If we can't signal ourselves and the abort instruction failed, exit.  */
-  if (stage == 5)
-    {
-      ++stage;
-      _exit (127);
-    }
+void
+__abort_lock_unlock (void)
+{
+  __libc_lock_unlock (lock);
+}
 
-  /* 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;
+/* Cause an abnormal program termination with core-dump.  */
+_Noreturn void
+abort (void)
+{
+  raise (SIGABRT);
+
+  /* There is a SIGABRT handler installed and it returned, or SIGABRT was
+     blocked or ignored.  In this case use a AS-safe lock to prevent sigaction
+     to change the signal disposition, reinstall the handle to abort the
+     process, and raise the signal again.  */
+  internal_signal_block_all (NULL);
+  __libc_lock_lock (lock);
+
+  struct sigaction act = {.sa_handler = SIG_DFL, .sa_flags = 0 };
+  __sigfillset (&act.sa_mask);
+  __libc_sigaction (SIGABRT, &act, NULL);
+  __pthread_raise_internal (SIGABRT);
+  internal_signal_unblock_signal (SIGABRT);
+
+  /* This code should be unreachable, try the arch-specific code and the
+     syscall fallback.  */
+  ABORT_INSTRUCTION;
+
+  _exit (127);
 }
 libc_hidden_def (abort)
diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h
index e2e9f9fd49..1c0f7b2e6c 100644
--- a/sysdeps/generic/internal-signals.h
+++ b/sysdeps/generic/internal-signals.h
@@ -42,7 +42,31 @@ 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  */
diff --git a/sysdeps/htl/pthreadP.h b/sysdeps/htl/pthreadP.h
index 3f052f0e53..7410261d1a 100644
--- a/sysdeps/htl/pthreadP.h
+++ b/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)
diff --git a/sysdeps/nptl/_Fork.c b/sysdeps/nptl/_Fork.c
index f8322ae557..f6da952ce0 100644
--- a/sysdeps/nptl/_Fork.c
+++ b/sysdeps/nptl/_Fork.c
@@ -17,11 +17,19 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <arch-fork.h>
+#include <libc-lock.h>
 #include <pthreadP.h>
 
 pid_t
 _Fork (void)
 {
+  /* The lock acquisition needs to be AS-safe to avoid deadlock if _Fork is
+     called from the signal handler that has interrupted fork itself.  */
+  internal_sigset_t set;
+
+  internal_signal_block_all (&set);
+  __abort_lock_lock ();
+
   pid_t pid = arch_fork (&THREAD_SELF->tid);
   if (pid == 0)
     {
@@ -44,6 +52,10 @@ _Fork (void)
       INTERNAL_SYSCALL_CALL (set_robust_list, &self->robust_head,
 			     sizeof (struct robust_list_head));
     }
+
+  __abort_lock_unlock ();
+  internal_signal_restore_set (&set);
+
   return pid;
 }
 libc_hidden_def (_Fork)
diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
index 54f9198681..6cc675c5a5 100644
--- a/sysdeps/nptl/pthreadP.h
+++ b/sysdeps/nptl/pthreadP.h
@@ -508,6 +508,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);
diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
index 43c3c0b4a0..c58f568dda 100644
--- a/sysdeps/unix/sysv/linux/internal-signals.h
+++ b/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.  */
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index ec687cb423..3ef07ec633 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -371,6 +371,7 @@ __spawnix (pid_t * pid, const char *file,
   args.xflags = xflags;
 
   internal_signal_block_all (&args.oldmask);
+  __abort_lock_lock ();
 
   /* 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
@@ -402,6 +403,8 @@ __spawnix (pid_t * pid, const char *file,
 					   &args);
     }
 
+  __abort_lock_unlock ();
+
   /* It needs to collect the case where the auxiliary process was created
      but failed to execute the file (due either any preparation step or
      for execve itself).  */
-- 
2.34.1

Re: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h
> index e2e9f9fd49..1c0f7b2e6c 100644
> --- a/sysdeps/generic/internal-signals.h
> +++ b/sysdeps/generic/internal-signals.h
> @@ -42,7 +42,31 @@ 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  */

This should probably go into a separate patch.  I recall writing a
similar patch, can't find it right now.

Thanks,
Florian

Re: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> +void
> +__abort_lock_lock (void)
> +{
> +  __libc_lock_lock (lock);
> +}

I think __abort_lock_lock and __abort_lock_unlock should take a signal
mask argument, so that they are more like a async-signal-safe mutex.

> +/* Cause an abnormal program termination with core-dump.  */
> +_Noreturn void
> +abort (void)
> +{
> +  raise (SIGABRT);
> +
> +  /* There is a SIGABRT handler installed and it returned, or SIGABRT was
> +     blocked or ignored.  In this case use a AS-safe lock to prevent sigaction
> +     to change the signal disposition, reinstall the handle to abort the
> +     process, and raise the signal again.  */
> +  internal_signal_block_all (NULL);
> +  __libc_lock_lock (lock);

This could call __abort_lock_lock.

Thanks,
Florian

Re: [PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> POSIX relaxed the relation of setjmp/longjmp and the signal mask
> save/restore, meaning that setjmp does not require to be routed to
> _setjmp to be standard compliant.
>
> This is done to avoid breakage of SIGABRT handlers, since to fully
> make abort AS-safe, it is required to remove the recurisve lock
> used to unblock SIGABRT prior raised the signal.
>
> Also, it allows caller to actually use setjmp, since from
> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally
> routed to _setjmp.

Doesn't this have non-trivial performance impact?

Thanks,
Florian

Re: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Cristian Rodríguez]

[-- Attachment #1: Type: text/plain, Size: 328 bytes --]

On Mon, Jul 31, 2023 at 1:19 PM Adhemerval Zanella via Libc-alpha <
libc-alpha@sourceware.org> wrote:

>
> ABORT_INSTRUCTION after _exit (if the syscall does not terminate the
> process, the system is really broken).
>

Yes, at least the linux kernel ultimately BUGs out if it cannot exit. the
whole system crashes.

Re: [PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Adhemerval Zanella Netto]

On 01/08/23 05:35, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> POSIX relaxed the relation of setjmp/longjmp and the signal mask
>> save/restore, meaning that setjmp does not require to be routed to
>> _setjmp to be standard compliant.
>>
>> This is done to avoid breakage of SIGABRT handlers, since to fully
>> make abort AS-safe, it is required to remove the recurisve lock
>> used to unblock SIGABRT prior raised the signal.
>>
>> Also, it allows caller to actually use setjmp, since from
>> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally
>> routed to _setjmp.
> 
> Doesn't this have non-trivial performance impact?

Yes, it is two extra sigprocmask to get/set the signal mask.  This is not 
*strictly* required, but the SIGABRT on abort generates racy conditions on
process creation and.  This patch can be dropped, but it would mean that
to get expected semantic for abort handlers will need to use sigsetjmp (..., 1)
instead of setjmp. 

Re: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Adhemerval Zanella Netto]

On 01/08/23 05:10, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h
>> index e2e9f9fd49..1c0f7b2e6c 100644
>> --- a/sysdeps/generic/internal-signals.h
>> +++ b/sysdeps/generic/internal-signals.h
>> @@ -42,7 +42,31 @@ 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  */
> 
> This should probably go into a separate patch.  I recall writing a
> similar patch, can't find it right now.

These function are not really used and only required for Hurd for the abort 
change.  I think it is more logical to add them when they are actually used
instead of a separated patch.

Re: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Adhemerval Zanella Netto]

On 01/08/23 05:26, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> +void
>> +__abort_lock_lock (void)
>> +{
>> +  __libc_lock_lock (lock);
>> +}
> 
> I think __abort_lock_lock and __abort_lock_unlock should take a signal
> mask argument, so that they are more like a async-signal-safe mutex.

That was my first approach, but I changed it due __spawnix usage.  The
lock is really only required on clone call, so we can release it after
clone returns.  However we need to only umask signals after the execve
call in helper process.

We can move the abort unlock later, it should be ok as well (it would
add a slight more latency on highly multithead programs that spawns
a lot of thread and try to abort(), but it should be ok). 

> 
>> +/* Cause an abnormal program termination with core-dump.  */
>> +_Noreturn void
>> +abort (void)
>> +{
>> +  raise (SIGABRT);
>> +
>> +  /* There is a SIGABRT handler installed and it returned, or SIGABRT was
>> +     blocked or ignored.  In this case use a AS-safe lock to prevent sigaction
>> +     to change the signal disposition, reinstall the handle to abort the
>> +     process, and raise the signal again.  */
>> +  internal_signal_block_all (NULL);
>> +  __libc_lock_lock (lock);
> 
> This could call __abort_lock_lock.

Ack.

Re: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> +  /* There is a SIGABRT handler installed and it returned, or SIGABRT was
> +     blocked or ignored.  In this case use a AS-safe lock to prevent sigaction
> +     to change the signal disposition, reinstall the handle to abort the

typo: handle[r]

Maybe mention here that the handler cannot change again because
sigaction blocks on the lock?

I also don't quite understand why we need to take the abort lock in
posix_spawn.  There isn't any user code that can run in the same address
space after the vfork.

Thanks,
Florian

Re: [PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Florian Weimer]

* Adhemerval Zanella Netto:

> On 01/08/23 05:35, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> POSIX relaxed the relation of setjmp/longjmp and the signal mask
>>> save/restore, meaning that setjmp does not require to be routed to
>>> _setjmp to be standard compliant.
>>>
>>> This is done to avoid breakage of SIGABRT handlers, since to fully
>>> make abort AS-safe, it is required to remove the recurisve lock
>>> used to unblock SIGABRT prior raised the signal.
>>>
>>> Also, it allows caller to actually use setjmp, since from
>>> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally
>>> routed to _setjmp.
>> 
>> Doesn't this have non-trivial performance impact?
>
> Yes, it is two extra sigprocmask to get/set the signal mask.  This is
> not *strictly* required, but the SIGABRT on abort generates racy
> conditions on process creation and.  This patch can be dropped, but it
> would mean that to get expected semantic for abort handlers will need
> to use sigsetjmp (..., 1) instead of setjmp.

Sorry, I don't understand?  With the current locking, this change should
really not be required because the user SIGABRT handler does not run
with the signal mask changed.

Thanks,
Florian

Re: [PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Adhemerval Zanella Netto]

On 02/08/23 04:59, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 01/08/23 05:35, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> POSIX relaxed the relation of setjmp/longjmp and the signal mask
>>>> save/restore, meaning that setjmp does not require to be routed to
>>>> _setjmp to be standard compliant.
>>>>
>>>> This is done to avoid breakage of SIGABRT handlers, since to fully
>>>> make abort AS-safe, it is required to remove the recurisve lock
>>>> used to unblock SIGABRT prior raised the signal.
>>>>
>>>> Also, it allows caller to actually use setjmp, since from
>>>> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally
>>>> routed to _setjmp.
>>>
>>> Doesn't this have non-trivial performance impact?
>>
>> Yes, it is two extra sigprocmask to get/set the signal mask.  This is
>> not *strictly* required, but the SIGABRT on abort generates racy
>> conditions on process creation and.  This patch can be dropped, but it
>> would mean that to get expected semantic for abort handlers will need
>> to use sigsetjmp (..., 1) instead of setjmp.
> 
> Sorry, I don't understand?  With the current locking, this change should
> really not be required because the user SIGABRT handler does not run
> with the signal mask changed.
>

This change is only required to keep the same semantic of setjmp/longjmp
regarding SIGABRT handler, where current code keeps subsequent SIGABRT 
installed with default flags to not keep the signal masked.  Otherwise,
users that callers that handle SIGABRT will need to either a different
sigaction mask that do no change the blocked signals while handling
the signal, call sigprocmask after SIGABRT returns from longjmp, or
use sigsetjmp.

Re: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
> index 44e45a4e23..e3364fb5d1 100644
> --- a/nptl/pthread_kill.c
> +++ b/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)
> +{
> +  struct pthread *pd = THREAD_SELF;
> +  int ret = INTERNAL_SYSCALL_CALL (tgkill, __getpid (), pd->tid, signo);
> +  return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0;
> +}

This needs to use gettid (the system call) so that it works after vfork,
which may have an incorrect pd->tid.

There should be a comment to this effect in pthread_kill implementation
already.

Thanks,
Florian

Re: [PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Florian Weimer]

* Adhemerval Zanella Netto:

> On 02/08/23 04:59, Florian Weimer wrote:
>> * Adhemerval Zanella Netto:
>> 
>>> On 01/08/23 05:35, Florian Weimer wrote:
>>>> * Adhemerval Zanella via Libc-alpha:
>>>>
>>>>> POSIX relaxed the relation of setjmp/longjmp and the signal mask
>>>>> save/restore, meaning that setjmp does not require to be routed to
>>>>> _setjmp to be standard compliant.
>>>>>
>>>>> This is done to avoid breakage of SIGABRT handlers, since to fully
>>>>> make abort AS-safe, it is required to remove the recurisve lock
>>>>> used to unblock SIGABRT prior raised the signal.
>>>>>
>>>>> Also, it allows caller to actually use setjmp, since from
>>>>> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally
>>>>> routed to _setjmp.
>>>>
>>>> Doesn't this have non-trivial performance impact?
>>>
>>> Yes, it is two extra sigprocmask to get/set the signal mask.  This is
>>> not *strictly* required, but the SIGABRT on abort generates racy
>>> conditions on process creation and.  This patch can be dropped, but it
>>> would mean that to get expected semantic for abort handlers will need
>>> to use sigsetjmp (..., 1) instead of setjmp.
>> 
>> Sorry, I don't understand?  With the current locking, this change should
>> really not be required because the user SIGABRT handler does not run
>> with the signal mask changed.
>>
>
> This change is only required to keep the same semantic of setjmp/longjmp
> regarding SIGABRT handler, where current code keeps subsequent SIGABRT 
> installed with default flags to not keep the signal masked.  Otherwise,
> users that callers that handle SIGABRT will need to either a different
> sigaction mask that do no change the blocked signals while handling
> the signal, call sigprocmask after SIGABRT returns from longjmp, or
> use sigsetjmp.

Sorry, I still don't see it.  The new code switches the handler to
SIG_DFL atomically and blocks further sigaction calls.  This extends to
subprocesses because creating them is inhibited, too.  I think this
means that the difference in signal handler masking is not observable.

Thanks,
Florian

Re: [PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Adhemerval Zanella Netto]

On 02/08/23 09:42, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 02/08/23 04:59, Florian Weimer wrote:
>>> * Adhemerval Zanella Netto:
>>>
>>>> On 01/08/23 05:35, Florian Weimer wrote:
>>>>> * Adhemerval Zanella via Libc-alpha:
>>>>>
>>>>>> POSIX relaxed the relation of setjmp/longjmp and the signal mask
>>>>>> save/restore, meaning that setjmp does not require to be routed to
>>>>>> _setjmp to be standard compliant.
>>>>>>
>>>>>> This is done to avoid breakage of SIGABRT handlers, since to fully
>>>>>> make abort AS-safe, it is required to remove the recurisve lock
>>>>>> used to unblock SIGABRT prior raised the signal.
>>>>>>
>>>>>> Also, it allows caller to actually use setjmp, since from
>>>>>> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally
>>>>>> routed to _setjmp.
>>>>>
>>>>> Doesn't this have non-trivial performance impact?
>>>>
>>>> Yes, it is two extra sigprocmask to get/set the signal mask.  This is
>>>> not *strictly* required, but the SIGABRT on abort generates racy
>>>> conditions on process creation and.  This patch can be dropped, but it
>>>> would mean that to get expected semantic for abort handlers will need
>>>> to use sigsetjmp (..., 1) instead of setjmp.
>>>
>>> Sorry, I don't understand?  With the current locking, this change should
>>> really not be required because the user SIGABRT handler does not run
>>> with the signal mask changed.
>>>
>>
>> This change is only required to keep the same semantic of setjmp/longjmp
>> regarding SIGABRT handler, where current code keeps subsequent SIGABRT 
>> installed with default flags to not keep the signal masked.  Otherwise,
>> users that callers that handle SIGABRT will need to either a different
>> sigaction mask that do no change the blocked signals while handling
>> the signal, call sigprocmask after SIGABRT returns from longjmp, or
>> use sigsetjmp.
> 
> Sorry, I still don't see it.  The new code switches the handler to
> SIG_DFL atomically and blocks further sigaction calls.  This extends to
> subprocesses because creating them is inhibited, too.  I think this
> means that the difference in signal handler masking is not observable.

But this change it no to handle if raise returns, but rather if you have
a SIGABRT handler that does not (like the fortify tests) installed with
default flags.  In this case, the kernel will add SIGABRT on the masked
signal, longjmp will return to setjmp with the SIGBRT handler set mask,
and the next SIGABRT won't trigger the handler

Re: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Adhemerval Zanella Netto]

On 02/08/23 04:57, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> +  /* There is a SIGABRT handler installed and it returned, or SIGABRT was
>> +     blocked or ignored.  In this case use a AS-safe lock to prevent sigaction
>> +     to change the signal disposition, reinstall the handle to abort the
> 
> typo: handle[r]
> 
> Maybe mention here that the handler cannot change again because
> sigaction blocks on the lock?

Ack.

> 
> I also don't quite understand why we need to take the abort lock in
> posix_spawn.  There isn't any user code that can run in the same address
> space after the vfork.

My understanding is the potential issues is 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.

Re: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Adhemerval Zanella Netto]

On 02/08/23 09:38, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c
>> index 44e45a4e23..e3364fb5d1 100644
>> --- a/nptl/pthread_kill.c
>> +++ b/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)
>> +{
>> +  struct pthread *pd = THREAD_SELF;
>> +  int ret = INTERNAL_SYSCALL_CALL (tgkill, __getpid (), pd->tid, signo);
>> +  return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0;
>> +}
> 
> This needs to use gettid (the system call) so that it works after vfork,
> which may have an incorrect pd->tid.
> 
> There should be a comment to this effect in pthread_kill implementation
> already.
> 

Ack.

Re: [PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Florian Weimer]

* Adhemerval Zanella Netto:

> On 02/08/23 09:42, Florian Weimer wrote:
>> * Adhemerval Zanella Netto:
>> 
>>> On 02/08/23 04:59, Florian Weimer wrote:
>>>> * Adhemerval Zanella Netto:
>>>>
>>>>> On 01/08/23 05:35, Florian Weimer wrote:
>>>>>> * Adhemerval Zanella via Libc-alpha:
>>>>>>
>>>>>>> POSIX relaxed the relation of setjmp/longjmp and the signal mask
>>>>>>> save/restore, meaning that setjmp does not require to be routed to
>>>>>>> _setjmp to be standard compliant.
>>>>>>>
>>>>>>> This is done to avoid breakage of SIGABRT handlers, since to fully
>>>>>>> make abort AS-safe, it is required to remove the recurisve lock
>>>>>>> used to unblock SIGABRT prior raised the signal.
>>>>>>>
>>>>>>> Also, it allows caller to actually use setjmp, since from
>>>>>>> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally
>>>>>>> routed to _setjmp.
>>>>>>
>>>>>> Doesn't this have non-trivial performance impact?
>>>>>
>>>>> Yes, it is two extra sigprocmask to get/set the signal mask.  This is
>>>>> not *strictly* required, but the SIGABRT on abort generates racy
>>>>> conditions on process creation and.  This patch can be dropped, but it
>>>>> would mean that to get expected semantic for abort handlers will need
>>>>> to use sigsetjmp (..., 1) instead of setjmp.
>>>>
>>>> Sorry, I don't understand?  With the current locking, this change should
>>>> really not be required because the user SIGABRT handler does not run
>>>> with the signal mask changed.
>>>>
>>>
>>> This change is only required to keep the same semantic of setjmp/longjmp
>>> regarding SIGABRT handler, where current code keeps subsequent SIGABRT 
>>> installed with default flags to not keep the signal masked.  Otherwise,
>>> users that callers that handle SIGABRT will need to either a different
>>> sigaction mask that do no change the blocked signals while handling
>>> the signal, call sigprocmask after SIGABRT returns from longjmp, or
>>> use sigsetjmp.
>> 
>> Sorry, I still don't see it.  The new code switches the handler to
>> SIG_DFL atomically and blocks further sigaction calls.  This extends to
>> subprocesses because creating them is inhibited, too.  I think this
>> means that the difference in signal handler masking is not observable.
>
> But this change it no to handle if raise returns, but rather if you have
> a SIGABRT handler that does not (like the fortify tests) installed with
> default flags.  In this case, the kernel will add SIGABRT on the masked
> signal, longjmp will return to setjmp with the SIGBRT handler set mask,
> and the next SIGABRT won't trigger the handler

Ahh, you mean because use removed the signal unblocking from abort?

If the signal is blocked, it is not delivered before it is unblocked.
This means that the handler will not observe it blocked.

But POSIX says this:

| The abort() function shall override blocking or ignoring the SIGABRT
| signal.

It also says:

| The SIGABRT signal shall be sent to the calling process as if by means
| of raise() with the argument SIGABRT.

Strictly speaking, it is impossible to comply with both requirements,
but I think the handler is expected to run even if SIGABRT is blocked.
As far as I understand it, the new code terminates the process in this
case, without ever running the handler.

Thanks,
Florian

Re: [PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Adhemerval Zanella Netto]

On 02/08/23 10:17, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 02/08/23 09:42, Florian Weimer wrote:
>>> * Adhemerval Zanella Netto:
>>>
>>>> On 02/08/23 04:59, Florian Weimer wrote:
>>>>> * Adhemerval Zanella Netto:
>>>>>
>>>>>> On 01/08/23 05:35, Florian Weimer wrote:
>>>>>>> * Adhemerval Zanella via Libc-alpha:
>>>>>>>
>>>>>>>> POSIX relaxed the relation of setjmp/longjmp and the signal mask
>>>>>>>> save/restore, meaning that setjmp does not require to be routed to
>>>>>>>> _setjmp to be standard compliant.
>>>>>>>>
>>>>>>>> This is done to avoid breakage of SIGABRT handlers, since to fully
>>>>>>>> make abort AS-safe, it is required to remove the recurisve lock
>>>>>>>> used to unblock SIGABRT prior raised the signal.
>>>>>>>>
>>>>>>>> Also, it allows caller to actually use setjmp, since from
>>>>>>>> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally
>>>>>>>> routed to _setjmp.
>>>>>>>
>>>>>>> Doesn't this have non-trivial performance impact?
>>>>>>
>>>>>> Yes, it is two extra sigprocmask to get/set the signal mask.  This is
>>>>>> not *strictly* required, but the SIGABRT on abort generates racy
>>>>>> conditions on process creation and.  This patch can be dropped, but it
>>>>>> would mean that to get expected semantic for abort handlers will need
>>>>>> to use sigsetjmp (..., 1) instead of setjmp.
>>>>>
>>>>> Sorry, I don't understand?  With the current locking, this change should
>>>>> really not be required because the user SIGABRT handler does not run
>>>>> with the signal mask changed.
>>>>>
>>>>
>>>> This change is only required to keep the same semantic of setjmp/longjmp
>>>> regarding SIGABRT handler, where current code keeps subsequent SIGABRT 
>>>> installed with default flags to not keep the signal masked.  Otherwise,
>>>> users that callers that handle SIGABRT will need to either a different
>>>> sigaction mask that do no change the blocked signals while handling
>>>> the signal, call sigprocmask after SIGABRT returns from longjmp, or
>>>> use sigsetjmp.
>>>
>>> Sorry, I still don't see it.  The new code switches the handler to
>>> SIG_DFL atomically and blocks further sigaction calls.  This extends to
>>> subprocesses because creating them is inhibited, too.  I think this
>>> means that the difference in signal handler masking is not observable.
>>
>> But this change it no to handle if raise returns, but rather if you have
>> a SIGABRT handler that does not (like the fortify tests) installed with
>> default flags.  In this case, the kernel will add SIGABRT on the masked
>> signal, longjmp will return to setjmp with the SIGBRT handler set mask,
>> and the next SIGABRT won't trigger the handler
> 
> Ahh, you mean because use removed the signal unblocking from abort?
> 
> If the signal is blocked, it is not delivered before it is unblocked.
> This means that the handler will not observe it blocked.
> 
> But POSIX says this:
> 
> | The abort() function shall override blocking or ignoring the SIGABRT
> | signal.
> 
> It also says:
> 
> | The SIGABRT signal shall be sent to the calling process as if by means
> | of raise() with the argument SIGABRT.
> 
> Strictly speaking, it is impossible to comply with both requirements,
> but I think the handler is expected to run even if SIGABRT is blocked.
> As far as I understand it, the new code terminates the process in this
> case, without ever running the handler.

The later has been changed with a new clarification [1]:

  The SIGABRT signal shall be sent to the calling [CX]thread[/CX] as if by
  means of raise() with the argument SIGABRT. [CX]If this signal does not 
  terminate the process (for example, if the signal is caught and the handler 
  returns), abort() may change the disposition of SIGABRT to SIG_DFL and send 
  the signal (in the same way) again. If a second signal is sent and it does 
  not terminate the process, the behavior is unspecified, except that the 
  abort() call shall not return.[/CX]

[1] https://austingroupbugs.net/view.php?id=906#c5851

Re: [PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Florian Weimer]

* Adhemerval Zanella Netto:

>> Ahh, you mean because use removed the signal unblocking from abort?
>> 
>> If the signal is blocked, it is not delivered before it is unblocked.
>> This means that the handler will not observe it blocked.
>> 
>> But POSIX says this:
>> 
>> | The abort() function shall override blocking or ignoring the SIGABRT
>> | signal.
>> 
>> It also says:
>> 
>> | The SIGABRT signal shall be sent to the calling process as if by means
>> | of raise() with the argument SIGABRT.
>> 
>> Strictly speaking, it is impossible to comply with both requirements,
>> but I think the handler is expected to run even if SIGABRT is blocked.
>> As far as I understand it, the new code terminates the process in this
>> case, without ever running the handler.
>
> The later has been changed with a new clarification [1]:
>
>   The SIGABRT signal shall be sent to the calling [CX]thread[/CX] as if by
>   means of raise() with the argument SIGABRT. [CX]If this signal does not 
>   terminate the process (for example, if the signal is caught and the handler 
>   returns), abort() may change the disposition of SIGABRT to SIG_DFL and send 
>   the signal (in the same way) again. If a second signal is sent and it does 
>   not terminate the process, the behavior is unspecified, except that the 
>   abort() call shall not return.[/CX]
>
> [1] https://austingroupbugs.net/view.php?id=906#c5851

Okay, I missed that change.  So removing the unblocking should be okay
after this specification change.  I still don't see how the removal of
unblocking changes the signal mask observed by the signal handler,
though.

Thanks,
Florian

Re: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Florian Weimer]

* Adhemerval Zanella Netto:

>> I also don't quite understand why we need to take the abort lock in
>> posix_spawn.  There isn't any user code that can run in the same address
>> space after the vfork.
>
> My understanding is the potential issues is 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.

Right, I missed that.  Maybe add a comment?

Thanks,
Florian

Re: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275)

[Adhemerval Zanella Netto]

On 02/08/23 11:44, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>>> I also don't quite understand why we need to take the abort lock in
>>> posix_spawn.  There isn't any user code that can run in the same address
>>> space after the vfork.
>>
>> My understanding is the potential issues is 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.
> 
> Right, I missed that.  Maybe add a comment?

Ack.

Re: [PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Adhemerval Zanella Netto]

On 02/08/23 11:43, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>>> Ahh, you mean because use removed the signal unblocking from abort?
>>>
>>> If the signal is blocked, it is not delivered before it is unblocked.
>>> This means that the handler will not observe it blocked.
>>>
>>> But POSIX says this:
>>>
>>> | The abort() function shall override blocking or ignoring the SIGABRT
>>> | signal.
>>>
>>> It also says:
>>>
>>> | The SIGABRT signal shall be sent to the calling process as if by means
>>> | of raise() with the argument SIGABRT.
>>>
>>> Strictly speaking, it is impossible to comply with both requirements,
>>> but I think the handler is expected to run even if SIGABRT is blocked.
>>> As far as I understand it, the new code terminates the process in this
>>> case, without ever running the handler.
>>
>> The later has been changed with a new clarification [1]:
>>
>>   The SIGABRT signal shall be sent to the calling [CX]thread[/CX] as if by
>>   means of raise() with the argument SIGABRT. [CX]If this signal does not 
>>   terminate the process (for example, if the signal is caught and the handler 
>>   returns), abort() may change the disposition of SIGABRT to SIG_DFL and send 
>>   the signal (in the same way) again. If a second signal is sent and it does 
>>   not terminate the process, the behavior is unspecified, except that the 
>>   abort() call shall not return.[/CX]
>>
>> [1] https://austingroupbugs.net/view.php?id=906#c5851
> 
> Okay, I missed that change.  So removing the unblocking should be okay
> after this specification change.  I still don't see how the removal of
> unblocking changes the signal mask observed by the signal handler,
> though.

It is not by the signal handler, but rather after a SIGABRT handler issues
longjmp.  With default flags (0), SIGABRT will continue to be blocked:

$ cat test.c
#include <assert.h>
#include <setjmp.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>

static jmp_buf jb;

static void
sigabrt_handler (int sig)
{
  longjmp (jb, 1);
}

int main (int argc, char *argv[])
{
  sigset_t set;

  assert (sigprocmask (SIG_BLOCK, NULL, &set) == 0);
  printf ("SIGABRT blocked: %d\n", sigismember (&set, SIGABRT));

  struct sigaction sa = { .sa_handler = sigabrt_handler, .sa_flags = 0 };
  sigemptyset (&sa.sa_mask);
  assert (sigaction (SIGABRT, &sa, 0) == 0);

  if (setjmp (jb) == 0)
    abort ();

  printf ("first abort did not terminated\n");

  assert (sigprocmask (SIG_BLOCK, NULL, &set) == 0);
  printf ("SIGABRT blocked: %d\n", sigismember (&set, SIGABRT));

  if (setjmp (jb) == 0)
    abort ();

  printf ("second abort did not terminated\n");

  return 0;
}
$ gcc test.c -o test && ./test
SIGABRT blocked: 0
first abort did not terminated
SIGABRT blocked: 1
second abort did not terminated

By continuing to use _setjmp as 'setjmp' and not unblocking SIGABRT on abort
call, the process will be aborted regardless.  This is due the difference
historically between BSD and SysV regarding whether setjmp/longjmp should
save/restore the signal mask (POSIX current allows both semantics).