diff mbox

Fix segfault in maybe_script_execute.

Message ID 6fab031c-9748-42e4-7137-55bf9ae59545@linux.ibm.com
State Superseded
Headers

Commit Message

Stefan Liebler Sept. 5, 2018, 1:38 p.m. UTC 
  Hi,

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.
But if 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);

If argc > 1, the arguments inclusive the NULL termination of new_argv
is copied from argv with memcpy.

The implementation assumes that argc == 0 will never happen!

Okay to commit?

Bye
Stefan

---

ChangeLog:

	* sysdeps/unix/sysv/linux/spawni.c (maybe_script_execute):
	Increment size of new_argv by one.

Comments

Andreas Schwab Sept. 5, 2018, 1:44 p.m. UTC | #1 
On Sep 05 2018, Stefan Liebler <stli@linux.ibm.com> wrote:

> If argc > 1, the arguments inclusive the NULL termination of new_argv
> is copied from argv with memcpy.

But it has the same bug, doesn't it?

Andreas.
Stefan Liebler Sept. 6, 2018, 8:36 a.m. UTC | #2 
On 09/05/2018 03:44 PM, Andreas Schwab wrote:
> On Sep 05 2018, Stefan Liebler <stli@linux.ibm.com> wrote:
> 
>> If argc > 1, the arguments inclusive the NULL termination of new_argv
>> is copied from argv with memcpy.
> 
> But it has the same bug, doesn't it?
> 
> Andreas.
> 
Yes. It also has this bug.

Stefan
Andreas Schwab Sept. 6, 2018, 8:50 a.m. UTC | #3 
On Sep 06 2018, Stefan Liebler <stli@linux.ibm.com> wrote:

> On 09/05/2018 03:44 PM, Andreas Schwab wrote:
>> On Sep 05 2018, Stefan Liebler <stli@linux.ibm.com> wrote:
>>
>>> If argc > 1, the arguments inclusive the NULL termination of new_argv
>>> is copied from argv with memcpy.
>>
>> But it has the same bug, doesn't it?
>>
>> Andreas.
>>
> Yes. It also has this bug.

This makes the description rather confusing, implying there is a
difference between argc == 1 and argc > 1, which isn't.

Andreas.
diff mbox

Patch

commit d099212b378f23fef38ee3f51168bb247d5f719d
Author: Stefan Liebler <stli@linux.ibm.com>
Date:   Wed Sep 5 13:34:44 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.
    But if 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);
    
    If argc > 1, the arguments inclusive the NULL termination of new_argv
    is copied from argv with memcpy.
    
    The implementation assumes that argc == 0 will never happen!
    
    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)