From patchwork Fri Feb 26 13:56:39 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella X-Patchwork-Id: 63058 Delivered-To: patch@linaro.org Received: by 10.112.199.169 with SMTP id jl9csp731174lbc; Fri, 26 Feb 2016 05:57:12 -0800 (PST) X-Received: by 10.98.0.84 with SMTP id 81mr2195625pfa.67.1456495032303; Fri, 26 Feb 2016 05:57:12 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id db2si4648119pad.210.2016.02.26.05.57.11 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 26 Feb 2016 05:57:12 -0800 (PST) Received-SPF: pass (google.com: domain of libc-alpha-return-67662-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; spf=pass (google.com: domain of libc-alpha-return-67662-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-67662-patch=linaro.org@sourceware.org; dkim=pass header.i=@sourceware.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=Qqss/m89uLjm3sMMmHRcMulFAp1tHnZ 3xAMBN84P/O//geu2oSLRzMjksrsobKiHFNjKCC8bGTQHIiAOCAhSrHqGLZTBaeS Rw9DoSgNnGn+oTK7CZvKBxeryQYtxw+JhP8Wz/mzhxMdu1OjI0WL8kJ9MLV0cDAp uck5300kGYvo= 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=X3qtMhcJOhEonYs/T49q9DaO9eM=; b=hDRXO u2N4PuGjWw02Itah1/W48uz4Oboz9Hk95z8Pcy6Ip8V0x8/yrog/zMi5D165VV1L BYCfxPFqw4NAHOEnJOTYhSlhYb/WaHYFE1eOfKzRY2jUxzhSbP4uvvNd5Vt/W3W3 JoE0CxP8DU2OrSgE/k8ZUKDDzE1FyYm4+uFR5s= Received: (qmail 68943 invoked by alias); 26 Feb 2016 13:56:55 -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 68856 invoked by uid 89); 26 Feb 2016 13:56:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=Although, environ, **envp, 178 X-HELO: mail-yk0-f174.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references; bh=XAqwPfyWYbMMi2gbQ4wxI4BGseAlmgUoT67PGMCmbgE=; b=jXnRNu+OQif/BMQTjHdAnISl7I5NRtN7yG9r0oUHC6dBZ/gNUUBuKUfajuG9kU3Vsm eYXWHqGnIbPHB1lmEXIuczh+bDOQVtfxks4jlpH1a2yS9gsORiFuV+WPH8ebpl0L8rQZ 3Vr1B4Y2E61TDzeLrAOGjOF0/K6NrCH3zzKBCj1ar/X3fEpNTx+ap+V9n4sh73ls6mdl 8bdNnQHjbAtitIGk3lpbLlZs/PQU5HGO/Nn/Yae2Gch9gDuvR32c8NsSMG3EsK+mAOOX E5EXuTQhAAciz4yk00SNLZ9Xc/gdjukHmYQsR6GrOIInZ0gxWgXrvPPUmOoHxAEZF0UG Im1A== X-Gm-Message-State: AD7BkJJsantLJsBmC52YMGiDUJMXQoVblC0Q8O8m3Q6CuB8V+2CbpHGiE/vPBX3gaETWdYZY X-Received: by 10.37.230.204 with SMTP id d195mr843871ybh.134.1456495010759; Fri, 26 Feb 2016 05:56:50 -0800 (PST) From: Adhemerval Zanella To: libc-alpha@sourceware.org Subject: [PATCH 1/3] posix: Remove dynamic memory allocation from execl{e, p} Date: Fri, 26 Feb 2016 10:56:39 -0300 Message-Id: <1456495001-5298-2-git-send-email-adhemerval.zanella@linaro.org> In-Reply-To: <1456495001-5298-1-git-send-email-adhemerval.zanella@linaro.org> References: <1456495001-5298-1-git-send-email-adhemerval.zanella@linaro.org> GLIBC execl{e,p} implementation might use malloc if the total number of arguments exceed initial assumption size (1024). This might lead to issues in two situations: 1. execl/execle is stated to be async-signal-safe by POSIX [1]. However if execl is used in a signal handler with a large argument set (that may call malloc internally) and if the resulting call fails it might lead malloc in the program in a bad state. 2. If the functions are used in a vfork/clone(VFORK) situation it also might issue malloc internal bad state. This patch fixes it by using stack allocation instead. It also fixes BZ#19534. Tested on x86_64. [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html [BZ #19534] * posix/execl.c (execl): Remove dynamic memory allocation. * posix/execle.c (execle): Likewise. * posix/execlp.c (execlp): Likewise. --- posix/Makefile | 2 +- posix/execl.c | 69 ++++++++++++++++++++++----------------------------------- posix/execle.c | 70 +++++++++++++++++++++++----------------------------------- posix/execlp.c | 67 ++++++++++++++++++++++--------------------------------- 5 files changed, 89 insertions(+), 126 deletions(-) -- 1.9.1 diff --git a/posix/Makefile b/posix/Makefile index 4e90a95..55f4f31 100644 --- a/posix/Makefile +++ b/posix/Makefile @@ -221,8 +221,8 @@ CFLAGS-fexecve.os = -fomit-frame-pointer CFLAGS-execv.os = -fomit-frame-pointer CFLAGS-execle.os = -fomit-frame-pointer CFLAGS-execl.os = -fomit-frame-pointer -CFLAGS-execvp.os = -fomit-frame-pointer CFLAGS-execlp.os = -fomit-frame-pointer +CFLAGS-execvp.os = -fomit-frame-pointer tstgetopt-ARGS = -a -b -cfoobar --required foobar --optional=bazbug \ --none random --col --color --colour diff --git a/posix/execl.c b/posix/execl.c index 102d19d..5584a4c 100644 --- a/posix/execl.c +++ b/posix/execl.c @@ -16,58 +16,43 @@ . */ #include +#include #include -#include -#include -#include - -#include - +#include /* Execute PATH with all arguments after PATH until a NULL pointer and environment from `environ'. */ int execl (const char *path, const char *arg, ...) { -#define INITIAL_ARGV_MAX 1024 - size_t argv_max = INITIAL_ARGV_MAX; - const char *initial_argv[INITIAL_ARGV_MAX]; - const char **argv = initial_argv; - va_list args; - - argv[0] = arg; - - va_start (args, arg); - unsigned int i = 0; - while (argv[i++] != NULL) + int argc; + va_list ap; + va_start (ap, arg); + for (argc = 1; va_arg (ap, const char *); argc++) { - if (i == argv_max) + if (argc == INT_MAX) { - argv_max *= 2; - const char **nptr = realloc (argv == initial_argv ? NULL : argv, - argv_max * sizeof (const char *)); - if (nptr == NULL) - { - if (argv != initial_argv) - free (argv); - va_end (args); - return -1; - } - if (argv == initial_argv) - /* We have to copy the already filled-in data ourselves. */ - memcpy (nptr, argv, i * sizeof (const char *)); - - argv = nptr; + errno = E2BIG; + return -1; } - - argv[i] = va_arg (args, const char *); + continue; } - va_end (args); - - int ret = __execve (path, (char *const *) argv, __environ); - if (argv != initial_argv) - free (argv); - - return ret; + va_end (ap); + + /* Avoid dynamic memory allocation due two main issues: + 1. The function should be async-signal-safe and a running on a signal + handler with a fail outcome might lead to malloc bad state. + 2. It might be used in a vfork/clone(VFORK) scenario where using + malloc also might lead to internal bad state. */ + int i; + char *argv[argc + 1]; + va_start (ap, arg); + argv[0] = (char *) arg; + for (i = 1; i < argc; i++) + argv[i] = va_arg (ap, char *); + argv[i] = NULL; + va_end (ap); + + return __execve (path, argv, __environ); } libc_hidden_def (execl) diff --git a/posix/execle.c b/posix/execle.c index 8edc03a..66b5738 100644 --- a/posix/execle.c +++ b/posix/execle.c @@ -17,57 +17,43 @@ #include #include -#include -#include -#include - -#include +#include +#include /* Execute PATH with all arguments after PATH until a NULL pointer, and the argument after that for environment. */ int execle (const char *path, const char *arg, ...) { -#define INITIAL_ARGV_MAX 1024 - size_t argv_max = INITIAL_ARGV_MAX; - const char *initial_argv[INITIAL_ARGV_MAX]; - const char **argv = initial_argv; - va_list args; - argv[0] = arg; - - va_start (args, arg); - unsigned int i = 0; - while (argv[i++] != NULL) + int argc; + va_list ap; + va_start (ap, arg); + for (argc = 1; va_arg (ap, const char *); argc++) { - if (i == argv_max) + if (argc == INT_MAX) { - argv_max *= 2; - const char **nptr = realloc (argv == initial_argv ? NULL : argv, - argv_max * sizeof (const char *)); - if (nptr == NULL) - { - if (argv != initial_argv) - free (argv); - va_end (args); - return -1; - } - if (argv == initial_argv) - /* We have to copy the already filled-in data ourselves. */ - memcpy (nptr, argv, i * sizeof (const char *)); - - argv = nptr; + errno = E2BIG; + return -1; } - - argv[i] = va_arg (args, const char *); + continue; } - - const char *const *envp = va_arg (args, const char *const *); - va_end (args); - - int ret = __execve (path, (char *const *) argv, (char *const *) envp); - if (argv != initial_argv) - free (argv); - - return ret; + va_end (ap); + + /* Avoid dynamic memory allocation due two main issues: + 1. The function should be async-signal-safe and a running on a signal + handler with a fail outcome might lead to malloc bad state. + 2. It might be used in a vfork/clone(VFORK) scenario where using + malloc also might lead to internal bad state. */ + int i; + char *argv[argc + 1]; + char **envp; + va_start (ap, arg); + argv[0] = (char *) arg; + for (i = 1; i <= argc; i++) + argv[i] = va_arg (ap, char *); + envp = va_arg (ap, char **); + va_end (ap); + + return __execve (path, argv, envp); } libc_hidden_def (execle) diff --git a/posix/execlp.c b/posix/execlp.c index 6700994..7b84ffa 100644 --- a/posix/execlp.c +++ b/posix/execlp.c @@ -17,11 +17,8 @@ #include #include -#include -#include -#include - -#include +#include +#include /* Execute FILE, searching in the `PATH' environment variable if it contains no slashes, with all arguments after FILE until a @@ -29,45 +26,33 @@ int execlp (const char *file, const char *arg, ...) { -#define INITIAL_ARGV_MAX 1024 - size_t argv_max = INITIAL_ARGV_MAX; - const char *initial_argv[INITIAL_ARGV_MAX]; - const char **argv = initial_argv; - va_list args; - - argv[0] = arg; - - va_start (args, arg); - unsigned int i = 0; - while (argv[i++] != NULL) + int argc; + va_list ap; + va_start (ap, arg); + for (argc = 1; va_arg (ap, const char *); argc++) { - if (i == argv_max) + if (argc == INT_MAX) { - argv_max *= 2; - const char **nptr = realloc (argv == initial_argv ? NULL : argv, - argv_max * sizeof (const char *)); - if (nptr == NULL) - { - if (argv != initial_argv) - free (argv); - va_end (args); - return -1; - } - if (argv == initial_argv) - /* We have to copy the already filled-in data ourselves. */ - memcpy (nptr, argv, i * sizeof (const char *)); - - argv = nptr; + errno = E2BIG; + return -1; } - - argv[i] = va_arg (args, const char *); + continue; } - va_end (args); - - int ret = execvp (file, (char *const *) argv); - if (argv != initial_argv) - free (argv); - - return ret; + va_end (ap); + + /* Although posix does not state execlp as an async-safe function + it can not use malloc to allocate the arguments since it might + be used in a vfork scenario and it may lead to malloc internal + bad state. */ + int i; + char *argv[argc + 1]; + va_start (ap, arg); + argv[0] = (char *) arg; + for (i = 1; i < argc; i++) + argv[i] = va_arg (ap, char *); + argv[i] = NULL; + va_end (ap); + + return __execvpe (file, argv, __environ); } libc_hidden_def (execlp)