I don't know whether calling atfork handlers from popen violates POSIX, but it's annoying and causes popen to be unsafe in conjunction with various libraries when it should be perfectly safe. It would also be much faster if it used vfork or appropriate clone flags on Linux.
POSIX pthread_atfork [1] description only list 'fork' as the function where should issue the atfork handlers. Popen description [2] states that it '[...] shall be *as if* a child process were created within the popen() call using the fork() function [...]' This description does not state if popen should follow all fork semantic, neither if it allows of not popen be based on internally on fork (specially regarding atfork handlers). Other libc/system seems to follow the idea atfork handlers should not be issue for popen: libc/system | run atfork handles | notes ----------------|----------------------|--------------------------------------- freebsd master | no | uses vfork solaris 11 | no | MacOSX (11.13) | no | implemented through posix_spawn syscall ----------------|----------------------|---------------------------------------- And I also agree that, as for 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 inconsistent are not really required. I have sent the fix for this issue on libc-alpha [3]. [1] http://pubs.opengroup.org/onlinepubs/9699919799/ [2] http://pubs.opengroup.org/onlinepubs/9699919799/ [3] https://sourceware.org/ml/libc-alpha/2018-09/msg00218.html
I think POSIX is actually pretty clear here, and that POSIX requires that the fork handlers are used from within popen and system. But I also think this is a defect in POSIX, so I don't see a problem with deviating from POSIX here.
Considering the rationale on pthread_atfork, it would make sense to actually run atfork handlers on popen/system iff the api ran some user-provided callback which might taking any user-provided locks. In any case, I think we need to either: 1. Accept this bug and open an Austin Group defect to change the popen/system definition. 2. Close this bug and open a Austin Group clarification about popen/system behavior regarding atfork handlers. I am more inclined to follow 1.
(In reply to Adhemerval Zanella from comment #3) > Considering the rationale on pthread_atfork, it would make sense to actually > run atfork handlers on popen/system iff the api ran some user-provided > callback which might taking any user-provided locks. > > In any case, I think we need to either: > > 1. Accept this bug and open an Austin Group defect to change the > popen/system definition. > > 2. Close this bug and open a Austin Group clarification about popen/system > behavior regarding atfork handlers. > > I am more inclined to follow 1. I agree (“accept this bug” meaning “fix this bug in glibc”).
Right, so my changes [1] [2] follow this direction. [1] https://sourceware.org/ml/libc-alpha/2018-10/msg00364.html [2] https://sourceware.org/ml/libc-alpha/2018-10/msg00363.html
This is an automated email from the git hooks/post-receive script. It was generated because a ref change was pushed to the repository containing the project "GNU C Library master sources". The branch, master has been updated via 5fb7fc96350575c9adb1316833e48ca11553be49 (commit) via 14d0e87d9b8caaa2eca7ca81f1189596671fe4fb (commit) from e5d262effe3a87164308a3f37e61b32d0348692a (commit) Those revisions listed above that are new to this repository have not appeared on any other notification email; so we list those revisions in full, below. - Log ----------------------------------------------------------------- https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=5fb7fc96350575c9adb1316833e48ca11553be49 commit 5fb7fc96350575c9adb1316833e48ca11553be49 Author: Adhemerval Zanella <adhemerval.zanella@linaro.org> Date: Wed Oct 24 16:29:38 2018 -0300 posix: Use posix_spawn on system This patch uses posix_spawn on system implementation. On Linux this has the advantage of much lower memory consumption (usually 32 Kb minimum for the mmap stack area). Although POSIX does not require, glibc system implementation aims to be thread and cancellation safe. The cancellation code is moved to generic implementation and enabled iff SIGCANCEL is defined (similar on how the cancellation handler is enabled on nptl-init.c). Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu, arm-linux-gnueabihf, and powerpc64le-linux-gnu. * sysdeps/unix/sysv/linux/spawni.c (__spawni_child): Use __sigismember instead of sigismember. * sysdeps/posix/system.c [SIGCANCEL] (cancel_handler_args, cancel_handler): New definitions. (CLEANUP_HANDLER, CLEANUP_RESET): Likewise. (DO_LOCK, DO_UNLOCK, INIT_LOCK, ADD_REF, SUB_REF): Remove. (do_system): Use posix_spawn instead of fork and execl and remove reentracy code. * sysdeps/generic/not-errno.h (__kill_noerrno): New prototype. * sysdeps/unix/sysv/linux/not-errno.h (__kill_noerrno): Likewise. * sysdeps/unix/sysv/linux/ia64/system.c: Remove file. * sysdeps/unix/sysv/linux/s390/system.c: Likewise. * sysdeps/unix/sysv/linux/sparc/system.c: Likewise. * sysdeps/unix/sysv/linux/system.c: Likewise. https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=14d0e87d9b8caaa2eca7ca81f1189596671fe4fb commit 14d0e87d9b8caaa2eca7ca81f1189596671fe4fb Author: Adhemerval Zanella <adhemerval.zanella@linaro.org> Date: Wed Sep 12 10:32:05 2018 -0300 posix: Use posix_spawn on popen 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 that should execute the atfork handlers, 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 executed for popen: libc/system | run atfork handles | notes ------------|----------------------|--------------------------------------- Freebsd | no | uses vfork Solaris 11 | no | MacOSX 11 | 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: the described scenario, where the forked process might access invalid memory due an inconsistent state in multithreaded environment, should not happen because posix_spawn does not access the affected data structure (proc_file_chain). Checked on x86_64-linux-gnu and i686-linux-gnu. [BZ #22834] [BZ #17490] * NEWS: Add new semantic for atfork with popen and system. * libio/iopopen.c (_IO_new_proc_open): use posix_spawn instead of fork and execl. ----------------------------------------------------------------------- Summary of changes: ChangeLog | 21 ++++ NEWS | 6 + libio/iopopen.c | 134 +++++++++++++++-------- sysdeps/generic/not-errno.h | 2 + sysdeps/posix/system.c | 189 +++++++++++++++++--------------- sysdeps/unix/sysv/linux/ia64/system.c | 30 ----- sysdeps/unix/sysv/linux/not-errno.h | 14 +++ sysdeps/unix/sysv/linux/s390/system.c | 29 ----- sysdeps/unix/sysv/linux/sparc/system.c | 29 ----- sysdeps/unix/sysv/linux/spawni.c | 4 +- sysdeps/unix/sysv/linux/system.c | 76 ------------- 11 files changed, 231 insertions(+), 303 deletions(-) delete mode 100644 sysdeps/unix/sysv/linux/ia64/system.c delete mode 100644 sysdeps/unix/sysv/linux/s390/system.c delete mode 100644 sysdeps/unix/sysv/linux/sparc/system.c delete mode 100644 sysdeps/unix/sysv/linux/system.c
Fixed on 2.29 (commit 14d0e87d9b8caaa2eca7ca81f1189596671fe4fb).