musl 1.1.17 released

musl 1.1.17 released

[Rich Felker]

*** 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)

Re: musl 1.1.17 released

[Petr Skocik]

[-- 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

posix_spawn topics [was: Re: [musl] musl 1.1.17 released]

[Rich Felker]

On Fri, Oct 20, 2017 at 02:11:35AM +0200, Petr Skocik wrote:
> 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.

It's actually rather ugly that this happens at all. Your "fix" is
presumably that ^C now works, which of course helps. But short of
terminals signals there's no legitimate way to kill the hung process
because its pid has not been returned yet.

I'm not really sure what should happen here; it may call for an
interpretation request.

> 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).

I wasn't aware of that part of #411; I agree it should be adopted now
without waiting for a new standard.

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

The pipe is there for a very important reason, and the reason is not
passing the error. It just happens that, since we already have to read
from it in the parent anyway, it makes a convenient channel to pass
the error on. Your modified version has serious memory corruption
because the parent continues running (and may return from posix_spawn)
after args->ret is set but before the child exits, thereby ending the
lifetime of the child's stack. Atomicity of closing of the
close-on-exec pipe with respect to exec is the property we're using
here to detect when exec has taken effect and memory is no longer
shared.

An argument can be made that CLONE_VFORK mandates the schedudler
prevent this, but historically it did not always do this correctly,
especially when traced (and I believe it's still possible when tracing
to manually resume the parent while the child is still sharing
memory), and the intent is to be compatible with kernels where
CLONE_VFORK is a nop. The main reason CLONE_VFORK is used is actually
as a hint to qemu-userlevel to get it to emulate things right.

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

Otherwise no, not "stupid", but the patches are a lot larger/more
invasive than they need to be and don't follow coding style.

> @@ -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)

This (FDOP_OPEN) is not a case covered by the POSIX interpretation,
and using it here would actually be wrong if it were reachable, but
you're already inside "if (fd != op->fd)" and thus there's no way
fd==op->fd can be true. So the change in this line is just spurious.
There's then just one place where dup3_or_set_cloexec is called (the
previous case), and it doesn't make sense to have a function; rather
the conditional should just be inline in that case.

Rich

Re: posix_spawn topics [was: Re: [musl] musl 1.1.17 released]

[Petr Skocik]

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

Thanks for the feedback. I was using it modified like this because  I need
the enhanced adddup2 semantics since I want to do all my opens in the
parent for more fine-grained error reporting.  So I only really care about
clearing the FD_CLOEXEC flag in the FDOP_DUP2 case, and so I'm glad you're
willing to accept the semantics change. (Attached is perhaps a mergeable
patch.)

On Fri, Oct 20, 2017 at 3:17 AM, Rich Felker <dalias@libc.org> wrote:

> On Fri, Oct 20, 2017 at 02:11:35AM +0200, Petr Skocik wrote:
> > 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.
>
> It's actually rather ugly that this happens at all. Your "fix" is
> presumably that ^C now works, which of course helps. But short of
> terminals signals there's no legitimate way to kill the hung process
> because its pid has not been returned yet.
>
> I'm not really sure what should happen here; it may call for an
> interpretation request.
>
> > 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).
>
> I wasn't aware of that part of #411; I agree it should be adopted now
> without waiting for a new standard.
>
> > Finally, I noticed the error passing code can be reduced to a simple
> write
> > to a volatile int made from the child (purely cosmetic).
>
> The pipe is there for a very important reason, and the reason is not
> passing the error. It just happens that, since we already have to read
> from it in the parent anyway, it makes a convenient channel to pass
> the error on. Your modified version has serious memory corruption
> because the parent continues running (and may return from posix_spawn)
> after args->ret is set but before the child exits, thereby ending the
> lifetime of the child's stack. Atomicity of closing of the
> close-on-exec pipe with respect to exec is the property we're using
> here to detect when exec has taken effect and memory is no longer
> shared.
>
> An argument can be made that CLONE_VFORK mandates the schedudler
> prevent this, but historically it did not always do this correctly,
> especially when traced (and I believe it's still possible when tracing
> to manually resume the parent while the child is still sharing
> memory), and the intent is to be compatible with kernels where
> CLONE_VFORK is a nop. The main reason CLONE_VFORK is used is actually
> as a hint to qemu-userlevel to get it to emulate things right.
>
> > Attached are patches for these 3 things in case you wanted them. (I hope
> > I'm not doing something stupid.)
>
> Otherwise no, not "stupid", but the patches are a lot larger/more
> invasive than they need to be and don't follow coding style.
>
> > @@ -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)
>
> This (FDOP_OPEN) is not a case covered by the POSIX interpretation,
> and using it here would actually be wrong if it were reachable, but
> you're already inside "if (fd != op->fd)" and thus there's no way
> fd==op->fd can be true. So the change in this line is just spurious.
> There's then just one place where dup3_or_set_cloexec is called (the
> previous case), and it doesn't make sense to have a function; rather
> the conditional should just be inline in that case.
>
> Rich
>

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

