From patchwork Thu Apr 25 21:36:21 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 162893 Delivered-To: patch@linaro.org Received: by 2002:a02:c6d8:0:0:0:0:0 with SMTP id r24csp2456332jan; Thu, 25 Apr 2019 14:36:39 -0700 (PDT) X-Google-Smtp-Source: APXvYqyXwq0DX+CNgwrdrqkd95DXMnh8AyT2ieJK7EGcE6L5oWFyOkLPYHlyScrCncddJTUjXDZR X-Received: by 2002:a62:7d57:: with SMTP id y84mr17750062pfc.52.1556228199627; Thu, 25 Apr 2019 14:36:39 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1556228199; cv=none; d=google.com; s=arc-20160816; b=Qm36GCBf9hNnvddxI16STCvwmHkSsqM3YCANvfKh7/ap8tBnwuZKnMyisENUKoITE5 Dr5OS+js6ngO2EE4MFfPeZ8LPTfplU8FTvJotFEl1RhwNrGzr/ZibjM5nURnXdFnuKZw MA+opIykpqQL0to5aX6rb6SLuNB7AMrctaf7pvc0tKZO72HFUtsr4KNcHP7OhkpIGEp4 TXji1J8TTLUED+y3EI676huhbHci8PAD2DnPj3IaZXjpfc9Ly2noaqJQRNZPvNw5AZph Um1Y4qPPKxKR0jx7Jeurf0v7VEVPUexT3hYkZpoRay7Zf+0TtuRToGUqHpbG5qnSuh3L F0PA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=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=MnDKT7/PTrRaWz4YdnXvU9wBZWqSLPviAOEZhQl3Xdk=; b=VxnaZ9CCCZmBm9dAlI7sh4oHs5ZOC1nQ5Gogt/ykupJMwiaVmvpEY3Bo7jHUgCCF55 JZS8QksJoqyzJIs/Qc+e6rD+NkzMXGwL3IQDqLENLogH9NKdTzwdt60/QJNhu8fFDKRP AGYn0QDnLjowvUu/6OTsBzwCgilI1F5XWhiAiZb/IgAjwTCAwcKnQlhfcJGWahlIOzFn cU+3WJqgzAeR0gzFfJrtkq8eOe2nxHSW8cYaehbRxQVi2cot7E7+fv9ktcY1vtaIn4SF DmHkv6mNiAnjHT/5dia/fnkK838VRp/PzMt+2Dbg3s+/E/DgdwXCyHq+XPYxN2pmK/OL ABGQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sourceware.org header.s=default header.b=ugVTZGIT; dkim=pass header.i=@linaro.org header.s=google header.b=LAGTXW6b; spf=pass (google.com: domain of libc-alpha-return-101619-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom="libc-alpha-return-101619-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 d12si12989631pla.405.2019.04.25.14.36.39 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 25 Apr 2019 14:36:39 -0700 (PDT) Received-SPF: pass (google.com: domain of libc-alpha-return-101619-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=ugVTZGIT; dkim=pass header.i=@linaro.org header.s=google header.b=LAGTXW6b; spf=pass (google.com: domain of libc-alpha-return-101619-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom="libc-alpha-return-101619-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; q=dns; s= default; b=muPIggwX6l3/BJP1qq4/ffA/duRwkp9Zk6JkwEjrDPuuOrJ/ehO5/ aEgrZMYj5jwrW0+rkomRiyaT657Ol3/Kspd6nvYcqnALldX9C0pucEaPA7wbNDiG z11/wsdPB3q7b30e8xQBm1IrhvMGYxag5IEuVQh5uH7v39j55Z3u+o= 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; s=default; bh=vgI98Xe6dxg0GA6xwNTG07aK6Zc=; b=ugVTZGITHtG7reccHGY2YrFVOUFx SHh362HVBZapAZQce+LKcxcRLaJwXsXszaRJwj8zFz2HLD151FPxvwmo5o7ppO5n 4WzXWnfpofzbH+PDQul/k5I4eMvW6jNvjnl5lj/RVBhlHEkPty8kW/l4wS2m/50c wX088eoNWyAyFNg= Received: (qmail 30582 invoked by alias); 25 Apr 2019 21:36:30 -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 30573 invoked by uid 89); 25 Apr 2019 21:36:30 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=cancellation, 603, device X-HELO: mail-vs1-f67.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:subject:date:message-id; bh=MnDKT7/PTrRaWz4YdnXvU9wBZWqSLPviAOEZhQl3Xdk=; b=LAGTXW6bEmBXhEcbxE1Z9mNinoBMN1NySWt4s0lWn7FzZoQ23Eti6+GP5/+WdTEfPz 7LhWpCUjHphqcMwapFVyavbOqk30Ht3+hLfRVOGXnrk55zWpBhhSsw1sBi8dSprf8nkL fQ/3lcNY74G5sIKVFfWUpqN2Ara9l1Wi4v7OgcCmeQaEAfcivs8gaZ9U0pCaNsPs/8Ed YjAnuMpiAsaxdL03m3kR9MV6EvQ0jy4I6BU0t56KmpDJCEIl4q18ixPGc7V40DTa8zCS 2+gVp9IFFQLRKiq5e3qfQgF7k6V7jfY572xaNNiQwd5HAc0MtnzQgowBUQvrYCbeNoGW M9XQ== Return-Path: From: Adhemerval Zanella To: libc-alpha@sourceware.org Subject: [PATCH] posix: Use posix_spawn for wordexp Date: Thu, 25 Apr 2019 18:36:21 -0300 Message-Id: <20190425213621.689-1-adhemerval.zanella@linaro.org> This patch replaces the fork+exec by posix_spawn on wordexp, which allows a better scability on Linux and simplifies the thread cancellation handling. The only change which can not be implemented with posix_spawn the /dev/null check to certify it is indeed the expected device. I am not sure how effetive this check really is since /dev/null tampering means something very wrong with the system and this is the least of the issues. My view is the tests is really out of the place and the hardening provided is minimum. If the idea is still to provide such check, I think a possibilty would be to open /dev/null, check it, add a dup2 file action, and close the file descriptor. Checked on powerpc64le-linux-gnu and x86_64-linux-gnu. * include/spawn.h (__posix_spawn_file_actions_addopen): New prototype. * posix/spawn_faction_addopen.c (posix_spawn_file_actions_addopen): Add internal alias. * posix/wordexp.c (create_environment, free_environment): New functions. (exec_comm_child, exec_comm): Use posix_spawn instead of fork+exec. --- include/spawn.h | 3 + posix/spawn_faction_addopen.c | 8 +- posix/wordexp.c | 167 ++++++++++++++++------------------ 3 files changed, 87 insertions(+), 91 deletions(-) -- 2.17.1 diff --git a/include/spawn.h b/include/spawn.h index 7fdd965bd7..4a0b1849da 100644 --- a/include/spawn.h +++ b/include/spawn.h @@ -11,6 +11,9 @@ __typeof (posix_spawn_file_actions_addclose) __typeof (posix_spawn_file_actions_adddup2) __posix_spawn_file_actions_adddup2 attribute_hidden; +__typeof (posix_spawn_file_actions_addopen) + __posix_spawn_file_actions_addopen attribute_hidden; + __typeof (posix_spawn_file_actions_destroy) __posix_spawn_file_actions_destroy attribute_hidden; diff --git a/posix/spawn_faction_addopen.c b/posix/spawn_faction_addopen.c index 742eb9526d..2e598de300 100644 --- a/posix/spawn_faction_addopen.c +++ b/posix/spawn_faction_addopen.c @@ -25,9 +25,9 @@ /* Add an action to FILE-ACTIONS which tells the implementation to call `open' for the given file during the `spawn' call. */ int -posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, - int fd, const char *path, int oflag, - mode_t mode) +__posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, + int fd, const char *path, int oflag, + mode_t mode) { struct __spawn_action *rec; @@ -60,3 +60,5 @@ posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions, return 0; } +weak_alias (__posix_spawn_file_actions_addopen, + posix_spawn_file_actions_addopen) diff --git a/posix/wordexp.c b/posix/wordexp.c index 22c6d18a9c..f0425f39a8 100644 --- a/posix/wordexp.c +++ b/posix/wordexp.c @@ -25,33 +25,18 @@ #include #include #include -#include #include #include -#include #include #include -#include -#include -#include -#include #include #include -#include #include -#include +#include #include - -#include #include <_itoa.h> - -/* Undefine the following line for the production version. */ -/* #define NDEBUG 1 */ #include -/* Get some device information. */ -#include - /* * This is a recursive-descent-style word expansion routine. */ @@ -812,61 +797,87 @@ parse_arith (char **word, size_t *word_length, size_t *max_length, return WRDE_SYNTAX; } +static char ** +create_environment (void) +{ + size_t s = 0; + /* Calculate environment total sizei, including 'IFS'. */ + for (char **ep = __environ; *ep != NULL; ep++, s++); + + /* Include final NULL pointer. */ + char **newenviron = malloc (s * (sizeof (char*) + 1)); + if (newenviron == NULL) + return NULL; + + /* Copy current environment excluding 'IFS', to make sure the subshell + doesn't field-split on our behalf. */ + size_t i, j; + for (i = 0, j = 0; i < s; i++) + if (strncmp (__environ[i], "IFS=", sizeof ("IFS=")-1) != 0) + newenviron[j++] = __strdup (__environ[i]); + newenviron[j] = NULL; + + return newenviron; +} + +static void +free_environment (char **environ) +{ + for (char **ep = environ; *ep != NULL; ep++) + free (*ep); + free (environ); +} + /* Function called by child process in exec_comm() */ -static inline void -__attribute__ ((always_inline)) -exec_comm_child (char *comm, int *fildes, int showerr, int noexec) +static pid_t +exec_comm_child (char *comm, int *fildes, bool showerr, bool noexec) { - const char *args[4] = { _PATH_BSHELL, "-c", comm, NULL }; + pid_t pid = -1; - /* Execute the command, or just check syntax? */ - if (noexec) - args[1] = "-nc"; + /* Execute the command, or just check syntax? */ + const char *args[] = { _PATH_BSHELL, noexec ? "-nc" : "-c", comm, NULL }; - /* Redirect output. */ - if (__glibc_likely (fildes[1] != STDOUT_FILENO)) - { - __dup2 (fildes[1], STDOUT_FILENO); - __close (fildes[1]); - } - else - /* Reset the close-on-exec flag (if necessary). */ - __fcntl (fildes[1], F_SETFD, 0); + posix_spawn_file_actions_t fa; + /* posix_spawn_file_actions_init does not fail. */ + __posix_spawn_file_actions_init (&fa); - /* Redirect stderr to /dev/null if we have to. */ - if (showerr == 0) + /* Redirect output. For check syntax only (noexec being true), exec_comm + explicits sets fildes[1] to -1, so check its value to avoid a failure in + __posix_spawn_file_actions_adddup2. */ + if (fildes[1] != -1) { - struct stat64 st; - int fd; - __close (STDERR_FILENO); - fd = __open (_PATH_DEVNULL, O_WRONLY); - if (fd >= 0 && fd != STDERR_FILENO) + if (__glibc_likely (fildes[1] != STDOUT_FILENO)) { - __dup2 (fd, STDERR_FILENO); - __close (fd); + if (__posix_spawn_file_actions_adddup2 (&fa, fildes[1], + STDOUT_FILENO) != 0 + || __posix_spawn_file_actions_addclose (&fa, fildes[1]) != 0) + goto deallocate; } - /* Be paranoid. Check that we actually opened the /dev/null - device. */ - if (__builtin_expect (__fxstat64 (_STAT_VER, STDERR_FILENO, &st), 0) != 0 - || __builtin_expect (S_ISCHR (st.st_mode), 1) == 0 -#if defined DEV_NULL_MAJOR && defined DEV_NULL_MINOR - || st.st_rdev != __gnu_dev_makedev (DEV_NULL_MAJOR, DEV_NULL_MINOR) -#endif - ) - /* It's not the /dev/null device. Stop right here. The - problem is: how do we stop? We use _exit() with an - hopefully unusual exit code. */ - _exit (90); + else + /* Reset the close-on-exec flag (if necessary). */ + if (__posix_spawn_file_actions_adddup2 (&fa, fildes[1], fildes[1]) + != 0) + goto deallocate; } - /* Make sure the subshell doesn't field-split on our behalf. */ - __unsetenv ("IFS"); - - __close (fildes[0]); - __execve (_PATH_BSHELL, (char *const *) args, __environ); - - /* Bad. What now? */ - abort (); + /* Redirect stderr to /dev/null if we have to. */ + if (!showerr) + if (__posix_spawn_file_actions_addopen (&fa, STDERR_FILENO, _PATH_DEVNULL, + O_WRONLY, 0) != 0) + goto deallocate; + + char **newenv = create_environment (); + if (newenv == NULL) + goto deallocate; + /* pid is unset if posix_spawn fails, so it keep the original value + of -1. */ + __posix_spawn (&pid, _PATH_BSHELL, &fa, NULL, (char *const *) args, newenv); + free_environment (newenv); + +deallocate: + __posix_spawn_file_actions_destroy (&fa); + + return pid; } /* Function to execute a command and retrieve the results */ @@ -884,7 +895,7 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, size_t maxnewlines = 0; char buffer[bufsize]; pid_t pid; - int noexec = 0; + bool noexec = false; /* Do nothing if command substitution should not succeed. */ if (flags & WRDE_NOCMD) @@ -898,19 +909,15 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, return WRDE_NOSPACE; again: - if ((pid = __fork ()) < 0) + pid = exec_comm_child (comm, fildes, noexec ? false : flags & WRDE_SHOWERR, + noexec); + if (pid < 0) { - /* Bad */ __close (fildes[0]); __close (fildes[1]); return WRDE_NOSPACE; } - if (pid == 0) - exec_comm_child (comm, fildes, noexec ? 0 : flags & WRDE_SHOWERR, noexec); - - /* Parent */ - /* If we are just testing the syntax, only wait. */ if (noexec) return (TEMP_FAILURE_RETRY (__waitpid (pid, &status, 0)) == pid @@ -1091,7 +1098,7 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length, /* Check for syntax error (re-execute but with "-n" flag) */ if (buflen < 1 && status != 0) { - noexec = 1; + noexec = true; goto again; } @@ -1143,26 +1150,10 @@ parse_comm (char **word, size_t *word_length, size_t *max_length, /* Go -- give script to the shell */ if (comm) { -#ifdef __libc_ptf_call - /* We do not want the exec_comm call to be cut short - by a thread cancellation since cleanup is very - ugly. Therefore disable cancellation for - now. */ - // XXX Ideally we do want the thread being cancelable. - // XXX If demand is there we'll change it. - int state = PTHREAD_CANCEL_ENABLE; - __libc_ptf_call (__pthread_setcancelstate, - (PTHREAD_CANCEL_DISABLE, &state), 0); -#endif - + /* posix_spawn already handles thread cancellation, so + there is no need to handle it here. */ error = exec_comm (comm, word, word_length, max_length, flags, pwordexp, ifs, ifs_white); - -#ifdef __libc_ptf_call - __libc_ptf_call (__pthread_setcancelstate, - (state, NULL), 0); -#endif - free (comm); }