Re: musl 1.1.17 released

[Petr Skocik <pskocik@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2843 bytes --]

Hi.
Since posix_spawn is being discussed, I'd like to report a what I think is
a bug in the current implementation:
Since the child is unmasking its signals only after it's performed its file
actions, the process may get blocked indefinitely (e.g., on a FIFO
open) while being unresponsive to signals.

Example program (with close to no error checking):


#define _GNU_SOURCE
#include <unistd.h>
#include <spawn.h>
#include <stdlib.h>
#include <fcntl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/wait.h>
int main()
{
pid_t pid;
mkfifo("fifo", 0640);
posix_spawn_file_actions_t fa;
posix_spawn_file_actions_init(&fa);
posix_spawn_file_actions_addopen(&fa, 0, "fifo", O_RDONLY, 0);
posix_spawnp(&pid, "tr", &fa, 0, (char *const[]){ "tr", "a-z", "A-Z", 0},
environ);

        //will get stuck here

}


It  think the pthread_mask call should be moved above the file actions,
which fixes this problem.

Additionally, as far as extensions to the current POSIX standard are
concerned, adding the herein (http://austingroupbugs.net/view.php?id=411)
proposed change to *adddup2 semantics (clearing the FD_CLOEXEC flag on the
 (given target in dup2 file actions where the source and target
filedescriptor are identical) would be super nice (the reasoning for it
should be in the link).

Finally, I noticed the error passing code can be reduced to a simple write
to a volatile int made from the child (purely cosmetic).

Attached are patches for these 3 things in case you wanted them. (I hope
I'm not doing something stupid.)

Best regards and thanks for the project!

On Thu, Oct 19, 2017 at 10:13 PM, Rich Felker <dalias@libc.org> wrote:

> *** Due to at least one fairly serious security bug, all users should
> upgrade or patch. ***
>
>
> This release fixes numerous bugs affecting visible behavior and
> safety/internal consistency, including a stack-based buffer overflow
> in dns parsing and multiple sources of invalid memory accesses that
> may lead to crashes. See the release notes in WHATSNEW for details.
>
> Many new features have also been added, including deferred symbol
> binding in the dynamic linker (RTLD_LAZY emulation), an option to
> overrid argv[0] when running ldso to execute a program, support for
> starting new sessions via posix_spawn (POSIX_SPAWN_SETSID, accepted
> for standardization), and ability to query the active thread-local
> locale (via _NL_LOCALE_NAME extension). Improvements in compatibility
> with applications, build tools, and platforms have also been made.
>
> https://www.musl-libc.org/releases/musl-1.1.17.tar.gz
> https://www.musl-libc.org/releases/musl-1.1.17.tar.gz.asc
>
> Special thanks to musl's release sponsors (patreon.com/musl):
>
> * Justin Cormack
> * Jevin Sweval
> * Les Aker
> * Neal Gompa
> * Hurricane Labs (hurricanelabs.com)
> * The Midipix Project (midipix.org)
>

[-- Attachment #1.2: Type: text/html, Size: 4844 bytes --]

[-- Attachment #2: 0001-simplify-posix_spawn-by-avoiding-the-error-pipe.patch --]
[-- Type: text/x-patch, Size: 3161 bytes --]

From c241f622624fb0849864d68c7048e98f63812b41 Mon Sep 17 00:00:00 2001
From: Petr Skocik <gmail@pskocik.com>
Date: Tue, 17 Oct 2017 00:13:43 +0200
Subject: [PATCH 1/3] simplify posix_spawn by avoiding the error pipe

---
 src/process/posix_spawn.c | 32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

diff --git a/src/process/posix_spawn.c b/src/process/posix_spawn.c
index ea5d2998..d4377288 100644
--- a/src/process/posix_spawn.c
+++ b/src/process/posix_spawn.c
@@ -11,7 +11,7 @@
 #include "libc.h"
 
 struct args {
-	int p[2];
+	volatile int ret;
 	sigset_t oldmask;
 	const char *path;
 	int (*exec)(const char *, char *const *, char *const *);
@@ -41,12 +41,10 @@ static int child(void *args_vp)
 	int i, ret;
 	struct sigaction sa = {0};
 	struct args *args = args_vp;
-	int p = args->p[1];
 	const posix_spawn_file_actions_t *fa = args->fa;
 	const posix_spawnattr_t *restrict attr = args->attr;
 	sigset_t hset;
 
-	close(args->p[0]);
 
 	/* All signal dispositions must be either SIG_DFL or SIG_IGN
 	 * before signals are unblocked. Otherwise a signal handler
@@ -94,16 +92,6 @@ static int child(void *args_vp)
 		int fd;
 		for (op = fa->__actions; op->next; op = op->next);
 		for (; op; op = op->prev) {
-			/* It's possible that a file operation would clobber
-			 * the pipe fd used for synchronizing with the
-			 * parent. To avoid that, we dup the pipe onto
-			 * an unoccupied fd. */
-			if (op->fd == p) {
-				ret = __syscall(SYS_dup, p);
-				if (ret < 0) goto fail;
-				__syscall(SYS_close, p);
-				p = ret;
-			}
 			switch(op->cmd) {
 			case FDOP_CLOSE:
 				__syscall(SYS_close, op->fd);
@@ -125,12 +113,6 @@ static int child(void *args_vp)
 		}
 	}
 
-	/* Close-on-exec flag may have been lost if we moved the pipe
-	 * to a different fd. We don't use F_DUPFD_CLOEXEC above because
-	 * it would fail on older kernels and atomicity is not needed --
-	 * in this process there are no threads or signal handlers. */
-	__syscall(SYS_fcntl, p, F_SETFD, FD_CLOEXEC);
-
 	pthread_sigmask(SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
 		? &attr->__mask : &args->oldmask, 0);
 
@@ -140,7 +122,7 @@ static int child(void *args_vp)
 fail:
 	/* Since sizeof errno < PIPE_BUF, the write is atomic. */
 	ret = -ret;
-	if (ret) while (__syscall(SYS_write, p, &ret, sizeof ret) < 0);
+	if (ret) args->ret = ret;
 	_exit(127);
 }
 
@@ -156,9 +138,6 @@ int __posix_spawnx(pid_t *restrict res, const char *restrict path,
 	int ec=0, cs;
 	struct args args;
 
-	if (pipe2(args.p, O_CLOEXEC))
-		return errno;
-
 	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
 
 	args.path = path;
@@ -171,17 +150,14 @@ int __posix_spawnx(pid_t *restrict res, const char *restrict path,
 
 	pid = __clone(child, stack+sizeof stack,
 		CLONE_VM|CLONE_VFORK|SIGCHLD, &args);
-	close(args.p[1]);
 
 	if (pid > 0) {
-		if (read(args.p[0], &ec, sizeof ec) != sizeof ec) ec = 0;
-		else waitpid(pid, &(int){0}, 0);
+		if (0!=(ec=args.ret)) 
+			waitpid(pid, &(int){0}, 0);
 	} else {
 		ec = -pid;
 	}
 
-	close(args.p[0]);
-
 	if (!ec && res) *res = pid;
 
 	pthread_sigmask(SIG_SETMASK, &args.oldmask, 0);
-- 
2.14.2


[-- Attachment #3: 0002-make-addup2-remove-FD_CLOEXEC-if-the-descriptors-are.patch --]
[-- Type: text/x-patch, Size: 2337 bytes --]

From e38b822525a7e4cd5ebba5bc81c4fef83f6ec277 Mon Sep 17 00:00:00 2001
From: Petr Skocik <gmail@pskocik.com>
Date: Wed, 18 Oct 2017 02:25:19 +0200
Subject: [PATCH 2/3] make addup2 remove FD_CLOEXEC if the descriptors are
 equal

Consider the following scenario:

1. The user is in a multithreaded program.
2. They don't want to use multiple posix_spawn_file_actions_addopen
   because posix_spawn can't report _which_ open failed
3. So they open with O_CLOEXEC in the parent and use posix_spawn_file_actions_adddup2
   to dup it onto the right fd in the child while removing the O_CLOEXEC
   flag
4. If, by chance, the file opened at the right filedescriptor,
   then due to the semantics of dup2, it will remain FD_CLOEXEC
   and hence won't carry over across execve.

Losing the O_CLOEXEC flag in the MT parent would lead to races
and since there's not posix_spawn_file_actions_addfcntl, I think the
solution should be to remove the CLOEXEC flag in the child if
the dup2 source and target are identical.
---
 src/process/posix_spawn.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/process/posix_spawn.c b/src/process/posix_spawn.c
index d4377288..137898d8 100644
--- a/src/process/posix_spawn.c
+++ b/src/process/posix_spawn.c
@@ -10,6 +10,17 @@
 #include "fdop.h"
 #include "libc.h"
 
+/*
+ the user may wish to adddup2 a CLOEXEC filedescriptor 
+ this should remove the CLOEXEC flag and the user shouldn't be expected to do it
+ because if the CLOEXEC flag is on, it probably needs to be on, because the parent
+ is multithreaded
+*/
+static int dup3_or_set_cloexec(int SrcFd, int DestFd, int Flg)
+{
+	rt (DestFd!=SrcFd) ? dup3(SrcFd,DestFd,Flg) : fcntl(DestFd,F_SETFD,Flg);
+}
+
 struct args {
 	volatile int ret;
 	sigset_t oldmask;
@@ -97,14 +108,14 @@ static int child(void *args_vp)
 				__syscall(SYS_close, op->fd);
 				break;
 			case FDOP_DUP2:
-				if ((ret=__sys_dup2(op->srcfd, op->fd))<0)
+				if ((ret=dup3_or_set_cloexec(op->srcfd, op->fd, 0))<0)
 					goto fail;
 				break;
 			case FDOP_OPEN:
 				fd = __sys_open(op->path, op->oflag, op->mode);
 				if ((ret=fd) < 0) goto fail;
 				if (fd != op->fd) {
-					if ((ret=__sys_dup2(fd, op->fd))<0)
+					if ((ret=dup3_or_set_cloexec(fd, op->fd, 0))<0)
 						goto fail;
 					__syscall(SYS_close, fd);
 				}
-- 
2.14.2


[-- Attachment #4: 0003-posix_spawn-unmask-signals-before-potentially-long-b.patch --]
[-- Type: text/x-patch, Size: 1925 bytes --]

From 9e29bd9e59dc17b6505f72303fd37d15c95d1b02 Mon Sep 17 00:00:00 2001
From: Petr Skocik <gmail@pskocik.com>
Date: Wed, 18 Oct 2017 11:46:42 +0200
Subject: [PATCH 3/3] posix_spawn: unmask signals before potentially
 long-blocking calls

---
 src/process/posix_spawn.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/src/process/posix_spawn.c b/src/process/posix_spawn.c
index 137898d8..72a95a45 100644
--- a/src/process/posix_spawn.c
+++ b/src/process/posix_spawn.c
@@ -10,15 +10,9 @@
 #include "fdop.h"
 #include "libc.h"
 
-/*
- the user may wish to adddup2 a CLOEXEC filedescriptor 
- this should remove the CLOEXEC flag and the user shouldn't be expected to do it
- because if the CLOEXEC flag is on, it probably needs to be on, because the parent
- is multithreaded
-*/
 static int dup3_or_set_cloexec(int SrcFd, int DestFd, int Flg)
 {
-	rt (DestFd!=SrcFd) ? dup3(SrcFd,DestFd,Flg) : fcntl(DestFd,F_SETFD,Flg);
+	return (DestFd!=SrcFd) ? dup3(SrcFd,DestFd,Flg) : fcntl(DestFd,F_SETFD,Flg);
 }
 
 struct args {
@@ -81,6 +75,7 @@ static int child(void *args_vp)
 		}
 		__libc_sigaction(i, &sa, 0);
 	}
+	
 
 	if (attr->__flags & POSIX_SPAWN_SETSID)
 		if ((ret=__syscall(SYS_setsid)) < 0)
@@ -98,6 +93,12 @@ static int child(void *args_vp)
 		    (ret=__syscall(SYS_setuid, __syscall(SYS_getuid))) )
 			goto fail;
 
+	/*file actions (opens) are potentially long-blocking, so the process should
+	 become interruptible before they're invoked */
+	pthread_sigmask(SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
+		? &attr->__mask : &args->oldmask, 0);
+
+
 	if (fa && fa->__actions) {
 		struct fdop *op;
 		int fd;
@@ -124,9 +125,6 @@ static int child(void *args_vp)
 		}
 	}
 
-	pthread_sigmask(SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
-		? &attr->__mask : &args->oldmask, 0);
-
 	args->exec(args->path, args->argv, args->envp);
 	ret = -errno;
 
-- 
2.14.2