[-- Attachment #2: 0001-New-posix_spawn_file_actions_adddup2-semantics.patch --]
[-- Type: text/x-patch, Size: 1168 bytes --]

From 8e729632a04746baa565f45cf5fdae225db1420b Mon Sep 17 00:00:00 2001
From: Petr Skocik <gmail@pskocik.com>
Date: Fri, 20 Oct 2017 10:50:24 +0200
Subject: [PATCH] New posix_spawn_file_actions_adddup2 semantics

Clear FD_CLOEXEC on the target fd in dup2 file actions where target fd == src_fd.
Ref: http://austingroupbugs.net/view.php?id=411
---
 src/process/posix_spawn.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/process/posix_spawn.c b/src/process/posix_spawn.c
index ea5d2998..721d077d 100644
--- a/src/process/posix_spawn.c
+++ b/src/process/posix_spawn.c
@@ -109,8 +109,15 @@ static int child(void *args_vp)
 				__syscall(SYS_close, op->fd);
 				break;
 			case FDOP_DUP2:
-				if ((ret=__sys_dup2(op->srcfd, op->fd))<0)
-					goto fail;
+				if (op->srcfd != op->fd){
+					if ((ret=__sys_dup2(op->srcfd, op->fd))<0)
+						goto fail;
+				}else{
+					if ((ret=__syscall(SYS_fcntl, op->fd, F_GETFD))<0)
+						goto fail;
+					if ((ret=__syscall(SYS_fcntl, op->fd, F_SETFD, ret & ~FD_CLOEXEC))
+						goto fail;
+				}
 				break;
 			case FDOP_OPEN:
 				fd = __sys_open(op->path, op->oflag, op->mode);
-- 
2.14.2

Re: posix_spawn topics [was: Re: [musl] musl 1.1.17 released]

[Petr Skočík]

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

Patch patch (Forgot `<0`)

---------- Forwarded message ----------
From: Petr Skocik <pskocik@gmail.com>
Date: Fri, Oct 20, 2017 at 11:02 AM
Subject: Re: [musl] posix_spawn topics [was: Re: [musl] musl 1.1.17
released]
To: musl@lists.openwall.com


Thanks for the feedback. I was using it modified like this because  I need
the enhanced adddup2 semantics since I want to do all my opens in the
parent for more fine-grained error reporting.  So I only really care about
clearing the FD_CLOEXEC flag in the FDOP_DUP2 case, and so I'm glad you're
willing to accept the semantics change. (Attached is perhaps a mergeable
patch.)

On Fri, Oct 20, 2017 at 3:17 AM, Rich Felker <dalias@libc.org> wrote:

> On Fri, Oct 20, 2017 at 02:11:35AM +0200, Petr Skocik wrote:
> > 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.
>
> It's actually rather ugly that this happens at all. Your "fix" is
> presumably that ^C now works, which of course helps. But short of
> terminals signals there's no legitimate way to kill the hung process
> because its pid has not been returned yet.
>
> I'm not really sure what should happen here; it may call for an
> interpretation request.
>
> > 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).
>
> I wasn't aware of that part of #411; I agree it should be adopted now
> without waiting for a new standard.
>
> > Finally, I noticed the error passing code can be reduced to a simple
> write
> > to a volatile int made from the child (purely cosmetic).
>
> The pipe is there for a very important reason, and the reason is not
> passing the error. It just happens that, since we already have to read
> from it in the parent anyway, it makes a convenient channel to pass
> the error on. Your modified version has serious memory corruption
> because the parent continues running (and may return from posix_spawn)
> after args->ret is set but before the child exits, thereby ending the
> lifetime of the child's stack. Atomicity of closing of the
> close-on-exec pipe with respect to exec is the property we're using
> here to detect when exec has taken effect and memory is no longer
> shared.
>
> An argument can be made that CLONE_VFORK mandates the schedudler
> prevent this, but historically it did not always do this correctly,
> especially when traced (and I believe it's still possible when tracing
> to manually resume the parent while the child is still sharing
> memory), and the intent is to be compatible with kernels where
> CLONE_VFORK is a nop. The main reason CLONE_VFORK is used is actually
> as a hint to qemu-userlevel to get it to emulate things right.
>
> > Attached are patches for these 3 things in case you wanted them. (I hope
> > I'm not doing something stupid.)
>
> Otherwise no, not "stupid", but the patches are a lot larger/more
> invasive than they need to be and don't follow coding style.
>
> > @@ -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)
>
> This (FDOP_OPEN) is not a case covered by the POSIX interpretation,
> and using it here would actually be wrong if it were reachable, but
> you're already inside "if (fd != op->fd)" and thus there's no way
> fd==op->fd can be true. So the change in this line is just spurious.
> There's then just one place where dup3_or_set_cloexec is called (the
> previous case), and it doesn't make sense to have a function; rather
> the conditional should just be inline in that case.
>
> Rich
>

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

