From patchwork Mon Jul 31 17:19:00 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 73390 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 510EF385783F for ; Mon, 31 Jul 2023 17:19:37 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 510EF385783F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1690823977; bh=sDp+vq0k2NawskRbdBovmfUSfq0LEWMa+/0ukFs09Z8=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=HyacOda94MTBc9BYElpp8UN6iDwQVCBr0be61y4xoR6eTjhZdvXCXEWOaTVn8PZC3 Nli6DUpedKRbNr1d6+RG+w4iOQ7fM97UGRsaPY7J4IKbivadEOvqx6tUbBdKlJ+MMp jT1nTqv5uZ+/yk1Y28ETlSO3yHU0JKQKqqBBQyz0= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-oo1-xc36.google.com (mail-oo1-xc36.google.com [IPv6:2607:f8b0:4864:20::c36]) by sourceware.org (Postfix) with ESMTPS id E1A503858C66 for ; Mon, 31 Jul 2023 17:19:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E1A503858C66 Received: by mail-oo1-xc36.google.com with SMTP id 006d021491bc7-565f2567422so3264957eaf.2 for ; Mon, 31 Jul 2023 10:19:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690823949; x=1691428749; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=sDp+vq0k2NawskRbdBovmfUSfq0LEWMa+/0ukFs09Z8=; b=bMTATL/8axJePZDarQNu5uhz6fesGhl4ZafUITA/uoouB4jHft3nQlmRPVVQZvV/i/ SJS/Ffs3eMjRlTVCyaOrw+J2segvAbBMtgfgvuEbXViyTO2QCaJxLR7cTkbFzzsPKhrH PyiEw9e3xcnjSUvcEGzbNbDQew+GRs44mf4/JOWibk7IRAjj16lba5fwbyZNe1BslHE8 2C3BXRk6Kb1FHLtD926pgIXazWJ9AyivXaLAjeocpw1xH2HvO69U+yNhnMupXgSWA4mX +BFI7GLM/6OfoOrT23sqPYWpAN3aPUptYHiBQuBejwIpyvxyga0Dkd31jj+HAN82XLnh l0OA== X-Gm-Message-State: ABy/qLbhc9pHIppzZ/A+mu/zn6UhRZjAvcB5LBsDGf2ppON5HtF2Ua4z gjmfVUcJw+qeO1NiuwO0oJAiV3UO0LkezXZLg9+Cvw== X-Google-Smtp-Source: APBJJlFc/0/KjJ0zDcayUQ+myUPIehQfch+YGdg63v3USRj76bikeq84kHpoeWzNj5aOGrVEvXvTdQ== X-Received: by 2002:a05:6808:2a59:b0:3a7:17b6:3501 with SMTP id fa25-20020a0568082a5900b003a717b63501mr6484124oib.23.1690823949151; Mon, 31 Jul 2023 10:19:09 -0700 (PDT) Received: from mandiga.. ([2804:1b3:a7c1:440b:68be:64f1:9a6f:2423]) by smtp.gmail.com with ESMTPSA id k16-20020a05680808d000b003a724566afdsm1560047oij.20.2023.07.31.10.19.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 31 Jul 2023 10:19:08 -0700 (PDT) To: libc-alpha@sourceware.org, Carlos O'Donell Subject: [PATCH 2/2] stdlib: Make abort AS-safe (BZ 26275) Date: Mon, 31 Jul 2023 14:19:00 -0300 Message-Id: <20230731171900.4065501-3-adhemerval.zanella@linaro.org> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20230731171900.4065501-1-adhemerval.zanella@linaro.org> References: <20230731171900.4065501-1-adhemerval.zanella@linaro.org> MIME-Version: 1.0 X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Adhemerval Zanella via Libc-alpha From: Adhemerval Zanella Netto Reply-To: Adhemerval Zanella Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" 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 @@ . */ #include -#include #include +#include +#include /* 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 . */ -#include #include -#include -#include -#include -#include #include +#include +#include +#include /* 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 @@ . */ #include +#include #include 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). */