From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-2.6 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 4B656249F6 for ; Sat, 20 Jan 2024 09:12:43 +0100 (CET) Received: (qmail 21659 invoked by uid 550); 20 Jan 2024 08:10:43 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 21623 invoked from network); 20 Jan 2024 08:10:43 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=s31663417; t=1705738350; x=1706343150; i=nullplan@gmx.net; bh=UySEcJ7GQ0yc0jkffLBsXiRHlrzBXfE3rkd/3OL9Vf8=; h=X-UI-Sender-Class:Date:From:To:Subject; b=qxiG0oYO7I6jUYjW2n1Ya5QRV9oL2MXGXxm4Zod3v1OUGl/JkbhqPbUJNoirjkXA P6XUPF+UHE5uYwZWBf8ltDX5RDgcCwGnB23GhiINHoagoZ5L6JvIPas2UjVWM5ZVS KhQOA6bqfTMC3BBjjnvsmJBKfU5exvo+dcs6p7WWNrpUV1PcZAdLtTmTCPTDl44dG lAtQl3L6HZpIKJ263K73ksrl4OevjMYDk4GP1zXi9bQ/ctjLQIkKrChA/ztmnAol/ EZL8LK3mAlJiOsaO54Lr63zAiQY04rVITwCCe67CrRA6J2ZIqkEOXl5OkeDFB4IIz YHPzvZNRsP2x57RWbw== X-UI-Sender-Class: 724b4f7f-cbec-4199-ad4e-598c01a50d3a Date: Sat, 20 Jan 2024 09:12:28 +0100 From: Markus Wichmann To: musl@lists.openwall.com Message-ID: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="FfvHggis9fu6bWQ8" Content-Disposition: inline X-Provags-ID: V03:K1:V9yysicorgQ7b2MUm8r/HgkMbrlVKv0UG1j0FyFcRzQRXq+0vB3 cNf9M82BdNGUGCuSPLPiJw0o0OFHLgVc97w+WC+/Mh/FBOSGCYIqD8OzHhtBuN7uLGn1Pbq DHxw2uRfJGgZc1Tupi8ONRcnwx785wGWY9gStgncz8k6eia9A5il3dmwC1qORuReK1ciAI4 7SvrZbgrn97uQnhwHsRxw== UI-OutboundReport: notjunk:1;M01:P0:P5RaXjXlvBg=;jmdf+SdAQ5tkLpfY3lqpRfnfaKZ H/6fVqvBEMaKiSvM7eCQX3kqgwT8q8x0uEN/APYekjFyrL3BkfvVqE8mQUl0gtQxqWzX3Usfx ifcjnMeUyWAiHAISSQ8DKoPJjrGVD+UmMvxV6m5VMpceQytOgL0KeYNAfjW4rI3CrdQ4fBXsd eJiuDAyracH6pvWixMmaHaxQecamvJVuq5Paw0PUt48Yfxly1ZMSsiW2o492j8jQzDhelyAGP +txkjHpOfwtW0wHxdWKrktiJa7raoAqhvqsOPY8Ff7/QVmes4nO2mQPaI9jWWQ7JSyMdoKYk3 a8eI7O6zDF2mwlCHZwCTnfCdT1atHBBCgrK4X/aQJyp8Q6RIRly95vs5iGkPpPE05AuyaUEO1 beGsOgFSIQ6pkxnqjr+DIkDtmeEq5YUkawruN5u30MluKTEQQrNYf71ri+S8POAApitSAd0Zr CWcUoad5oA9MnRKoFNDFWiHOdCQ7cPq6YCTRlpRbtqKzx1gqLMriPV3JCMK31YL3GvaacBUSZ mtAC35KnbHCp0Nwe2ur76oUE5H0rDBb950KPiSiRigJK2f33bx86TolixyrmpOTc1PPiUR2s4 BdZj6dZIRGaxrFku2TuCfKzGizVbeMv2YmRQDB28a7shuon82+An2gWlrDvFVGBrtx/txYXKd bhdJ5jLdFHsOp1/qyjtbjbRgPxECBlV0aZbW776LEn7/f6VAJmA92wWTs8g+N38bwzPXkeoqW Wh8L9gUaFhUGKd7mBLBedwkgi6FjXeHuPAinWMbxkbAyxMZxTTVp2js/UlwS6TBlxQOkRVLE2 RBIyeQwSSFQYRZQuPVI1X5UBeDlQh4boqU2k9vQaTzCQqmp+/yr+8MgnRr55KIiHpjW6a/eN1 A9ye3kUP6BMjrGf95pI6R6t0lbXwbT2rcIKKCgkgcKA0jztE7EmhTWabNprc0gA0znJ0c0tGz kK1fpA== Subject: [musl] [PATCH] Split fork and abort locks --FfvHggis9fu6bWQ8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi all, a while ago I had noticed that __abort_lock was being taken in some functions that have nothing to do with SIGABRT. Namely in the forking functions. Investigating this a bit, I noticed that __abort_lock had become dual purpose. But this is a code smell. Actually, there are several locks that have expanded in scope a bit since their introduction. At least the ptc lock (__inhibit_ptc() et al.) deserves a closer look later on as well. Seems to me like in case of the default stack size, that lock is used simply because an rwlock was needed and this one was around. The corruption in this case probably came from posix_spawn(). That function takes the abort lock, because, as a baffling comment above the lock statement tells us, this guards against SIGABRT disposition changing. This is because abort() changes the disposition without calling sigaction(), so a handler would not be noted in the handler set. However, actually posix_spawn() does not need to care about this. The handler set is strictly additive, so all the signals it contains /may/ have a handler. And abort() removes a handler. In the worst case, the handler set will spuriously contain SIGABRT, which is a condition the child needs to check for anyway. And a concurrent sigaction() call from the application establishing a handler is no different for SIGABRT than for any other signal. That is handled by retrieving the handler set in the child, when everything is fixed since the child is single-threaded. For the same reason, there cannot be a concurrent call to abort() in the child. Anyway, with posix_spawn() taking __abort_lock, when it came time to guard against leaking communication pipes to concurrently created child processes, it was only natural to use the same lock in clone() and _Fork(). But this is a new purpose and deserves a new lock. So I am submitting a patch to introduce that lock. Ciao, Markus --FfvHggis9fu6bWQ8 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-Introduce-new-fork-lock-separate-from-__abort_lock.patch" Content-Transfer-Encoding: quoted-printable =46rom 7931b41e4b7ff7bc0c25d3141fef1fdc113b8203 Mon Sep 17 00:00:00 2001 From: Markus Wichmann Date: Sat, 20 Jan 2024 08:44:04 +0100 Subject: [PATCH] Introduce new fork lock separate from __abort_lock. The abort lock was being taken in several functions that have nothing to do with SIGABRT or signal dispositions. The lock had become dual-purpose, guarding against SIGABRT disposition changes and against a concurrent fork. But these are two different purposes, so use a new lock for the second one. posix_spawn() has no reason to care about the SIGABRT disposition any more than any other one, and it only changes signal dispositions in the child process, which cannot call abort() concurrently because it is single-threaded. =2D-- src/internal/fork_impl.h | 1 + src/linux/clone.c | 2 +- src/process/_Fork.c | 4 ++-- src/process/fork_lock.c | 3 +++ src/process/posix_spawn.c | 10 +++++----- 5 files changed, 12 insertions(+), 8 deletions(-) create mode 100644 src/process/fork_lock.c diff --git a/src/internal/fork_impl.h b/src/internal/fork_impl.h index f995fce2..3889d38a 100644 =2D-- a/src/internal/fork_impl.h +++ b/src/internal/fork_impl.h @@ -13,6 +13,7 @@ extern hidden volatile int *const __timezone_lockptr; extern hidden volatile int *const __bump_lockptr; extern hidden volatile int *const __vmlock_lockptr; +extern hidden volatile int __fork_lock[1]; hidden void __malloc_atfork(int); hidden void __ldso_atfork(int); diff --git a/src/linux/clone.c b/src/linux/clone.c index 257c1cec..1e4201ae 100644 =2D-- a/src/linux/clone.c +++ b/src/linux/clone.c @@ -51,7 +51,7 @@ int clone(int (*func)(void *), void *stack, int flags, v= oid *arg, ...) __clone(func, stack, flags, arg, ptid, tls, ctid)); __block_all_sigs(&csa.sigmask); - LOCK(__abort_lock); + LOCK(__fork_lock); /* Setup the a wrapper start function for the child process to do * mimic _Fork in producing a consistent execution state. */ diff --git a/src/process/_Fork.c b/src/process/_Fork.c index 9c07792d..370191c1 100644 =2D-- a/src/process/_Fork.c +++ b/src/process/_Fork.c @@ -22,7 +22,7 @@ void __post_Fork(int ret) libc.threads_minus_1 =3D 0; if (libc.need_locks) libc.need_locks =3D -1; } - UNLOCK(__abort_lock); + UNLOCK(__fork_lock); if (!ret) __aio_atfork(1); } @@ -31,7 +31,7 @@ pid_t _Fork(void) pid_t ret; sigset_t set; __block_all_sigs(&set); - LOCK(__abort_lock); + LOCK(__fork_lock); #ifdef SYS_fork ret =3D __syscall(SYS_fork); #else diff --git a/src/process/fork_lock.c b/src/process/fork_lock.c new file mode 100644 index 00000000..204af1f4 =2D-- /dev/null +++ b/src/process/fork_lock.c @@ -0,0 +1,3 @@ +#include "fork_impl.h" + +volatile int __fork_lock[1]; diff --git a/src/process/posix_spawn.c b/src/process/posix_spawn.c index 728551b3..74883527 100644 =2D-- a/src/process/posix_spawn.c +++ b/src/process/posix_spawn.c @@ -9,6 +9,7 @@ #include "lock.h" #include "pthread_impl.h" #include "fdop.h" +#include "fork_impl.h" struct args { int p[2]; @@ -180,12 +181,11 @@ int posix_spawn(pid_t *restrict res, const char *res= trict path, args.envp =3D envp; pthread_sigmask(SIG_BLOCK, SIGALL_SET, &args.oldmask); - /* The lock guards both against seeing a SIGABRT disposition change - * by abort and against leaking the pipe fd to fork-without-exec. */ - LOCK(__abort_lock); + /* The lock guards against leaking the pipe fd to fork-without-exec. */ + LOCK(__fork_lock); if (pipe2(args.p, O_CLOEXEC)) { - UNLOCK(__abort_lock); + UNLOCK(__fork_lock); ec =3D errno; goto fail; } @@ -193,7 +193,7 @@ int posix_spawn(pid_t *restrict res, const char *restr= ict path, pid =3D __clone(child, stack+sizeof stack, CLONE_VM|CLONE_VFORK|SIGCHLD, &args); close(args.p[1]); - UNLOCK(__abort_lock); + UNLOCK(__fork_lock); if (pid > 0) { if (read(args.p[0], &ec, sizeof ec) !=3D sizeof ec) ec =3D 0; =2D- 2.39.2 --FfvHggis9fu6bWQ8--