[-- Attachment #2: 0001-New-posix_spawn_file_actions_adddup2-semantics.patch --]
[-- Type: text/x-patch, Size: 1171 bytes --]

From 8c9a5708e7d7fde18f5b72d064f449156d5bfb3b Mon Sep 17 00:00:00 2001
From: Petr Skocik <gmail@pskocik.com>
Date: Fri, 20 Oct 2017 10:50:24 +0200
Subject: [PATCH] New posix_spawn_file_actions_adddup2 semantics

Clear FD_CLOEXEC on the target fd in dup2 file actions where target fd == src_fd.
Ref: http://austingroupbugs.net/view.php?id=411
---
 src/process/posix_spawn.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/process/posix_spawn.c b/src/process/posix_spawn.c
index ea5d2998..cd355f17 100644
--- a/src/process/posix_spawn.c
+++ b/src/process/posix_spawn.c
@@ -109,8 +109,15 @@ static int child(void *args_vp)
 				__syscall(SYS_close, op->fd);
 				break;
 			case FDOP_DUP2:
-				if ((ret=__sys_dup2(op->srcfd, op->fd))<0)
-					goto fail;
+				if (op->srcfd != op->fd){
+					if ((ret=__sys_dup2(op->srcfd, op->fd))<0)
+						goto fail;
+				}else{
+					if ((ret=__syscall(SYS_fcntl, op->fd, F_GETFD))<0)
+						goto fail;
+					if ((ret=__syscall(SYS_fcntl, op->fd, F_SETFD, ret & ~FD_CLOEXEC))<0)
+						goto fail;
+				}
 				break;
 			case FDOP_OPEN:
 				fd = __sys_open(op->path, op->oflag, op->mode);
-- 
2.14.2