Re: [PATCH] Fix segfault in maybe_script_execute.

[Stefan Liebler <stli at linux dot ibm dot com>]

On 09/06/2018 11:14 AM, Andreas Schwab wrote:
> On Sep 06 2018, Stefan Liebler <stli@linux.ibm.com> wrote:
>
> > commit 6359e1c59f856a1115fdbcfa233052ac62e32c83
> > Author: Stefan Liebler <stli@linux.ibm.com>
> > Date:   Thu Sep 6 11:03:38 2018 +0200
> >
> >      Fix segfault in maybe_script_execute.
> >      
> >      If glibc is built with gcc 8 and -march=z900,
> >      the testcase posix/tst-spawn4-compat crashes with a segfault.
> >      
> >      In function maybe_script_execute, the new_argv array is dynamically
> >      initialized on stack with (argc + 1) elements.
> >      The function wants to add _PATH_BSHELL as the first argument
> >      and writes out of bounds of new_argv.
> >      
> >      In case of argc == 1, it writes three instead of two elements:
> >      new_argv[0] = (char *) _PATH_BSHELL;
> >      new_argv[1] = (char *) args->file;
> >      new_argv[2] = NULL;
> >      
> >      The latter write access writes to the same location where the
> >      pointer args is stored on stack.  Then it segfaults while accessing args:
> >      args->exec (new_argv[0], new_argv, args->envp);
> >      
> >      In case of argc > 1, new_argv[0] and new_argv[1] are set in the same
>
> This still make a difference between argc == 1 and argc > 1.  Just say
> that there is an off-by-one because maybe_script_execute fails to count
> the terminating NULL when sizing new_argv.
>
> Andreas.
What about this?

Stefan

commit 03a8c686eab8e12b4c478e7606963db6a72f6f0e
Author: Stefan Liebler <stli@linux.ibm.com>
Date:   Thu Sep 6 12:27:38 2018 +0200

    Fix segfault in maybe_script_execute.
    
    If glibc is built with gcc 8 and -march=z900,
    the testcase posix/tst-spawn4-compat crashes with a segfault.
    
    In function maybe_script_execute, the new_argv array is dynamically
    initialized on stack with (argc + 1) elements.
    The function wants to add _PATH_BSHELL as the first argument
    and writes out of bounds of new_argv.
    There is an off-by-one because maybe_script_execute fails to count
    the terminating NULL when sizing new_argv.
    
    ChangeLog:
    
            * sysdeps/unix/sysv/linux/spawni.c (maybe_script_execute):
            Increment size of new_argv by one.

diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index cf0213ece5..85239cedbf 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -101,7 +101,7 @@ maybe_script_execute (struct posix_spawn_args *args)
       ptrdiff_t argc = args->argc;
 
       /* Construct an argument list for the shell.  */
-      char *new_argv[argc + 1];
+      char *new_argv[argc + 2];
       new_argv[0] = (char *) _PATH_BSHELL;
       new_argv[1] = (char *) args->file;
       if (argc > 1)