From patchwork Fri Oct 19 23:26:08 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 149356 Delivered-To: patch@linaro.org Received: by 2002:a2e:8595:0:0:0:0:0 with SMTP id b21-v6csp3894416lji; Fri, 19 Oct 2018 16:26:38 -0700 (PDT) X-Google-Smtp-Source: ACcGV61gQ4XsJBrSwrnXU59xNchRXafhpxHnV9bAQYaZ6GqwuAd0vPfag3kGqV5DymDM/IGyXT/H X-Received: by 2002:a17:902:b943:: with SMTP id h3-v6mr36128349pls.5.1539991598208; Fri, 19 Oct 2018 16:26:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1539991598; cv=none; d=google.com; s=arc-20160816; b=GBXH9TIiLNsZQoyGWh/dFjB3EjirKITZW1g3OaHSVkRk0Nk22XGtMXmXrP1OYrqkAA ZddXkSMGYWsAiM3OfM5D7rcFIPxokPPTwrpzxRyDiwm4UEI2YEhVUQAgAtBPTEBSLr3f 92Nlp5GZ3AQLUqm+IBkmrMja8DyTVm6UxwHjiP1P5HnJVHFDGX/FMLeTzX3/GyXd/NJv j79Qnrg5yD9oDjU/aWgI5yX/30mOizelgi5SF10bplfBHTSmuPY/Wt3rGC2hwibQeTJ/ dWLR2UWKDNJTBPuzrurB78kJTIy/ygc57BzyAa+W6+DlsNuvPOo8XTCPSGSSd26ysAZ1 C5yQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=references:in-reply-to:message-id:date:subject:to:from :dkim-signature:delivered-to:sender:list-help:list-post:list-archive :list-subscribe:list-unsubscribe:list-id:precedence:mailing-list :dkim-signature:domainkey-signature; bh=3vTxn6omg6+ipxj4eCp+BlsHzeMABUUXDJRuJVnGsYk=; b=PO5MeJ5IOVv8unF3G6hbwlAomE4CZKW0DH96X/4uguqPH9KIGQgcYbm3DDsXE3DiK2 v/jkoPUioCze7DRVCgKLzkSSebbtScCqfhsqaq6lgZnMSa3L7oeMMJsw0XY9hNaSv9xn 7e5nIO1HtfxpISsprvpnNWdXneNDTdH1c2qo3Fm4yXaHYJHNGJ3Uv61mQyEfv9inRCgy CxyjbwcdIJ5+rgMMKnQz+QPVjKd8dfEaBRCrFi8YMz3iNN6LzhWf8L7MvE4V09C8gyr1 KyCZX+ShfZXxwPi72UbR9PF+B25mgjUcjlXPhaYvHRh+V41h3W3C3z4gnbVpg9LhaB45 vqyQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sourceware.org header.s=default header.b=BPdSKQFR; dkim=pass header.i=@linaro.org header.s=google header.b=BQWd6tib; spf=pass (google.com: domain of libc-alpha-return-96544-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom="libc-alpha-return-96544-patch=linaro.org@sourceware.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id bj12-v6si24442085plb.178.2018.10.19.16.26.37 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 19 Oct 2018 16:26:38 -0700 (PDT) Received-SPF: pass (google.com: domain of libc-alpha-return-96544-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@sourceware.org header.s=default header.b=BPdSKQFR; dkim=pass header.i=@linaro.org header.s=google header.b=BQWd6tib; spf=pass (google.com: domain of libc-alpha-return-96544-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom="libc-alpha-return-96544-patch=linaro.org@sourceware.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id:in-reply-to :references; q=dns; s=default; b=C3lzpZOxyVOERYf3Ft6lpPUkJcaohoh Y61/N7Pkk+5P6DwBXqgIFE3Cz/QGbKQCb4aec+ZrIeOD77uOhz5ZQ1vtJfmtMrlg P4WUYOzpxmppOejQpGLnNneW5/xz0/b9AL2U9B+CCwgMpoAAaCGnwE996q+3ltXP zaMfyQqusVfo= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id:in-reply-to :references; s=default; bh=K+7fHa2UyaOXIizywEhx+1BP/7g=; b=BPdSK QFRU53f24+whc60Iz6FjDb1vtWpFKXHJOf7h36AXjPrk6CHoYunp5kbOFaEBjuNk PGA3uv8gqyr7qEFMe0LojrZbY7m5sTcy1axrdhyLDKB5HYs3hIa8YqUuArjd77ec octx1cwg3OBfxpWlcwhkWZ/WylWNONsjNtNe/4= Received: (qmail 44029 invoked by alias); 19 Oct 2018 23:26:20 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 43956 invoked by uid 89); 19 Oct 2018 23:26:20 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=unlock, HX-Received:sk:18-v6mr X-HELO: mail-qk1-f194.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:subject:date:message-id:in-reply-to:references; bh=3vTxn6omg6+ipxj4eCp+BlsHzeMABUUXDJRuJVnGsYk=; b=BQWd6tibLEP8DlXC6ul8hsW5f6VZXkZORbFYoH3LbDqADBgBCbD2qDUVDCEIKLiPba UhDErmV7c43lLsYfb7/N+msxsp6iE6IhUJw8GW39sVsud2sQtLYBgJMTGz0NGVPY273n EqE7wswx0VSCeKYDlv+ULZ+LLoJTnqRVIoY/c= Return-Path: From: Adhemerval Zanella To: libc-alpha@sourceware.org Subject: [PATCH v2 2/3] posix: Use posix_spawn on popen Date: Fri, 19 Oct 2018 20:26:08 -0300 Message-Id: <20181019232609.25531-2-adhemerval.zanella@linaro.org> In-Reply-To: <20181019232609.25531-1-adhemerval.zanella@linaro.org> References: <20181019232609.25531-1-adhemerval.zanella@linaro.org> 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 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 #include #include -#include +#include +#include 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