From patchwork Thu Oct 3 18:41:10 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 832429 Delivered-To: patch@linaro.org Received: by 2002:adf:8b52:0:b0:367:895a:4699 with SMTP id v18csp431570wra; Thu, 3 Oct 2024 11:43:29 -0700 (PDT) X-Forwarded-Encrypted: i=3; AJvYcCVPeW97DqSnpiB8z1Mja9PfFn1vg4e2uU+f4wIReiB1jZG+C9ClNPeMyRasqfoMwjpG5lc/PA==@linaro.org X-Google-Smtp-Source: AGHT+IFeE2CzTbTJhqDmvGX8zt4PzRup5agIIbib8o/ZeXa7D0KsGQRzceMhfbkZaZ02Wnwio8NK X-Received: by 2002:a05:6808:1808:b0:3e3:9fbb:4e2f with SMTP id 5614622812f47-3e3c154f2f5mr304167b6e.15.1727981009280; Thu, 03 Oct 2024 11:43:29 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1727981009; cv=pass; d=google.com; s=arc-20240605; b=RRN1qNWq53FGtHuhIBIaE8WLqhhynBI+I8JoUoBrLj4xjlb25K7hX5SleZS5dkqH3q 90QLrym0zS3e2WvEX1R65bJQtKnlwVq8trIbg+rfy4CBpquhFR1ARvNTN/29kV6ZmYbX boUEAy3wHth3e/0gFkjE+b4nnW0JqjtpQtLzo1kFqmqnA/W8SDjxclDBt1bTuE7u+oPK SZRu8RdeAPvdL3taBykZ6Et88SJV4pUfa2ShXSO+PW4xjj6les6KJSqqYR6J6ymRaDUb MEwKRMzgaNUZ6M8jJfh1y42B/TCT3sa1cYkGGp8yDWYwQiN7d0UzxpA1Hy5UOhXmSm9v TtEg== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=errors-to:list-subscribe:list-help:list-post:list-archive :list-unsubscribe:list-id:precedence:content-transfer-encoding :mime-version:message-id:date:subject:cc:to:from:dkim-signature :arc-filter:dmarc-filter:delivered-to; bh=yWO1E8slzO7NB/fJ7eOrczJtyacT0aPJS7uMVY6FLW8=; fh=QJZFeP/+ZBvxZSEjCFOrnUtvmpMYM/oP4HKTp37CJVk=; b=az92RGXQ9viuLO3KAQCfPOGpT1a/MIGYBRcris5JRERTse7bLy2saZs6u+AHhLpUm8 EyVAF6G2JGt1Xmy5svC8cC7Zmk6prrCVp50S+XENJmo6ESIDhbxwx+A+Jj+TZyOip3WN h5+TOgEZ7o+Wbudyq0ohYpm3ewhdhcA+4Y/aMFznwDfa/q9jQxAdRlH73dU+UDgUAKGL lSu6IQ1fWZjEd7qX9vd7aKDXTuLAuOd04X5dUoc4tPygm8wbM2YvbnZlZYFjUtOfUyqD u9zu3+vUNrH1lYh7X8Jip5+TImylEJ21gtFbuUEJJCa6pHwoX/XOcxiwVhiJ+6CIwOwz yuLA==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=Gr2xiQl+; arc=pass (i=1); spf=pass (google.com: domain of libc-alpha-bounces~patch=linaro.org@sourceware.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="libc-alpha-bounces~patch=linaro.org@sourceware.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from server2.sourceware.org (server2.sourceware.org. [8.43.85.97]) by mx.google.com with ESMTPS id d75a77b69052e-45d9524dfbdsi14938121cf.31.2024.10.03.11.43.29 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Oct 2024 11:43:29 -0700 (PDT) Received-SPF: pass (google.com: domain of libc-alpha-bounces~patch=linaro.org@sourceware.org designates 8.43.85.97 as permitted sender) client-ip=8.43.85.97; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=Gr2xiQl+; arc=pass (i=1); spf=pass (google.com: domain of libc-alpha-bounces~patch=linaro.org@sourceware.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="libc-alpha-bounces~patch=linaro.org@sourceware.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D80A8384A820 for ; Thu, 3 Oct 2024 18:43:28 +0000 (GMT) X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-pj1-x102f.google.com (mail-pj1-x102f.google.com [IPv6:2607:f8b0:4864:20::102f]) by sourceware.org (Postfix) with ESMTPS id C51AB385DDD1 for ; Thu, 3 Oct 2024 18:43:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C51AB385DDD1 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org C51AB385DDD1 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::102f ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1727980996; cv=none; b=P4fbZzx97BrCCb3Kn21UNDXvPl03Ouy70iSFkemIgDJBPIhzgqcyT25taAjZHXVVd79TvXDtfQdPoaptMWJmeb1T4QVBPhFZF1bYKkobFsd9JpkCSuh7N8kLWdD60UnNRbgMztqWu585Umo2gUu2fTlxOOpcdfBabsnZEsnCRUU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1727980996; c=relaxed/simple; bh=r1qH+0hJwJrDQ/wMKufJAiYpB3AfLJsolx5lsM1A/oU=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=pAvQ0ZpoQ9rFvz8049ZU24eUt7N+ZfQcPTjm1qSrXx0tsOnErztTtDfIQxQMSmWZvGelsdz8psw1B4tN6jOidP5OFnIxTjE2ajZd1tUPMXHwG9+AJn1uwRr9vrwh9i7f6HOdT/yHGm14PhfxU+U+PXwRsN384XSa8bWJb5qIdyo= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pj1-x102f.google.com with SMTP id 98e67ed59e1d1-2e09f67bc39so1153908a91.1 for ; Thu, 03 Oct 2024 11:43:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1727980991; x=1728585791; darn=sourceware.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=yWO1E8slzO7NB/fJ7eOrczJtyacT0aPJS7uMVY6FLW8=; b=Gr2xiQl+yhrGRVK/MiVVqm13BWl95ZgqHxYpal8ih6nVq+VVJXBqP5MFxODsccjQmj UEYzfvbXCL3PVLZMjpTK7/yef8C2lpdxrhkOnCmkhn1TAv92tjRzjWbWV1YiAH3ukMwR wbng1hwmpEuNW1hPs6sFNN2hmzdOAYq5nkd1jNMKPMTvIBWy7bYfzAp86qQGbXac0q7l 78wfl/SLdo4ssc8gax5qIS+R36hF5woJOm46EF5pA700fLZdPW/U97y7x4x4vdfyPeRg P/fLgbPjvaiQr8oQMrOtKp0i4vCAzSxTFk7TLclQz1oOnn5blCx/TL5xNBK8J3ic+5zc P7Cg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727980991; x=1728585791; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=yWO1E8slzO7NB/fJ7eOrczJtyacT0aPJS7uMVY6FLW8=; b=Ui4/F1mqjXv9zwgwIH6oC3EYELOUpEBj6/tTlnxLNdglxlXgQ4vkdFwT4xhMo1k1t6 8h6MZf4E64tKVzUWtmdZAGp1HnC4jQR5QGGy7ahhunR402dT7loBJ5Rif99sO3GmqxyA nnzFsqkwGYxuJkT9JjoW9M5fJIgPYF7Bh5+3Mvi+0wVOnG5Zh9U8H7nx4dETHVRpvWdK Mbhxql2pnwrFGKucIa4J7+eo7+w0i+tevpjoxRMEq+J2LKa3jBZHPu/5I1kYy6BsvjIK 0owklDptO7ma0lhXv0epb+9Css3naF09XpUExeCOf8ksMN085g7+EU+5+18COpvBnzTl U8Sw== X-Gm-Message-State: AOJu0YzSCeYDwbByARAbuE2QEHUpI4V2jkQUSnArSULPmbqAlEIYBnE9 juQzSJVWh2z9aUQENNtYCP6kKsnPhxk85suP3eZ1muEsT8BChwhCHes/huWdttaa8hAnW/WsD4c QW3I= X-Received: by 2002:a17:90a:a00b:b0:2e0:8740:26d1 with SMTP id 98e67ed59e1d1-2e1e6213b51mr67886a91.3.1727980991087; Thu, 03 Oct 2024 11:43:11 -0700 (PDT) Received: from mandiga.. ([2804:1b3:a7c1:b563:7492:69d4:7967:bf0d]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2e1bfb19489sm2030049a91.22.2024.10.03.11.43.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Oct 2024 11:43:10 -0700 (PDT) From: Adhemerval Zanella To: libc-alpha@sourceware.org Cc: Florian Weimer , Zack Weinberg , Carlos O'Donell Subject: [PATCH v5] stdlib: Make abort/_Exit AS-safe (BZ 26275) Date: Thu, 3 Oct 2024 15:41:10 -0300 Message-ID: <20241003184306.1915962-1-adhemerval.zanella@linaro.org> X-Mailer: git-send-email 2.43.0 MIME-Version: 1.0 X-Spam-Status: No, score=-12.7 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 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.30 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libc-alpha-bounces~patch=linaro.org@sourceware.org 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. --- Changes from v4: - Rebase against master. - Change profil.c to call __libc_sigaction. Changes from v3: - Use a read-write lock to avoid serialize _Fork and posix_spawn execution. - Only use the lock for libc, not for the loader. Changes from v2: - Dropped the setjmp to follow BSD semantic and adjusted the tests to use sigsetjmp. - Improve the abort documentation on why the lock is required as a QoI, and why it is still async-signal-safe. Changes from v1: - Rename de signal block and lock to __abort_lock_lock. - Improve comments on both abort, where the signal disposition can not be changed, and on posix_spawn on why it needs to take the abort lock. - Use gettid() on __pthread_raise_internal. - Added a NEWS entry for the setjmp fix. NEWS | 5 + debug/test-strcpy_chk.c | 6 +- debug/tst-fortify-wide.c | 4 +- debug/tst-fortify.c | 4 +- include/bits/unistd_ext.h | 3 + include/stdlib.h | 7 ++ manual/startup.texi | 9 +- nptl/pthread_kill.c | 11 ++ posix/fork.c | 2 + signal/sigaction.c | 15 ++- stdlib/abort.c | 138 ++++++++------------- sysdeps/generic/internal-signals.h | 27 +++- sysdeps/generic/internal-sigset.h | 26 ++++ sysdeps/htl/pthreadP.h | 2 + sysdeps/nptl/_Fork.c | 8 +- sysdeps/nptl/pthreadP.h | 1 + sysdeps/posix/profil.c | 6 +- sysdeps/unix/sysv/linux/internal-signals.h | 9 ++ sysdeps/unix/sysv/linux/internal-sigset.h | 2 +- sysdeps/unix/sysv/linux/spawni.c | 8 +- 20 files changed, 186 insertions(+), 107 deletions(-) create mode 100644 sysdeps/generic/internal-sigset.h diff --git a/NEWS b/NEWS index b1ae1c31ca..9afb3e67ae 100644 --- a/NEWS +++ b/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] diff --git a/debug/test-strcpy_chk.c b/debug/test-strcpy_chk.c index 14b11ea62a..55f2cc8768 100644 --- a/debug/test-strcpy_chk.c +++ b/debug/test-strcpy_chk.c @@ -59,7 +59,7 @@ static int test_main (void); #include 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, diff --git a/debug/tst-fortify-wide.c b/debug/tst-fortify-wide.c index 9c6f3af855..0f38e55e9c 100644 --- a/debug/tst-fortify-wide.c +++ b/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; \ diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c index ae738ff10a..e12d538458 100644 --- a/debug/tst-fortify.c +++ b/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; \ diff --git a/include/bits/unistd_ext.h b/include/bits/unistd_ext.h index 277be05746..eeb07baf70 100644 --- a/include/bits/unistd_ext.h +++ b/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 diff --git a/include/stdlib.h b/include/stdlib.h index 0cab3f5b56..57f4ab8545 100644 --- a/include/stdlib.h +++ b/include/stdlib.h @@ -20,6 +20,7 @@ # include # include +# include 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) diff --git a/manual/startup.texi b/manual/startup.texi index 8ac3b97eed..95b0ed8fc7 100644 --- a/manual/startup.texi +++ b/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{}}} -@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. +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}} 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 diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c index 71e5a7bf5b..fa5121a583 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) +{ + /* 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) { diff --git a/posix/fork.c b/posix/fork.c index 298765a1ff..c2b476ff2d 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 811062ae96..81ae550315 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,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) diff --git a/stdlib/abort.c b/stdlib/abort.c index e2b84baac4..f74ff3bb20 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,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 - 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_rwlock_define_initialized (static, lock); -/* We should be prepared for multiple threads trying to run abort. */ -__libc_lock_define_initialized_recursive (static, lock); +void +__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); +} -/* Cause an abnormal program termination with core-dump. */ 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. */ +_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); - 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); - } - - /* 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; + raise (SIGABRT); + + /* There is a SIGABRT handle 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 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); + + 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 3db100be10..e031a96bac 100644 --- a/sysdeps/generic/internal-signals.h +++ b/sysdeps/generic/internal-signals.h @@ -20,6 +20,7 @@ # define __INTERNAL_SIGNALS_H #include +#include #include #include #include @@ -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 */ diff --git a/sysdeps/generic/internal-sigset.h b/sysdeps/generic/internal-sigset.h new file mode 100644 index 0000000000..80279ffc47 --- /dev/null +++ b/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 + . */ + +#ifndef _INTERNAL_SIGSET_H +#define _INTERNAL_SIGSET_H + +#include + +typedef sigset_t internal_sigset_t; + +#endif diff --git a/sysdeps/htl/pthreadP.h b/sysdeps/htl/pthreadP.h index cf8a2efe86..b0c9ceb23a 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 fd2cfc7840..52c90e61e3 100644 --- a/sysdeps/nptl/_Fork.c +++ b/sysdeps/nptl/_Fork.c @@ -17,15 +17,17 @@ . */ #include +#include #include 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) diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h index 7d9b95e6ac..c2db165052 100644 --- a/sysdeps/nptl/pthreadP.h +++ b/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); diff --git a/sysdeps/posix/profil.c b/sysdeps/posix/profil.c index 3b3c87e25a..6f4aa00eeb 100644 --- a/sysdeps/posix/profil.c +++ b/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; diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h index a6fae59aaa..6e3a3d7692 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/internal-sigset.h b/sysdeps/unix/sysv/linux/internal-sigset.h index 5d7020b42d..4b19affd75 100644 --- a/sysdeps/unix/sysv/linux/internal-sigset.h +++ b/sysdeps/unix/sysv/linux/internal-sigset.h @@ -21,7 +21,7 @@ #include -typedef struct +typedef struct _internal_sigset_t { unsigned long int __val[__NSIG_WORDS]; } internal_sigset_t; diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index f57e92815e..bbb1a6f062 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/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 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. */ + __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);