diff mbox

Fix segfault in maybe_script_execute.

Message ID 2ed9096b-245f-9ddb-164d-46e137568146@linux.ibm.com
State Committed
Headers

Commit Message

Stefan Liebler Sept. 6, 2018, 10:29 a.m. UTC 
  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

Comments

Andreas Schwab Sept. 6, 2018, 10:33 a.m. UTC | #1 
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.
Stefan Liebler Sept. 6, 2018, 12:30 p.m. UTC | #2 
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
Adhemerval Zanella Sept. 7, 2018, 7:57 a.m. UTC | #3 
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
>
Stefan Liebler Sept. 7, 2018, 10:33 a.m. UTC | #4 
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
>>
>
Adhemerval Zanella Sept. 7, 2018, 1:36 p.m. UTC | #5 
> 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.
Stefan Liebler Sept. 10, 2018, 12:32 p.m. UTC | #6 
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.
>
Adhemerval Zanella Sept. 10, 2018, 2:12 p.m. UTC | #7 
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.
>
diff mbox

Patch

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)