Message ID | 1494876985-21990-2-git-send-email-adhemerval.zanella@linaro.org |
---|---|
State | Accepted |
Commit | ccfb2964726512f6669fea99a43afa714e2e6a80 |
Headers | show |
Series | [1/3] posix: Adapt tst-spawn{2,3} to use libsupport. | expand |
Since this is not used in any port currently (since hurd uses its own implementation which I think suffers from the same issue of the current posix one) and this patch just align the posix one to Linux recent fixes I will commit this shortly if no one objects. On 15/05/2017 16:36, Adhemerval Zanella wrote: > This patch improves the default posix implementation of posix_spawn{p} > and align with Linux one. The main idea is to try shared common > implementation bits with Linux and avoid code duplication, fix some > issues already fixed in Linux code, and deprecated vfork internal > usage (source of various bug reports). In a short: > > - It moves POSIX_SPAWN_USEVFORK usage and sets it a no-op. Since > the process that actually spawn the new process do not share > memory with parent (with vfork), it fixes BZ#14750 for this > implementation. > > - It uses a pipe to correctly obtain the return upon failure > of execution (BZ#18433). > > - It correctly enable/disable asynchronous cancellation (checked > on ptl/tst-exec5.c). > > - It correctly disable/enable signal handling. > > Using this version instead of Linux shows only one regression, > posix/tst-spawn3, because of pipe2 usage which increase total > number of file descriptor. > > * sysdeps/posix/spawni.c (__spawni_child): New function. > (__spawni): Rename to __spawnix. > --- > ChangeLog | 3 + > sysdeps/posix/spawni.c | 362 ++++++++++++++++++++++++------------------------- > 2 files changed, 182 insertions(+), 183 deletions(-) > > diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c > index 9cad25c..e096a42 100644 > --- a/sysdeps/posix/spawni.c > +++ b/sysdeps/posix/spawni.c > @@ -16,20 +16,23 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > -#include <errno.h> > +#include <spawn.h> > +#include <assert.h> > #include <fcntl.h> > #include <paths.h> > -#include <spawn.h> > -#include <stdbool.h> > -#include <stdlib.h> > #include <string.h> > -#include <unistd.h> > -#include <signal.h> > #include <sys/resource.h> > -#include "spawn_int.h" > +#include <sys/wait.h> > +#include <sys/param.h> > +#include <sys/mman.h> > #include <not-cancel.h> > #include <local-setxid.h> > #include <shlib-compat.h> > +#include <nptl/pthreadP.h> > +#include <dl-sysdep.h> > +#include <libc-pointer-arith.h> > +#include <ldsodefs.h> > +#include "spawn_int.h" > > > /* The Unix standard contains a long explanation of the way to signal > @@ -39,94 +42,59 @@ > normal program exit with the exit code 127. */ > #define SPAWN_ERROR 127 > > - > -/* The file is accessible but it is not an executable file. Invoke > - the shell to interpret it as a script. */ > -static void > -internal_function > -script_execute (const char *file, char *const argv[], char *const envp[]) > +struct posix_spawn_args > { > - /* Count the arguments. */ > - int argc = 0; > - while (argv[argc++]) > - ; > - > - /* Construct an argument list for the shell. */ > - { > - char *new_argv[argc + 1]; > - new_argv[0] = (char *) _PATH_BSHELL; > - new_argv[1] = (char *) file; > - while (argc > 1) > - { > - new_argv[argc] = argv[argc - 1]; > - --argc; > - } > - > - /* Execute the shell. */ > - __execve (new_argv[0], new_argv, envp); > - } > -} > - > -static inline void > -maybe_script_execute (const char *file, char *const argv[], char *const envp[], > - int xflags) > + sigset_t oldmask; > + const char *file; > + int (*exec) (const char *, char *const *, char *const *); > + const posix_spawn_file_actions_t *fa; > + const posix_spawnattr_t *restrict attr; > + char *const *argv; > + ptrdiff_t argc; > + char *const *envp; > + int xflags; > + int pipe[2]; > +}; > + > +/* Older version requires that shell script without shebang definition > + to be called explicitly using /bin/sh (_PATH_BSHELL). */ > +static void > +maybe_script_execute (struct posix_spawn_args *args) > { > if (SHLIB_COMPAT (libc, GLIBC_2_2, GLIBC_2_15) > - && (xflags & SPAWN_XFLAGS_TRY_SHELL) > - && errno == ENOEXEC) > - script_execute (file, argv, envp); > -} > - > -/* Spawn a new process executing PATH with the attributes describes in *ATTRP. > - Before running the process perform the actions described in FILE-ACTIONS. */ > -int > -__spawni (pid_t *pid, const char *file, > - const posix_spawn_file_actions_t *file_actions, > - const posix_spawnattr_t *attrp, char *const argv[], > - char *const envp[], int xflags) > -{ > - pid_t new_pid; > - char *path, *p, *name; > - size_t len; > - size_t pathlen; > - > - /* Do this once. */ > - short int flags = attrp == NULL ? 0 : attrp->__flags; > - > - /* Generate the new process. */ > - if ((flags & POSIX_SPAWN_USEVFORK) != 0 > - /* If no major work is done, allow using vfork. Note that we > - might perform the path searching. But this would be done by > - a call to execvp(), too, and such a call must be OK according > - to POSIX. */ > - || ((flags & (POSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEF > - | POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER > - | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS > - | POSIX_SPAWN_SETSID)) == 0 > - && file_actions == NULL)) > - new_pid = __vfork (); > - else > - new_pid = __fork (); > - > - if (new_pid != 0) > + && (args->xflags & SPAWN_XFLAGS_TRY_SHELL) && errno == ENOEXEC) > { > - if (new_pid < 0) > - return errno; > - > - /* The call was successful. Store the PID if necessary. */ > - if (pid != NULL) > - *pid = new_pid; > + char *const *argv = args->argv; > + ptrdiff_t argc = args->argc; > + > + /* Construct an argument list for the shell. */ > + char *new_argv[argc + 1]; > + new_argv[0] = (char *) _PATH_BSHELL; > + new_argv[1] = (char *) args->file; > + if (argc > 1) > + memcpy (new_argv + 2, argv + 1, argc * sizeof(char *)); > + else > + new_argv[2] = NULL; > > - return 0; > + /* Execute the shell. */ > + args->exec (new_argv[0], new_argv, args->envp); > } > +} > + > +/* Function used in the clone call to setup the signals mask, posix_spawn > + attributes, and file actions. */ > +static int > +__spawni_child (void *arguments) > +{ > + struct posix_spawn_args *args = arguments; > + const posix_spawnattr_t *restrict attr = args->attr; > + const posix_spawn_file_actions_t *file_actions = args->fa; > + int ret; > > - /* Set signal mask. */ > - if ((flags & POSIX_SPAWN_SETSIGMASK) != 0 > - && __sigprocmask (SIG_SETMASK, &attrp->__ss, NULL) != 0) > - _exit (SPAWN_ERROR); > + __close (args->pipe[0]); > > /* Set signal default action. */ > - if ((flags & POSIX_SPAWN_SETSIGDEF) != 0) > + if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) != 0) > { > /* We have to iterate over all signals. This could possibly be > done better but it requires system specific solutions since > @@ -139,41 +107,41 @@ __spawni (pid_t *pid, const char *file, > sa.sa_handler = SIG_DFL; > > for (sig = 1; sig <= _NSIG; ++sig) > - if (__sigismember (&attrp->__sd, sig) != 0 > + if (__sigismember (&attr->__sd, sig) != 0 > && __sigaction (sig, &sa, NULL) != 0) > - _exit (SPAWN_ERROR); > - > + goto fail; > } > > #ifdef _POSIX_PRIORITY_SCHEDULING > /* Set the scheduling algorithm and parameters. */ > - if ((flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER)) > + if ((attr->__flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER)) > == POSIX_SPAWN_SETSCHEDPARAM) > { > - if (__sched_setparam (0, &attrp->__sp) == -1) > - _exit (SPAWN_ERROR); > + if ((ret = __sched_setparam (0, &attr->__sp)) == -1) > + goto fail; > } > - else if ((flags & POSIX_SPAWN_SETSCHEDULER) != 0) > + else if ((attr->__flags & POSIX_SPAWN_SETSCHEDULER) != 0) > { > - if (__sched_setscheduler (0, attrp->__policy, &attrp->__sp) == -1) > - _exit (SPAWN_ERROR); > + if ((ret = __sched_setscheduler (0, attr->__policy, &attr->__sp) == -1)) > + goto fail; > } > #endif > > - if ((flags & POSIX_SPAWN_SETSID) != 0 > - && __setsid () < 0) > - _exit (SPAWN_ERROR); > + /* Set the process session ID. */ > + if ((attr->__flags & POSIX_SPAWN_SETSID) != 0 > + && (ret = __setsid ()) < 0) > + goto fail; > > /* Set the process group ID. */ > - if ((flags & POSIX_SPAWN_SETPGROUP) != 0 > - && __setpgid (0, attrp->__pgrp) != 0) > - _exit (SPAWN_ERROR); > + if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0 > + && (ret =__setpgid (0, attr->__pgrp)) != 0) > + goto fail; > > /* Set the effective user and group IDs. */ > - if ((flags & POSIX_SPAWN_RESETIDS) != 0 > - && (local_seteuid (__getuid ()) != 0 > - || local_setegid (__getgid ()) != 0)) > - _exit (SPAWN_ERROR); > + if ((attr->__flags & POSIX_SPAWN_RESETIDS) != 0 > + && ((ret = local_seteuid (__getuid ())) != 0 > + || (ret = local_setegid (__getgid ())) != 0)) > + goto fail; > > /* Execute the file actions. */ > if (file_actions != NULL) > @@ -191,7 +159,7 @@ __spawni (pid_t *pid, const char *file, > case spawn_do_close: > if (close_not_cancel (action->action.close_action.fd) != 0) > { > - if (! have_fdlimit) > + if (have_fdlimit == 0) > { > __getrlimit64 (RLIMIT_NOFILE, &fdlimit); > have_fdlimit = true; > @@ -200,120 +168,148 @@ __spawni (pid_t *pid, const char *file, > /* Only signal errors for file descriptors out of range. */ > if (action->action.close_action.fd < 0 > || action->action.close_action.fd >= fdlimit.rlim_cur) > - /* Signal the error. */ > - _exit (SPAWN_ERROR); > + { > + ret = -1; > + goto fail; > + } > } > break; > > case spawn_do_open: > { > + /* POSIX states that if fildes was already an open file descriptor, > + it shall be closed before the new file is opened. This avoid > + pontential issues when posix_spawn plus addopen action is called > + with the process already at maximum number of file descriptor > + opened and also for multiple actions on single-open special > + paths (like /dev/watchdog). */ > + close_not_cancel (action->action.open_action.fd); > + > int new_fd = open_not_cancel (action->action.open_action.path, > action->action.open_action.oflag > | O_LARGEFILE, > action->action.open_action.mode); > > - if (new_fd == -1) > - /* The `open' call failed. */ > - _exit (SPAWN_ERROR); > + if ((ret = new_fd) == -1) > + goto fail; > > /* Make sure the desired file descriptor is used. */ > if (new_fd != action->action.open_action.fd) > { > - if (__dup2 (new_fd, action->action.open_action.fd) > + if ((ret = __dup2 (new_fd, action->action.open_action.fd)) > != action->action.open_action.fd) > - /* The `dup2' call failed. */ > - _exit (SPAWN_ERROR); > + goto fail; > > - if (close_not_cancel (new_fd) != 0) > - /* The `close' call failed. */ > - _exit (SPAWN_ERROR); > + if ((ret = close_not_cancel (new_fd) != 0)) > + goto fail; > } > } > break; > > case spawn_do_dup2: > - if (__dup2 (action->action.dup2_action.fd, > - action->action.dup2_action.newfd) > + if ((ret = __dup2 (action->action.dup2_action.fd, > + action->action.dup2_action.newfd)) > != action->action.dup2_action.newfd) > - /* The `dup2' call failed. */ > - _exit (SPAWN_ERROR); > + goto fail; > break; > } > } > } > > - if ((xflags & SPAWN_XFLAGS_USE_PATH) == 0 || strchr (file, '/') != NULL) > - { > - /* The FILE parameter is actually a path. */ > - __execve (file, argv, envp); > + /* Set the initial signal mask of the child if POSIX_SPAWN_SETSIGMASK > + is set, otherwise restore the previous one. */ > + __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK) > + ? &attr->__ss : &args->oldmask, 0); > > - maybe_script_execute (file, argv, envp, xflags); > + args->exec (args->file, args->argv, args->envp); > > - /* Oh, oh. `execve' returns. This is bad. */ > - _exit (SPAWN_ERROR); > - } > + /* This is compatibility function required to enable posix_spawn run > + script without shebang definition for older posix_spawn versions > + (2.15). */ > + maybe_script_execute (args); > > - /* We have to search for FILE on the path. */ > - path = getenv ("PATH"); > - if (path == NULL) > - { > - /* There is no `PATH' in the environment. > - The default search path is the current directory > - followed by the path `confstr' returns for `_CS_PATH'. */ > - len = confstr (_CS_PATH, (char *) NULL, 0); > - path = (char *) __alloca (1 + len); > - path[0] = ':'; > - (void) confstr (_CS_PATH, path + 1, len); > - } > + ret = -errno; > > - len = strlen (file) + 1; > - pathlen = strlen (path); > - name = __alloca (pathlen + len + 1); > - /* Copy the file name at the top. */ > - name = (char *) memcpy (name + pathlen + 1, file, len); > - /* And add the slash. */ > - *--name = '/'; > +fail: > + /* Since sizeof errno < PIPE_BUF, the write is atomic. */ > + ret = -ret; > + if (ret) > + while (write_not_cancel (args->pipe[1], &ret, sizeof (ret)) < 0); > > - p = path; > - do > - { > - char *startp; > + _exit (SPAWN_ERROR); > +} > > - path = p; > - p = __strchrnul (path, ':'); > +/* Spawn a new process executing PATH with the attributes describes in *ATTRP. > + Before running the process perform the actions described in FILE-ACTIONS. */ > +int > +__spawnix (pid_t *pid, const char *file, > + const posix_spawn_file_actions_t *file_actions, > + const posix_spawnattr_t *attrp, char *const argv[], > + char *const envp[], int xflags, > + int (*exec) (const char *, char *const *, char *const *)) > +{ > + struct posix_spawn_args args; > + int ec; > > - if (p == path) > - /* Two adjacent colons, or a colon at the beginning or the end > - of `PATH' means to search the current directory. */ > - startp = name + 1; > - else > - startp = (char *) memcpy (name - (p - path), path, p - path); > + if (__pipe2 (args.pipe, O_CLOEXEC)) > + return errno; > > - /* Try to execute this name. If it works, execv will not return. */ > - __execve (startp, argv, envp); > + /* Disable asynchronous cancellation. */ > + int state; > + __libc_ptf_call (__pthread_setcancelstate, > + (PTHREAD_CANCEL_DISABLE, &state), 0); > > - maybe_script_execute (startp, argv, envp, xflags); > + ptrdiff_t argc = 0; > + ptrdiff_t limit = INT_MAX - 1; > + while (argv[argc++] != NULL) > + if (argc == limit) > + { > + errno = E2BIG; > + return errno; > + } > > - switch (errno) > - { > - case EACCES: > - case ENOENT: > - case ESTALE: > - case ENOTDIR: > - /* Those errors indicate the file is missing or not executable > - by us, in which case we want to just try the next path > - directory. */ > - break; > - > - default: > - /* Some other error means we found an executable file, but > - something went wrong executing it; return the error to our > - caller. */ > - _exit (SPAWN_ERROR); > - } > + args.file = file; > + args.exec = exec; > + args.fa = file_actions; > + args.attr = attrp ? attrp : &(const posix_spawnattr_t) { 0 }; > + args.argv = argv; > + args.argc = argc; > + args.envp = envp; > + args.xflags = xflags; > + > + /* Generate the new process. */ > + pid_t new_pid = __fork (); > + > + if (new_pid == 0) > + __spawni_child (&args); > + else if (new_pid > 0) > + { > + __close (args.pipe[1]); > + > + if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec) > + ec = 0; > + else > + __waitpid (new_pid, &(int) { 0 }, 0); > } > - while (*p++ != '\0'); > + else > + ec = -new_pid; > > - /* Return with an error. */ > - _exit (SPAWN_ERROR); > + __close (args.pipe[0]); > + > + if ((ec == 0) && (pid != NULL)) > + *pid = new_pid; > + > + __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0); > + > + return ec; > +} > + > +int > +__spawni (pid_t * pid, const char *file, > + const posix_spawn_file_actions_t * acts, > + const posix_spawnattr_t * attrp, char *const argv[], > + char *const envp[], int xflags) > +{ > + return __spawnix (pid, file, acts, attrp, argv, envp, xflags, > + xflags & SPAWN_XFLAGS_USE_PATH ? __execvpe : __execve); > } >
On Mai 15 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > + if ((ret = close_not_cancel (new_fd) != 0)) This assigns the wrong value to ret. > +fail: > + /* Since sizeof errno < PIPE_BUF, the write is atomic. */ > + ret = -ret; posix_spwan is supposed to return errno, but none of the arms going here set ret appropriately. > + /* Generate the new process. */ > + pid_t new_pid = __fork (); > + > + if (new_pid == 0) > + __spawni_child (&args); > + else if (new_pid > 0) > + { > + __close (args.pipe[1]); > + > + if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec) > + ec = 0; > + else > + __waitpid (new_pid, &(int) { 0 }, 0); > } > - while (*p++ != '\0'); > + else > + ec = -new_pid; new_pid isn't an errno either. Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
On 29/06/2017 11:22, Andreas Schwab wrote: > On Mai 15 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > >> + if ((ret = close_not_cancel (new_fd) != 0)) > > This assigns the wrong value to ret. Ugh, this should not be here... > >> +fail: >> + /* Since sizeof errno < PIPE_BUF, the write is atomic. */ >> + ret = -ret; > > posix_spwan is supposed to return errno, but none of the arms going here > set ret appropriately. Fixed below. > >> + /* Generate the new process. */ >> + pid_t new_pid = __fork (); >> + >> + if (new_pid == 0) >> + __spawni_child (&args); >> + else if (new_pid > 0) >> + { >> + __close (args.pipe[1]); >> + >> + if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec) >> + ec = 0; >> + else >> + __waitpid (new_pid, &(int) { 0 }, 0); >> } >> - while (*p++ != '\0'); >> + else >> + ec = -new_pid; > > new_pid isn't an errno either. Since posix_spawn is not support to set errno in case of failure we will need to save/restore for fork call. And this should be fixed on Linux implementation as well since clone will also clobber errno in case of an error (I will send a fix). Ideally I think the exported clone should not set errno and just return errno as a negative number. > > Andreas. > I intended to push this change:diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c index 1979830..93767a2 100644 --- a/sysdeps/posix/spawni.c +++ b/sysdeps/posix/spawni.c @@ -117,30 +117,30 @@ __spawni_child (void *arguments) if ((attr->__flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER)) == POSIX_SPAWN_SETSCHEDPARAM) { - if ((ret = __sched_setparam (0, &attr->__sp)) == -1) + if (__sched_setparam (0, &attr->__sp) == -1) goto fail; } else if ((attr->__flags & POSIX_SPAWN_SETSCHEDULER) != 0) { - if ((ret = __sched_setscheduler (0, attr->__policy, &attr->__sp) == -1)) + if (__sched_setscheduler (0, attr->__policy, &attr->__sp) == -1) goto fail; } #endif /* Set the process session ID. */ if ((attr->__flags & POSIX_SPAWN_SETSID) != 0 - && (ret = __setsid ()) < 0) + && __setsid () < 0) goto fail; /* Set the process group ID. */ if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0 - && (ret =__setpgid (0, attr->__pgrp)) != 0) + && __setpgid (0, attr->__pgrp) != 0) goto fail; /* Set the effective user and group IDs. */ if ((attr->__flags & POSIX_SPAWN_RESETIDS) != 0 - && ((ret = local_seteuid (__getuid ())) != 0 - || (ret = local_setegid (__getgid ())) != 0)) + && (local_seteuid (__getuid ()) != 0 + || local_setegid (__getgid ())) != 0) goto fail; /* Execute the file actions. */ @@ -168,10 +168,7 @@ __spawni_child (void *arguments) /* Only signal errors for file descriptors out of range. */ if (action->action.close_action.fd < 0 || action->action.close_action.fd >= fdlimit.rlim_cur) - { - ret = -1; - goto fail; - } + goto fail; } break; @@ -190,25 +187,25 @@ __spawni_child (void *arguments) | O_LARGEFILE, action->action.open_action.mode); - if ((ret = new_fd) == -1) + if (new_fd == -1) goto fail; /* Make sure the desired file descriptor is used. */ if (new_fd != action->action.open_action.fd) { - if ((ret = __dup2 (new_fd, action->action.open_action.fd)) + if (__dup2 (new_fd, action->action.open_action.fd) != action->action.open_action.fd) goto fail; - if ((ret = close_not_cancel (new_fd) != 0)) + if (close_not_cancel (new_fd) != 0) goto fail; } } break; case spawn_do_dup2: - if ((ret = __dup2 (action->action.dup2_action.fd, - action->action.dup2_action.newfd)) + if (__dup2 (action->action.dup2_action.fd, + action->action.dup2_action.newfd) != action->action.dup2_action.newfd) goto fail; break; @@ -228,12 +225,15 @@ __spawni_child (void *arguments) (2.15). */ maybe_script_execute (args); - ret = -errno; - fail: - /* Since sizeof errno < PIPE_BUF, the write is atomic. */ - ret = -ret; + /* errno should have an appropriate non-zero value; otherwise, + there's a bug in glibc or the kernel. For lack of an error code + (EINTERNALBUG) describing that, use ECHILD. Another option would + be to set args->err to some negative sentinel and have the parent + abort(), but that seems needlessly harsh. */ + ret = errno ? : ECHILD; if (ret) + /* Since sizeof errno < PIPE_BUF, the write is atomic. */ while (write_not_cancel (args->pipe[1], &ret, sizeof (ret)) < 0); _exit (SPAWN_ERROR); @@ -278,6 +278,7 @@ __spawnix (pid_t *pid, const char *file, args.xflags = xflags; /* Generate the new process. */ + int old_errno = errno; pid_t new_pid = __fork (); if (new_pid == 0) @@ -292,7 +293,8 @@ __spawnix (pid_t *pid, const char *file, __waitpid (new_pid, &(int) { 0 }, 0); } else - ec = -new_pid; + ec = errno; + errno = old_errno; __close (args.pipe[0]);
On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> Since posix_spawn is not support to set errno in case of failure
Is it?
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
On 29/06/2017 12:20, Andreas Schwab wrote: > On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > >> Since posix_spawn is not support to set errno in case of failure > > Is it? My understanding of posix_spawn return value [1] states that the error number should be returned as the function return value, not on errno. [1] http://pubs.opengroup.org/onlinepubs/9699919799/
On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > On 29/06/2017 12:20, Andreas Schwab wrote: >> On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >> >>> Since posix_spawn is not support to set errno in case of failure >> >> Is it? > > My understanding of posix_spawn return value [1] states that the error number > should be returned as the function return value, not on errno. But that doesn't forbid spurious writes to errno, does it? Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
On 03/07/2017 04:08, Andreas Schwab wrote: > On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > >> On 29/06/2017 12:20, Andreas Schwab wrote: >>> On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >>> >>>> Since posix_spawn is not support to set errno in case of failure >>> >>> Is it? >> >> My understanding of posix_spawn return value [1] states that the error number >> should be returned as the function return value, not on errno. > > But that doesn't forbid spurious writes to errno, does it? My understanding is POSIX is not strictly regarding spurious errno writes, but on general information [1] it has the rationale: "In order to avoid this problem altogether for new functions, these functions avoid using errno and, instead, return the error number directly as the function return value; a return value of zero indicates that no error was detected." Also, making 'clone' not setting errno should also a QoI IMHO: this is an Linux extension and making is avoid touching memory that might incur in some issue should be less prone to errors. [1] http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html
On Jul 03 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > My understanding is POSIX is not strictly regarding spurious errno writes, > but on general information [1] it has the rationale: > > "In order to avoid this problem altogether for new functions, these > functions avoid using errno and, instead, return the error number directly > as the function return value; a return value of zero indicates that no > error was detected." It also notes (in section Signal Actions): "Note in particular that even the "safe'' functions may modify the global variable errno" Andreas. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
On 03/07/2017 10:02, Andreas Schwab wrote: > On Jul 03 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > >> My understanding is POSIX is not strictly regarding spurious errno writes, >> but on general information [1] it has the rationale: >> >> "In order to avoid this problem altogether for new functions, these >> functions avoid using errno and, instead, return the error number directly >> as the function return value; a return value of zero indicates that no >> error was detected." > > It also notes (in section Signal Actions): > > "Note in particular that even the "safe'' functions may modify the global > variable errno" So it is more an implementation detail and/or QoI to whether modify errno on such functions and see no impeding reason of doing it on posix_spawn. The only rationale I am not sure is if its worth to remove clone wrapper error path setting errno.
diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c index 9cad25c..e096a42 100644 --- a/sysdeps/posix/spawni.c +++ b/sysdeps/posix/spawni.c @@ -16,20 +16,23 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ -#include <errno.h> +#include <spawn.h> +#include <assert.h> #include <fcntl.h> #include <paths.h> -#include <spawn.h> -#include <stdbool.h> -#include <stdlib.h> #include <string.h> -#include <unistd.h> -#include <signal.h> #include <sys/resource.h> -#include "spawn_int.h" +#include <sys/wait.h> +#include <sys/param.h> +#include <sys/mman.h> #include <not-cancel.h> #include <local-setxid.h> #include <shlib-compat.h> +#include <nptl/pthreadP.h> +#include <dl-sysdep.h> +#include <libc-pointer-arith.h> +#include <ldsodefs.h> +#include "spawn_int.h" /* The Unix standard contains a long explanation of the way to signal @@ -39,94 +42,59 @@ normal program exit with the exit code 127. */ #define SPAWN_ERROR 127 - -/* The file is accessible but it is not an executable file. Invoke - the shell to interpret it as a script. */ -static void -internal_function -script_execute (const char *file, char *const argv[], char *const envp[]) +struct posix_spawn_args { - /* Count the arguments. */ - int argc = 0; - while (argv[argc++]) - ; - - /* Construct an argument list for the shell. */ - { - char *new_argv[argc + 1]; - new_argv[0] = (char *) _PATH_BSHELL; - new_argv[1] = (char *) file; - while (argc > 1) - { - new_argv[argc] = argv[argc - 1]; - --argc; - } - - /* Execute the shell. */ - __execve (new_argv[0], new_argv, envp); - } -} - -static inline void -maybe_script_execute (const char *file, char *const argv[], char *const envp[], - int xflags) + sigset_t oldmask; + const char *file; + int (*exec) (const char *, char *const *, char *const *); + const posix_spawn_file_actions_t *fa; + const posix_spawnattr_t *restrict attr; + char *const *argv; + ptrdiff_t argc; + char *const *envp; + int xflags; + int pipe[2]; +}; + +/* Older version requires that shell script without shebang definition + to be called explicitly using /bin/sh (_PATH_BSHELL). */ +static void +maybe_script_execute (struct posix_spawn_args *args) { if (SHLIB_COMPAT (libc, GLIBC_2_2, GLIBC_2_15) - && (xflags & SPAWN_XFLAGS_TRY_SHELL) - && errno == ENOEXEC) - script_execute (file, argv, envp); -} - -/* Spawn a new process executing PATH with the attributes describes in *ATTRP. - Before running the process perform the actions described in FILE-ACTIONS. */ -int -__spawni (pid_t *pid, const char *file, - const posix_spawn_file_actions_t *file_actions, - const posix_spawnattr_t *attrp, char *const argv[], - char *const envp[], int xflags) -{ - pid_t new_pid; - char *path, *p, *name; - size_t len; - size_t pathlen; - - /* Do this once. */ - short int flags = attrp == NULL ? 0 : attrp->__flags; - - /* Generate the new process. */ - if ((flags & POSIX_SPAWN_USEVFORK) != 0 - /* If no major work is done, allow using vfork. Note that we - might perform the path searching. But this would be done by - a call to execvp(), too, and such a call must be OK according - to POSIX. */ - || ((flags & (POSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEF - | POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER - | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS - | POSIX_SPAWN_SETSID)) == 0 - && file_actions == NULL)) - new_pid = __vfork (); - else - new_pid = __fork (); - - if (new_pid != 0) + && (args->xflags & SPAWN_XFLAGS_TRY_SHELL) && errno == ENOEXEC) { - if (new_pid < 0) - return errno; - - /* The call was successful. Store the PID if necessary. */ - if (pid != NULL) - *pid = new_pid; + char *const *argv = args->argv; + ptrdiff_t argc = args->argc; + + /* Construct an argument list for the shell. */ + char *new_argv[argc + 1]; + new_argv[0] = (char *) _PATH_BSHELL; + new_argv[1] = (char *) args->file; + if (argc > 1) + memcpy (new_argv + 2, argv + 1, argc * sizeof(char *)); + else + new_argv[2] = NULL; - return 0; + /* Execute the shell. */ + args->exec (new_argv[0], new_argv, args->envp); } +} + +/* Function used in the clone call to setup the signals mask, posix_spawn + attributes, and file actions. */ +static int +__spawni_child (void *arguments) +{ + struct posix_spawn_args *args = arguments; + const posix_spawnattr_t *restrict attr = args->attr; + const posix_spawn_file_actions_t *file_actions = args->fa; + int ret; - /* Set signal mask. */ - if ((flags & POSIX_SPAWN_SETSIGMASK) != 0 - && __sigprocmask (SIG_SETMASK, &attrp->__ss, NULL) != 0) - _exit (SPAWN_ERROR); + __close (args->pipe[0]); /* Set signal default action. */ - if ((flags & POSIX_SPAWN_SETSIGDEF) != 0) + if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) != 0) { /* We have to iterate over all signals. This could possibly be done better but it requires system specific solutions since @@ -139,41 +107,41 @@ __spawni (pid_t *pid, const char *file, sa.sa_handler = SIG_DFL; for (sig = 1; sig <= _NSIG; ++sig) - if (__sigismember (&attrp->__sd, sig) != 0 + if (__sigismember (&attr->__sd, sig) != 0 && __sigaction (sig, &sa, NULL) != 0) - _exit (SPAWN_ERROR); - + goto fail; } #ifdef _POSIX_PRIORITY_SCHEDULING /* Set the scheduling algorithm and parameters. */ - if ((flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER)) + if ((attr->__flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER)) == POSIX_SPAWN_SETSCHEDPARAM) { - if (__sched_setparam (0, &attrp->__sp) == -1) - _exit (SPAWN_ERROR); + if ((ret = __sched_setparam (0, &attr->__sp)) == -1) + goto fail; } - else if ((flags & POSIX_SPAWN_SETSCHEDULER) != 0) + else if ((attr->__flags & POSIX_SPAWN_SETSCHEDULER) != 0) { - if (__sched_setscheduler (0, attrp->__policy, &attrp->__sp) == -1) - _exit (SPAWN_ERROR); + if ((ret = __sched_setscheduler (0, attr->__policy, &attr->__sp) == -1)) + goto fail; } #endif - if ((flags & POSIX_SPAWN_SETSID) != 0 - && __setsid () < 0) - _exit (SPAWN_ERROR); + /* Set the process session ID. */ + if ((attr->__flags & POSIX_SPAWN_SETSID) != 0 + && (ret = __setsid ()) < 0) + goto fail; /* Set the process group ID. */ - if ((flags & POSIX_SPAWN_SETPGROUP) != 0 - && __setpgid (0, attrp->__pgrp) != 0) - _exit (SPAWN_ERROR); + if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0 + && (ret =__setpgid (0, attr->__pgrp)) != 0) + goto fail; /* Set the effective user and group IDs. */ - if ((flags & POSIX_SPAWN_RESETIDS) != 0 - && (local_seteuid (__getuid ()) != 0 - || local_setegid (__getgid ()) != 0)) - _exit (SPAWN_ERROR); + if ((attr->__flags & POSIX_SPAWN_RESETIDS) != 0 + && ((ret = local_seteuid (__getuid ())) != 0 + || (ret = local_setegid (__getgid ())) != 0)) + goto fail; /* Execute the file actions. */ if (file_actions != NULL) @@ -191,7 +159,7 @@ __spawni (pid_t *pid, const char *file, case spawn_do_close: if (close_not_cancel (action->action.close_action.fd) != 0) { - if (! have_fdlimit) + if (have_fdlimit == 0) { __getrlimit64 (RLIMIT_NOFILE, &fdlimit); have_fdlimit = true; @@ -200,120 +168,148 @@ __spawni (pid_t *pid, const char *file, /* Only signal errors for file descriptors out of range. */ if (action->action.close_action.fd < 0 || action->action.close_action.fd >= fdlimit.rlim_cur) - /* Signal the error. */ - _exit (SPAWN_ERROR); + { + ret = -1; + goto fail; + } } break; case spawn_do_open: { + /* POSIX states that if fildes was already an open file descriptor, + it shall be closed before the new file is opened. This avoid + pontential issues when posix_spawn plus addopen action is called + with the process already at maximum number of file descriptor + opened and also for multiple actions on single-open special + paths (like /dev/watchdog). */ + close_not_cancel (action->action.open_action.fd); + int new_fd = open_not_cancel (action->action.open_action.path, action->action.open_action.oflag | O_LARGEFILE, action->action.open_action.mode); - if (new_fd == -1) - /* The `open' call failed. */ - _exit (SPAWN_ERROR); + if ((ret = new_fd) == -1) + goto fail; /* Make sure the desired file descriptor is used. */ if (new_fd != action->action.open_action.fd) { - if (__dup2 (new_fd, action->action.open_action.fd) + if ((ret = __dup2 (new_fd, action->action.open_action.fd)) != action->action.open_action.fd) - /* The `dup2' call failed. */ - _exit (SPAWN_ERROR); + goto fail; - if (close_not_cancel (new_fd) != 0) - /* The `close' call failed. */ - _exit (SPAWN_ERROR); + if ((ret = close_not_cancel (new_fd) != 0)) + goto fail; } } break; case spawn_do_dup2: - if (__dup2 (action->action.dup2_action.fd, - action->action.dup2_action.newfd) + if ((ret = __dup2 (action->action.dup2_action.fd, + action->action.dup2_action.newfd)) != action->action.dup2_action.newfd) - /* The `dup2' call failed. */ - _exit (SPAWN_ERROR); + goto fail; break; } } } - if ((xflags & SPAWN_XFLAGS_USE_PATH) == 0 || strchr (file, '/') != NULL) - { - /* The FILE parameter is actually a path. */ - __execve (file, argv, envp); + /* Set the initial signal mask of the child if POSIX_SPAWN_SETSIGMASK + is set, otherwise restore the previous one. */ + __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK) + ? &attr->__ss : &args->oldmask, 0); - maybe_script_execute (file, argv, envp, xflags); + args->exec (args->file, args->argv, args->envp); - /* Oh, oh. `execve' returns. This is bad. */ - _exit (SPAWN_ERROR); - } + /* This is compatibility function required to enable posix_spawn run + script without shebang definition for older posix_spawn versions + (2.15). */ + maybe_script_execute (args); - /* We have to search for FILE on the path. */ - path = getenv ("PATH"); - if (path == NULL) - { - /* There is no `PATH' in the environment. - The default search path is the current directory - followed by the path `confstr' returns for `_CS_PATH'. */ - len = confstr (_CS_PATH, (char *) NULL, 0); - path = (char *) __alloca (1 + len); - path[0] = ':'; - (void) confstr (_CS_PATH, path + 1, len); - } + ret = -errno; - len = strlen (file) + 1; - pathlen = strlen (path); - name = __alloca (pathlen + len + 1); - /* Copy the file name at the top. */ - name = (char *) memcpy (name + pathlen + 1, file, len); - /* And add the slash. */ - *--name = '/'; +fail: + /* Since sizeof errno < PIPE_BUF, the write is atomic. */ + ret = -ret; + if (ret) + while (write_not_cancel (args->pipe[1], &ret, sizeof (ret)) < 0); - p = path; - do - { - char *startp; + _exit (SPAWN_ERROR); +} - path = p; - p = __strchrnul (path, ':'); +/* Spawn a new process executing PATH with the attributes describes in *ATTRP. + Before running the process perform the actions described in FILE-ACTIONS. */ +int +__spawnix (pid_t *pid, const char *file, + const posix_spawn_file_actions_t *file_actions, + const posix_spawnattr_t *attrp, char *const argv[], + char *const envp[], int xflags, + int (*exec) (const char *, char *const *, char *const *)) +{ + struct posix_spawn_args args; + int ec; - if (p == path) - /* Two adjacent colons, or a colon at the beginning or the end - of `PATH' means to search the current directory. */ - startp = name + 1; - else - startp = (char *) memcpy (name - (p - path), path, p - path); + if (__pipe2 (args.pipe, O_CLOEXEC)) + return errno; - /* Try to execute this name. If it works, execv will not return. */ - __execve (startp, argv, envp); + /* Disable asynchronous cancellation. */ + int state; + __libc_ptf_call (__pthread_setcancelstate, + (PTHREAD_CANCEL_DISABLE, &state), 0); - maybe_script_execute (startp, argv, envp, xflags); + ptrdiff_t argc = 0; + ptrdiff_t limit = INT_MAX - 1; + while (argv[argc++] != NULL) + if (argc == limit) + { + errno = E2BIG; + return errno; + } - switch (errno) - { - case EACCES: - case ENOENT: - case ESTALE: - case ENOTDIR: - /* Those errors indicate the file is missing or not executable - by us, in which case we want to just try the next path - directory. */ - break; - - default: - /* Some other error means we found an executable file, but - something went wrong executing it; return the error to our - caller. */ - _exit (SPAWN_ERROR); - } + args.file = file; + args.exec = exec; + args.fa = file_actions; + args.attr = attrp ? attrp : &(const posix_spawnattr_t) { 0 }; + args.argv = argv; + args.argc = argc; + args.envp = envp; + args.xflags = xflags; + + /* Generate the new process. */ + pid_t new_pid = __fork (); + + if (new_pid == 0) + __spawni_child (&args); + else if (new_pid > 0) + { + __close (args.pipe[1]); + + if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec) + ec = 0; + else + __waitpid (new_pid, &(int) { 0 }, 0); } - while (*p++ != '\0'); + else + ec = -new_pid; - /* Return with an error. */ - _exit (SPAWN_ERROR); + __close (args.pipe[0]); + + if ((ec == 0) && (pid != NULL)) + *pid = new_pid; + + __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0); + + return ec; +} + +int +__spawni (pid_t * pid, const char *file, + const posix_spawn_file_actions_t * acts, + const posix_spawnattr_t * attrp, char *const argv[], + char *const envp[], int xflags) +{ + return __spawnix (pid, file, acts, attrp, argv, envp, xflags, + xflags & SPAWN_XFLAGS_USE_PATH ? __execvpe : __execve); }