| Message ID | 20181019232609.25531-2-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] posix: Add internal symbols for posix_spawn interface | expand |
* Adhemerval Zanella: > Changed from previous version: > > - Added note about BZ#17490 fix. > - Use proc_file_chain_lock to access proc_file_chain (BZ#22834). > > --- > > This patch uses posix_spawn on popen instead of fork and execl. On Linux > this has the advantage of much lower memory consumption (usually 32 Kb > minimum for the mmap stack area). Does this need a NEWS entry, noting the fork handler aspect? > + int op; The variable name isn't helpful. It's overloaded for pipe index and descriptor index in the child. Maybe use three variables with appropriate names instead? > + { > + posix_spawn_file_actions_t fa; > + /* posix_spawn_file_actions_init does not fail. */ > + __posix_spawn_file_actions_init (&fa); > > - /* If any stream from previous popen() calls has fileno > - child_std_end, it has been already closed by the dup2 syscall > - above. */ > - if (fd != child_std_end) > - __close_nocancel (fd); > - } > + /* The descriptor is already in the one the child will use. In this case > + it must be moved to another one, otherwise there is no safe way to > + remove the close-on-exec flag in the child without creating a FD leak > + race in the parent. */ I think “in the one” is a bit confusing here. > + if (pipe_fds[1 - op] == 1 - op) > + { > + int tmp = __fcntl (1 - op, F_DUPFD_CLOEXEC, 0); > + if (tmp < 0) > + goto spawn_failure; > + __close_nocancel (pipe_fds[1 - op]); Missing close of pipe_fds[op] in the error case; spawn_failure only closes the parent end. > + if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[1 - op], 1 - op) > + != 0) > + goto spawn_failure; Likewise, missing close of parent end. > + /* POSIX.2: "popen() shall ensure that any streams from previous popen() > + calls that remain open in the parent process are closed in the new > + child process." */ > +#ifdef _IO_MTSAFE_IO > + _IO_cleanup_region_start_noarg (unlock); > + _IO_lock_lock (proc_file_chain_lock); > +#endif You can assume that _IO_MTSAFE_IO is always defined here and not carry over the preprocessor conditionals. > + for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next) > + { > + int fd = _IO_fileno ((FILE *) p); > + > + /* If any stream from previous popen() calls has fileno > + child_send, it has been already closed by the dup2 syscall > + above. */ > + if (fd != 1 - op > + && __posix_spawn_file_actions_addclose (&fa, fd) != 0) > + goto spawn_failure; > + } The jump out of cleanup region is undefined, I think. Missing close of parent end. > +#ifdef _IO_MTSAFE_IO > + _IO_lock_unlock (proc_file_chain_lock); > + _IO_cleanup_region_end (0); > +#endif > + > + if (__posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, &fa, 0, > + (char *const[]){ (char*) "sh", (char*) "-c", > + (char *) command, NULL }, __environ) != 0) > + { > + spawn_failure: > + __posix_spawn_file_actions_destroy (&fa); > + __close_nocancel (pipe_fds[1 - op]); > + __set_errno (ENOMEM); > + return NULL; Not sure what's supposd to happen to the parent end in this case and if you need to close it here. > + } > + > + __posix_spawn_file_actions_destroy (&fa); > + } > + __close_nocancel (pipe_fds[1 - op]); > + if (((_IO_proc_file *) fp)->pid < 0) > { > - __close_nocancel (parent_end); > + __close_nocancel (pipe_fds[op]); > return NULL; > } How can ->pid be negative without a posix_spawn failure? Thanks, Florian
On 24/10/2018 07:07, Florian Weimer wrote: > * Adhemerval Zanella: > >> Changed from previous version: >> >> - Added note about BZ#17490 fix. >> - Use proc_file_chain_lock to access proc_file_chain (BZ#22834). >> >> --- >> >> This patch uses posix_spawn on popen instead of fork and execl. On Linux >> this has the advantage of much lower memory consumption (usually 32 Kb >> minimum for the mmap stack area). > > Does this need a NEWS entry, noting the fork handler aspect? Although BZ#17490 already tracks it, including a discussion about possible POSIX deviation, it won't hurt. What about: * The popen and system do not run atfork handlers anymore (BZ#17490). Although it is a possible POSIX violation, the POSIX rationale in pthread_atfork documentation regarding atfork handlers is to handle incosistent mutex state after fork call in multithread environment. In both popen and system there is no direct access to user defined mutexes. As a side-note, we also changed posix_spawn behaviour regarding atfork handler (at least when it used fork instead of vfork) with its new implementation. > >> + int op; > > The variable name isn't helpful. It's overloaded for pipe index and > descriptor index in the child. Maybe use three variables with > appropriate names instead? Alright, I used a variable for each case as before. > >> + { >> + posix_spawn_file_actions_t fa; >> + /* posix_spawn_file_actions_init does not fail. */ >> + __posix_spawn_file_actions_init (&fa); >> >> - /* If any stream from previous popen() calls has fileno >> - child_std_end, it has been already closed by the dup2 syscall >> - above. */ >> - if (fd != child_std_end) >> - __close_nocancel (fd); >> - } >> + /* The descriptor is already in the one the child will use. In this case >> + it must be moved to another one, otherwise there is no safe way to >> + remove the close-on-exec flag in the child without creating a FD leak >> + race in the parent. */ > > I think “in the one” is a bit confusing here. I meant 'The descriptor is already the one the child will use', I fixed that. > >> + if (pipe_fds[1 - op] == 1 - op) >> + { >> + int tmp = __fcntl (1 - op, F_DUPFD_CLOEXEC, 0); >> + if (tmp < 0) >> + goto spawn_failure; >> + __close_nocancel (pipe_fds[1 - op]); > > Missing close of pipe_fds[op] in the error case; spawn_failure only > closes the parent end. Ack, I have fixed it. > >> + if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[1 - op], 1 - op) >> + != 0) >> + goto spawn_failure; > > Likewise, missing close of parent end. Ack, I have fixed it. > >> + /* POSIX.2: "popen() shall ensure that any streams from previous popen() >> + calls that remain open in the parent process are closed in the new >> + child process." */ >> +#ifdef _IO_MTSAFE_IO >> + _IO_cleanup_region_start_noarg (unlock); >> + _IO_lock_lock (proc_file_chain_lock); >> +#endif > > You can assume that _IO_MTSAFE_IO is always defined here and not carry > over the preprocessor conditionals. Could we move this change to future cleanup patch instead? I would like to add for consistency with other _IO_MTSAFE_IO usage and to avoid remove all _IO_MTSAFE_IO on this patch specifically. > >> + for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next) >> + { >> + int fd = _IO_fileno ((FILE *) p); >> + >> + /* If any stream from previous popen() calls has fileno >> + child_send, it has been already closed by the dup2 syscall >> + above. */ >> + if (fd != 1 - op >> + && __posix_spawn_file_actions_addclose (&fa, fd) != 0) >> + goto spawn_failure; >> + } > > The jump out of cleanup region is undefined, I think. Missing close of > parent end. Indeed, I changed it. > >> +#ifdef _IO_MTSAFE_IO >> + _IO_lock_unlock (proc_file_chain_lock); >> + _IO_cleanup_region_end (0); >> +#endif >> + >> + if (__posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, &fa, 0, >> + (char *const[]){ (char*) "sh", (char*) "-c", >> + (char *) command, NULL }, __environ) != 0) >> + { >> + spawn_failure: >> + __posix_spawn_file_actions_destroy (&fa); >> + __close_nocancel (pipe_fds[1 - op]); >> + __set_errno (ENOMEM); >> + return NULL; > > Not sure what's supposd to happen to the parent end in this case and if > you need to close it here. I think we do need to close both ends otherwise we will leak parent's descriptor. > >> + } >> + >> + __posix_spawn_file_actions_destroy (&fa); >> + } >> + __close_nocancel (pipe_fds[1 - op]); >> + if (((_IO_proc_file *) fp)->pid < 0) >> { >> - __close_nocancel (parent_end); >> + __close_nocancel (pipe_fds[op]); >> return NULL; >> } > > How can ->pid be negative without a posix_spawn failure? It can not, I have remove it.
diff --git a/libio/iopopen.c b/libio/iopopen.c index 2eff45b4c8..a2867abb02 100644 --- a/libio/iopopen.c +++ b/libio/iopopen.c @@ -34,7 +34,8 @@ #include <not-cancel.h> #include <sys/types.h> #include <sys/wait.h> -#include <kernel-features.h> +#include <spawn.h> +#include <paths.h> struct _IO_proc_file { @@ -63,9 +64,8 @@ FILE * _IO_new_proc_open (FILE *fp, const char *command, const char *mode) { int read_or_write; - int parent_end, child_end; int pipe_fds[2]; - pid_t child_pid; + int op; int do_read = 0; int do_write = 0; @@ -108,59 +108,86 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode) if (do_read) { - parent_end = pipe_fds[0]; - child_end = pipe_fds[1]; + op = 0; read_or_write = _IO_NO_WRITES; } else { - parent_end = pipe_fds[1]; - child_end = pipe_fds[0]; + op = 1; read_or_write = _IO_NO_READS; } - ((_IO_proc_file *) fp)->pid = child_pid = __fork (); - if (child_pid == 0) - { - int child_std_end = do_read ? 1 : 0; - struct _IO_proc_file *p; - - if (child_end != child_std_end) - __dup2 (child_end, child_std_end); - else - /* The descriptor is already the one we will use. But it must - not be marked close-on-exec. Undo the effects. */ - __fcntl (child_end, F_SETFD, 0); - /* POSIX.2: "popen() shall ensure that any streams from previous - popen() calls that remain open in the parent process are closed - in the new child process." */ - for (p = proc_file_chain; p; p = p->next) - { - int fd = _IO_fileno ((FILE *) p); + { + posix_spawn_file_actions_t fa; + /* posix_spawn_file_actions_init does not fail. */ + __posix_spawn_file_actions_init (&fa); - /* If any stream from previous popen() calls has fileno - child_std_end, it has been already closed by the dup2 syscall - above. */ - if (fd != child_std_end) - __close_nocancel (fd); - } + /* The descriptor is already in the one the child will use. In this case + it must be moved to another one, otherwise there is no safe way to + remove the close-on-exec flag in the child without creating a FD leak + race in the parent. */ + if (pipe_fds[1 - op] == 1 - op) + { + int tmp = __fcntl (1 - op, F_DUPFD_CLOEXEC, 0); + if (tmp < 0) + goto spawn_failure; + __close_nocancel (pipe_fds[1 - op]); + pipe_fds[1 - op] = tmp; + } - execl ("/bin/sh", "sh", "-c", command, (char *) 0); - _exit (127); - } - __close_nocancel (child_end); - if (child_pid < 0) + if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[1 - op], 1 - op) + != 0) + goto spawn_failure; + + /* POSIX.2: "popen() shall ensure that any streams from previous popen() + calls that remain open in the parent process are closed in the new + child process." */ +#ifdef _IO_MTSAFE_IO + _IO_cleanup_region_start_noarg (unlock); + _IO_lock_lock (proc_file_chain_lock); +#endif + for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next) + { + int fd = _IO_fileno ((FILE *) p); + + /* If any stream from previous popen() calls has fileno + child_send, it has been already closed by the dup2 syscall + above. */ + if (fd != 1 - op + && __posix_spawn_file_actions_addclose (&fa, fd) != 0) + goto spawn_failure; + } +#ifdef _IO_MTSAFE_IO + _IO_lock_unlock (proc_file_chain_lock); + _IO_cleanup_region_end (0); +#endif + + if (__posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, &fa, 0, + (char *const[]){ (char*) "sh", (char*) "-c", + (char *) command, NULL }, __environ) != 0) + { + spawn_failure: + __posix_spawn_file_actions_destroy (&fa); + __close_nocancel (pipe_fds[1 - op]); + __set_errno (ENOMEM); + return NULL; + } + + __posix_spawn_file_actions_destroy (&fa); + } + __close_nocancel (pipe_fds[1 - op]); + if (((_IO_proc_file *) fp)->pid < 0) { - __close_nocancel (parent_end); + __close_nocancel (pipe_fds[op]); return NULL; } if (!do_cloexec) /* Undo the effects of the pipe2 call which set the close-on-exec flag. */ - __fcntl (parent_end, F_SETFD, 0); + __fcntl (pipe_fds[op], F_SETFD, 0); - _IO_fileno (fp) = parent_end; + _IO_fileno (fp) = pipe_fds[op]; /* Link into proc_file_chain. */ #ifdef _IO_MTSAFE_IO
Changed from previous version: - Added note about BZ#17490 fix. - Use proc_file_chain_lock to access proc_file_chain (BZ#22834). --- This patch uses posix_spawn on popen instead of fork and execl. On Linux this has the advantage of much lower memory consumption (usually 32 Kb minimum for the mmap stack area). Two issues are also fixed with this change: * BZ#17490: although POSIX pthread_atfork description only list 'fork' as the function where should issue the atfork handlers and popen description states that: '[...] shall be *as if* a child process were created within the popen() call using the fork() function [...]' Other libc/system seems to follow the idea atfork handlers should not be issue for popen: libc/system | run atfork handles | notes ------------|----------------------|--------------------------------------- freebsd | no | uses vfork solaris 11 | no | MacOSX 11i | no | implemented through posix_spawn syscall ------------|----------------------|---------------------------------------- Similar to posix_spawn and system, popen idea is to spawn a different binary so all the POSIX rationale to run the atfork handlers to avoid internal process inconsistency is not really required and in some cases might be unsafe. * BZ#22834: now that proc_file_chain is not copied on another process, it just require to access is through the proc_file_chain_lock. Checked on x86_64-linux-gnu and i686-linux-gnu. [BZ #22834] [BZ #17490] * libio/iopopen.c (_IO_new_proc_open): use posix_spawn instead of fork and execl. --- ChangeLog | 4 ++ libio/iopopen.c | 105 ++++++++++++++++++++++++++++++------------------ 2 files changed, 70 insertions(+), 39 deletions(-) -- 2.17.1