* musl 1.1.17 released @ 2017-10-19 20:13 Rich Felker 2017-10-20 0:11 ` Petr Skocik 0 siblings, 1 reply; 5+ messages in thread From: Rich Felker @ 2017-10-19 20:13 UTC (permalink / raw) To: musl *** 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) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: musl 1.1.17 released 2017-10-19 20:13 musl 1.1.17 released Rich Felker @ 2017-10-20 0:11 ` Petr Skocik 2017-10-20 1:17 ` posix_spawn topics [was: Re: [musl] musl 1.1.17 released] Rich Felker 0 siblings, 1 reply; 5+ messages in thread From: Petr Skocik @ 2017-10-20 0:11 UTC (permalink / raw) To: musl [-- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* posix_spawn topics [was: Re: [musl] musl 1.1.17 released] 2017-10-20 0:11 ` Petr Skocik @ 2017-10-20 1:17 ` Rich Felker 2017-10-20 9:02 ` Petr Skocik 0 siblings, 1 reply; 5+ messages in thread From: Rich Felker @ 2017-10-20 1:17 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: posix_spawn topics [was: Re: [musl] musl 1.1.17 released] 2017-10-20 1:17 ` posix_spawn topics [was: Re: [musl] musl 1.1.17 released] Rich Felker @ 2017-10-20 9:02 ` Petr Skocik 2017-10-20 9:09 ` Petr Skočík 0 siblings, 1 reply; 5+ messages in thread From: Petr Skocik @ 2017-10-20 9:02 UTC (permalink / raw) To: musl [-- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: posix_spawn topics [was: Re: [musl] musl 1.1.17 released] 2017-10-20 9:02 ` Petr Skocik @ 2017-10-20 9:09 ` Petr Skočík 0 siblings, 0 replies; 5+ messages in thread From: Petr Skočík @ 2017-10-20 9:09 UTC (permalink / raw) To: musl [-- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-10-20 9:09 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-19 20:13 musl 1.1.17 released Rich Felker 2017-10-20 0:11 ` Petr Skocik 2017-10-20 1:17 ` posix_spawn topics [was: Re: [musl] musl 1.1.17 released] Rich Felker 2017-10-20 9:02 ` Petr Skocik 2017-10-20 9:09 ` Petr Skočík
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).