| Message ID | 1454075599-2304-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 01/29/2016 02:53 PM, Adhemerval Zanella wrote: > GLIBC execl{e,p} implementation might use malloc if the total number of > arguments exceeds 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 it is used in a signal handler with a large argument set (that > may call malloc internally) and 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 break internal malloc state from parent. > > This patch fixes it by using stack allocation instead. It fixes > BZ#19534. I would rather see a variant of struct scratch_buffer which uses mmap for the fallback allocation, and use it here, rather than imposing arbitrary limits. It's not clear to me at all if __MAX_ALLOCA_CUTOFF is a reasonable value inside a signal handler. Florian
On Fri, Jan 29, 2016 at 8:53 AM, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > GLIBC execl{e,p} implementation might use malloc if the total number of > arguments exceeds initial assumption size (1024). This might lead to > issues in two situations: [...] I think it would help the discussion if you could outline the conditions under which these functions need to allocate memory in the first place. Does it happen all the time, or only when falling back to non-#! shell script invocation, or what? zw
On Fri, 29 Jan 2016, Florian Weimer wrote: > I would rather see a variant of struct scratch_buffer which uses mmap > for the fallback allocation, and use it here, rather than imposing > arbitrary limits. It's not clear to me at all if __MAX_ALLOCA_CUTOFF is Agreed. glibc functions should avoid arbitrary limits, which means they should not fail simply because the limit for stack allocation was exceeded. (This does mean the functions need to do deallocation of the memory allocated with mmap, if the underlying execve fails.)
> Em 29 de jan de 2016, às 13:00, Zack Weinberg <zackw@panix.com> escreveu: > > On Fri, Jan 29, 2016 at 8:53 AM, Adhemerval Zanella > <adhemerval.zanella@linaro.org> wrote: >> GLIBC execl{e,p} implementation might use malloc if the total number of >> arguments exceeds initial assumption size (1024). This might lead to >> issues in two situations: [...] > > I think it would help the discussion if you could outline the > conditions under which these functions need to allocate memory in the > first place. Does it happen all the time, or only when falling back > to non-#! shell script invocation, or what? > > zw Excel{e,p} first try to use a stack allocate buffer for up to 1024 arguments and try to allocate dynamic memory for large values (by trying to allocate memory to 2048 argument and doubling each time it does no fulfill the requirements). These functions does not try to run shell script with shebang (different than execpe).
> Em 29 de jan de 2016, às 11:58, Florian Weimer <fweimer@redhat.com> escreveu: > >> On 01/29/2016 02:53 PM, Adhemerval Zanella wrote: >> GLIBC execl{e,p} implementation might use malloc if the total number of >> arguments exceeds 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 it is used in a signal handler with a large argument set (that >> may call malloc internally) and 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 break internal malloc state from parent. >> >> This patch fixes it by using stack allocation instead. It fixes >> BZ#19534. > > I would rather see a variant of struct scratch_buffer which uses mmap > for the fallback allocation, and use it here, rather than imposing > arbitrary limits. It's not clear to me at all if __MAX_ALLOCA_CUTOFF is > a reasonable value inside a signal handler. > > Florian > I agree that __MAX_ALLOCA_CUTOFF is arbitrary and does not solve anything (for instance it does not impose constraints for deep stacked calls). Also mmap usage does not solve the second case (vfork/clone) in case of exec success since the call won't unmap the region.
On 29-01-2016 13:43, Joseph Myers wrote: > On Fri, 29 Jan 2016, Florian Weimer wrote: > >> I would rather see a variant of struct scratch_buffer which uses mmap >> for the fallback allocation, and use it here, rather than imposing >> arbitrary limits. It's not clear to me at all if __MAX_ALLOCA_CUTOFF is > > Agreed. glibc functions should avoid arbitrary limits, which means they > should not fail simply because the limit for stack allocation was > exceeded. > > (This does mean the functions need to do deallocation of the memory > allocated with mmap, if the underlying execve fails.) > The problem is the constraint associated with the functions where only stack allocation make sense. For all exec function family, where they can be called either through a signal handler or in vfork child, we can not really on dynamic memory allocation (either through malloc or directly by mmap). And I also see that __MAX_ALLOCA_CUTOFF is a limit that does not make any sense in this context: it is just an arbitrary value that will potentially limit the total function usability (total number of arguments). It also won't prevent stack overflow, since the call can be deeply nested and an allocation smaller than __MAX_ALLOCA_CUTOFF can trigger the overflow. My view is for such function is just try to allocate the buffer on stack and allow it fails through invalid access in case of buffer overflow.
On Fri, 29 Jan 2016, Adhemerval Zanella wrote: > My view is for such function is just try to allocate the buffer on > stack and allow it fails through invalid access in case of buffer > overflow. I'd say just trying the allocation is OK in this case (if stack allocation is all that's permitted by the function semantics, and bearing in mind that current Linux versions determine ARG_MAX dynamically based on the stack limit) *if* it's allocated in a way that guarantees failure if it's too large (i.e. touching each page in turn) rather than potentially overflowing into unrelated memory.
On 29-01-2016 15:45, Joseph Myers wrote: > On Fri, 29 Jan 2016, Adhemerval Zanella wrote: > >> My view is for such function is just try to allocate the buffer on >> stack and allow it fails through invalid access in case of buffer >> overflow. > > I'd say just trying the allocation is OK in this case (if stack allocation > is all that's permitted by the function semantics, and bearing in mind > that current Linux versions determine ARG_MAX dynamically based on the > stack limit) *if* it's allocated in a way that guarantees failure if it's > too large (i.e. touching each page in turn) rather than potentially > overflowing into unrelated memory. > That is the idea, the buffer is allocated to exactly fit the required arguments (different that current algorithm that double each iteration). For posix_spawn{p} I used a different strategy for the clone(VFORK), which I think it is slight better but does impose some constraints: allocate a mmap(STACK) with default architecture stack size and set is as the stack for children. I think it is better than possible clobber a stack allocated buffer from parent.
On 01/29/2016 04:52 PM, Adhemerval Zanella wrote:
> Also mmap usage does not solve the second case (vfork/clone) in case of exec success since the call won't unmap the region.
I've written a test case to confirm this. How unfortunate.
Are there different clone flags we could use? Probably not, because the
goal is to share the page tables.
For posix_spawn, this doesn't really matter because posix_spawn could
allocate and deallocate the mapping.
Florian
diff --git a/posix/execl.c b/posix/execl.c index 102d19d..26c28e1 100644 --- a/posix/execl.c +++ b/posix/execl.c @@ -16,58 +16,36 @@ <http://www.gnu.org/licenses/>. */ #include <unistd.h> +#include <errno.h> #include <stdarg.h> -#include <stddef.h> -#include <stdlib.h> -#include <string.h> - -#include <stackinfo.h> - +#include <sys/param.h> /* 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) - { - if (i == argv_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; - } - - argv[i] = va_arg (args, const char *); - } - va_end (args); - - int ret = __execve (path, (char *const *) argv, __environ); - if (argv != initial_argv) - free (argv); - - return ret; + int argc; + int limit = MIN (NCARGS, __MAX_ALLOCA_CUTOFF / sizeof (char *)); + va_list ap; + va_start (ap, arg); + for (argc = 1; va_arg (ap, const char *); argc++) + if (argc+1 >= limit) + { + errno = E2BIG; + return -1; + } + va_end (ap); + + 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..79c13e3 100644 --- a/posix/execle.c +++ b/posix/execle.c @@ -17,57 +17,36 @@ #include <unistd.h> #include <stdarg.h> -#include <stddef.h> -#include <stdlib.h> -#include <string.h> - -#include <stackinfo.h> +#include <errno.h> +#include <sys/param.h> /* 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) - { - if (i == argv_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; - } - - argv[i] = va_arg (args, const char *); - } - - 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; + int argc; + int limit = MIN (NCARGS, __MAX_ALLOCA_CUTOFF / sizeof (char *)); + va_list ap; + va_start (ap, arg); + for (argc = 1; va_arg (ap, const char *); argc++) + if (argc+1 >= limit) + { + errno = E2BIG; + return -1; + } + va_end (ap); + + 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..a4b603c 100644 --- a/posix/execlp.c +++ b/posix/execlp.c @@ -17,11 +17,8 @@ #include <unistd.h> #include <stdarg.h> -#include <stddef.h> -#include <stdlib.h> -#include <string.h> - -#include <stackinfo.h> +#include <errno.h> +#include <sys/param.h> /* Execute FILE, searching in the `PATH' environment variable if it contains no slashes, with all arguments after FILE until a @@ -29,45 +26,27 @@ 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) - { - if (i == argv_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; - } - - argv[i] = va_arg (args, const char *); - } - va_end (args); - - int ret = execvp (file, (char *const *) argv); - if (argv != initial_argv) - free (argv); - - return ret; + int argc; + int limit = MIN (NCARGS, __MAX_ALLOCA_CUTOFF / sizeof (char *)); + va_list ap; + va_start (ap, arg); + for (argc = 1; va_arg (ap, const char *); argc++) + if (argc+1 >= limit) + { + errno = E2BIG; + return -1; + } + va_end (ap); + + 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)
GLIBC execl{e,p} implementation might use malloc if the total number of arguments exceeds 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 it is used in a signal handler with a large argument set (that may call malloc internally) and 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 break internal malloc state from parent. This patch fixes it by using stack allocation instead. It fixes BZ#19534. One caveat is current stack allocation allocation limit the patch is using is based on previous posix_spawn{p}/execvpe comments to use internal __MAX_ALLOCA_CUTOFF definition. It is an arbitrary value that limits total argument handlings to 8192 for 32-bits and 4096 for 64-bits. I think it would be better to increase this value for higher threshold, either by increasing the __MAX_ALLOCA_CUTOFF value or using a different strategy to limit stack allocation on exec functions. 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. --- ChangeLog | 5 +++++ posix/execl.c | 70 ++++++++++++++++++++------------------------------------- posix/execle.c | 71 +++++++++++++++++++++------------------------------------- posix/execlp.c | 69 ++++++++++++++++++++------------------------------------ 4 files changed, 78 insertions(+), 137 deletions(-)