[PATCH] Fix segfault in maybe_script_execute.

[PATCH] Fix segfault in maybe_script_execute.

[Stefan Liebler]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

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.

[-- Attachment #2: 20180905_maybe_script_execute.patch --]
[-- Type: text/x-patch, Size: 1721 bytes --]

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)

Re: [PATCH] Fix segfault in maybe_script_execute.

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix segfault in maybe_script_execute.

[Stefan Liebler]

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

Re: [PATCH] Fix segfault in maybe_script_execute.

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix segfault in maybe_script_execute.

[Stefan Liebler]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

On 09/06/2018 10:50 AM, Andreas Schwab wrote:
> 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.
> 
Okay. I've adjusted the description.

Thanks.
Stefan

[-- Attachment #2: 20180906_maybe_script_execute.patch --]
[-- Type: text/x-patch, Size: 1955 bytes --]

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
    way as in the case above and the arguments inclusive the NULL termination
    of new_argv is copied from argv with memcpy.
    Note: The last copied element (NULL termination) is written out of
    bounds of new_argv.
    
    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)

Re: [PATCH] Fix segfault in maybe_script_execute.

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix segfault in maybe_script_execute.

[Stefan Liebler]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

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

[-- Attachment #2: 20180906_1230_maybe_script_execute.patch --]
[-- Type: text/x-patch, Size: 1326 bytes --]

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)

Re: [PATCH] Fix segfault in maybe_script_execute.

[Andreas Schwab]

On Sep 06 2018, Stefan Liebler <stli@linux.ibm.com> wrote:

> 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.

Ok, thanks.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix segfault in maybe_script_execute.

[Stefan Liebler]

On 09/06/2018 12:33 PM, Andreas Schwab wrote:
> On Sep 06 2018, Stefan Liebler <stli@linux.ibm.com> wrote:
> 
>> 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.
> 
> Ok, thanks.
> 
> Andreas.
> 

Committed.

Thanks.
Stefan

Re: [PATCH] Fix segfault in maybe_script_execute.

[Adhemerval Zanella]

Thanks for the patch, could you check if we need to backport this fix to older releases as well? 

Inviato da iPhone

> Il giorno 06 set 2018, alle ore 13:30, Stefan Liebler <stli@linux.ibm.com> ha scritto:
> 
>> On 09/06/2018 12:33 PM, Andreas Schwab wrote:
>>> On Sep 06 2018, Stefan Liebler <stli@linux.ibm.com> wrote:
>>> 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.
>> Ok, thanks.
>> Andreas.
> 
> Committed.
> 
> Thanks.
> Stefan
> 

Re: [PATCH] Fix segfault in maybe_script_execute.

[Stefan Liebler]

On 09/07/2018 09:57 AM, Adhemerval Zanella wrote:
> Thanks for the patch, could you check if we need to backport this fix to older releases as well?
> 
As far as I've seen, maybe_script_execute was introduced with commit 
"posix: New Linux posix_spawn{p} implementation" 
(https://sourceware.org/git/?p=glibc.git;a=commit;h=9ff72da471a509a8c19791efe469f47fa6977410)
=> first release was glibc 2.24.

Should it be backported to all versions: glibc 2.24 ... 2.28?

Bye
Stefan

> Inviato da iPhone
> 
>> Il giorno 06 set 2018, alle ore 13:30, Stefan Liebler <stli@linux.ibm.com> ha scritto:
>>
>>> On 09/06/2018 12:33 PM, Andreas Schwab wrote:
>>>> On Sep 06 2018, Stefan Liebler <stli@linux.ibm.com> wrote:
>>>> 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.
>>> Ok, thanks.
>>> Andreas.
>>
>> Committed.
>>
>> Thanks.
>> Stefan
>>
> 

Re: [PATCH] Fix segfault in maybe_script_execute.

[Adhemerval Zanella]

> Il giorno 07 set 2018, alle ore 11:33, Stefan Liebler <stli@linux.ibm.com> ha scritto:
> 
>> On 09/07/2018 09:57 AM, Adhemerval Zanella wrote:
>> Thanks for the patch, could you check if we need to backport this fix to older releases as well?
> As far as I've seen, maybe_script_execute was introduced with commit "posix: New Linux posix_spawn{p} implementation" (https://sourceware.org/git/?p=glibc.git;a=commit;h=9ff72da471a509a8c19791efe469f47fa6977410)
> => first release was glibc 2.24.
> 
> Should it be backported to all versions: glibc 2.24 ... 2.28?
> 

Ideally yes, this is quite simple patch so I thin backport should not require extra work.

[COMMITTED 2.24 / 2.25 / 2.26 / 2.27 / 2.28] Fix segfault in maybe_script_execute.

[Stefan Liebler]

Hi,

I've cherry picked the commit "Fix segfault in maybe_script_execute." 
(https://sourceware.org/git/?p=glibc.git;a=commit;h=28669f86f6780a18daca264f32d66b1428c9c6f1) 
and it is now available on glibc release branches 2.24 ... 2.28.

Bye
Stefan

On 09/07/2018 03:36 PM, Adhemerval Zanella wrote:
> 
> 
>> Il giorno 07 set 2018, alle ore 11:33, Stefan Liebler <stli@linux.ibm.com> ha scritto:
>>
>>> On 09/07/2018 09:57 AM, Adhemerval Zanella wrote:
>>> Thanks for the patch, could you check if we need to backport this fix to older releases as well?
>> As far as I've seen, maybe_script_execute was introduced with commit "posix: New Linux posix_spawn{p} implementation" (https://sourceware.org/git/?p=glibc.git;a=commit;h=9ff72da471a509a8c19791efe469f47fa6977410)
>> => first release was glibc 2.24.
>>
>> Should it be backported to all versions: glibc 2.24 ... 2.28?
>>
> 
> Ideally yes, this is quite simple patch so I thin backport should not require extra work.
> 

Re: [COMMITTED 2.24 / 2.25 / 2.26 / 2.27 / 2.28] Fix segfault in maybe_script_execute.

[Adhemerval Zanella]

Tyvm.

Inviato da iPhone

> Il giorno 10 set 2018, alle ore 13:32, Stefan Liebler <stli@linux.ibm.com> ha scritto:
> 
> Hi,
> 
> I've cherry picked the commit "Fix segfault in maybe_script_execute." (https://sourceware.org/git/?p=glibc.git;a=commit;h=28669f86f6780a18daca264f32d66b1428c9c6f1) and it is now available on glibc release branches 2.24 ... 2.28.
> 
> Bye
> Stefan
> 
> On 09/07/2018 03:36 PM, Adhemerval Zanella wrote:
>>> Il giorno 07 set 2018, alle ore 11:33, Stefan Liebler <stli@linux.ibm.com> ha scritto:
>>> 
>>>> On 09/07/2018 09:57 AM, Adhemerval Zanella wrote:
>>>> Thanks for the patch, could you check if we need to backport this fix to older releases as well?
>>> As far as I've seen, maybe_script_execute was introduced with commit "posix: New Linux posix_spawn{p} implementation" (https://sourceware.org/git/?p=glibc.git;a=commit;h=9ff72da471a509a8c19791efe469f47fa6977410)
>>> => first release was glibc 2.24.
>>> 
>>> Should it be backported to all versions: glibc 2.24 ... 2.28?
>>> 
>> Ideally yes, this is quite simple patch so I thin backport should not require extra work.
>