| Message ID | 6fab031c-9748-42e4-7137-55bf9ae59545@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix segfault in maybe_script_execute. | expand |
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.
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
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.
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 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)
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.
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)
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.
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
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 >
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 >> >
> 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.
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. >
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. >
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)