| Message ID | 20230322180430.986512-1-josimmon@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | system: Add "--" after "-c" for sh (BZ #28519) | expand |
On 22/03/23 15:04, Joe Simmons-Talbott via Libc-alpha wrote: > Prevent sh from interpreting a user string as shell options if it > starts with '-' or '+'. Since the version of /bin/sh used for testing > system() is different from the full-fledged system /bin/sh add support > to it for handling "--" after "-c". Add a testcase to ensure the > expected behavior. Since https://austingroupbugs.net/view.php?id=1440 was accept, Florian remarks on BZ#27143 [1] (comment 1 and 3) does not apply anymore. However, although POSIX 2017 does state '--' as mark the end of the options, it seems that there are still shells that does not support it: $ /bin/csh -c -- "echo 123" --: Command not found. $ /bin/tcsh -c -- "echo 123" --: Command not found. (there are from ubuntu 22 packages) I am not sure if it should be ok to break such environments, at least there are available shells in some environments. The rest of the patch looks ok. [1] https://sourceware.org/bugzilla/show_bug.cgi?id=27143 > > Signed-off-by: Joe Simmons-Talbott <josimmon@redhat.com> > --- > libio/iopopen.c | 2 +- > stdlib/tst-system.c | 14 ++++++++++++++ > support/shell-container.c | 7 ++++++- > sysdeps/posix/system.c | 1 + > 4 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/libio/iopopen.c b/libio/iopopen.c > index d0545ad5ea..eef6d1ef18 100644 > --- a/libio/iopopen.c > +++ b/libio/iopopen.c > @@ -89,7 +89,7 @@ spawn_process (posix_spawn_file_actions_t *fa, FILE *fp, const char *command, > } > > err = __posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, fa, 0, > - (char *const[]){ (char*) "sh", (char*) "-c", > + (char *const[]){ (char*) "sh", (char*) "-c", (char*) "--", > (char *) command, NULL }, __environ); > if (err != 0) > return err; > diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c > index 47a0afe6bf..3a55ec2791 100644 > --- a/stdlib/tst-system.c > +++ b/stdlib/tst-system.c > @@ -146,6 +146,20 @@ do_test (void) > TEST_COMPARE_STRING (result.out.buffer, "...\n"); > } > > + { > + struct support_capture_subprocess result; > + const char *cmd = "-echo"; > + result = support_capture_subprocess (call_system, > + &(struct args) { cmd, 127 }); > + support_capture_subprocess_check (&result, "system", 0, sc_allow_stderr | > + sc_allow_stdout); > + char *returnerr = xasprintf ("%s: execing -echo failed: " > + "No such file or directory", > + basename(_PATH_BSHELL)); > + TEST_COMPARE_STRING (result.err.buffer, returnerr); > + free (returnerr); > + } > + > { > struct support_capture_subprocess result; > result = support_capture_subprocess (call_system, > diff --git a/support/shell-container.c b/support/shell-container.c > index b1f9e793c1..28437e4206 100644 > --- a/support/shell-container.c > +++ b/support/shell-container.c > @@ -455,7 +455,12 @@ main (int argc, const char **argv) > dprintf (stderr, " argv[%d] is `%s'\n", i, argv[i]); > > if (strcmp (argv[1], "-c") == 0) > - run_command_string (argv[2], argv+3); > + { > + if (strcmp (argv[2], "--") == 0) > + run_command_string (argv[3], argv+4); > + else > + run_command_string (argv[2], argv+3); > + } > else > run_script (argv[1], argv+2); > > diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c > index d77720a625..488b95163b 100644 > --- a/sysdeps/posix/system.c > +++ b/sysdeps/posix/system.c > @@ -147,6 +147,7 @@ do_system (const char *line) > ret = __posix_spawn (&pid, SHELL_PATH, 0, &spawn_attr, > (char *const[]){ (char *) SHELL_NAME, > (char *) "-c", > + (char *) "--", > (char *) line, NULL }, > __environ); > __posix_spawnattr_destroy (&spawn_attr);
On Mon, Mar 27, 2023 at 3:56 PM Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org> wrote: > I am not sure if it should be ok to break such environments, at least there are > available shells in some environments. Isn't a posix compliant shell already required as "/bin/sh" ? if it is not in writing..it is already de facto required. A lot of work has already been done to get basic components running with dash which has strict compliance as goals.
On Mon, Mar 27, 2023, at 3:34 PM, Cristian Rodríguez via Libc-alpha wrote: > On Mon, Mar 27, 2023 at 3:56 PM Adhemerval Zanella Netto via Libc- > alpha <libc-alpha@sourceware.org> wrote: > >> I am not sure if it should be ok to break such environments, at least >> there are available shells in some environments. > > Isn't a posix compliant shell already required as "/bin/sh" ? if it is > not in writing..it is already de facto required. I wouldn't go so far as to say that, _but_ setting a version of the C shell as /bin/sh is definitely an improper system configuration and will break tons of other things. > A lot of work has already been done to get basic components running > with dash which has strict compliance as goals. You're thinking about this patch from the wrong end of things; whether this is a safe change to make is not about other programs that might not work with _no more than_ the features specified by POSIX for /bin/sh. Rather, it is a question of whether glibc can safely assume availability of _all of_ the features specified by POSIX for /bin/sh, in particular unusual corners of command line option handling. I think it would be a good idea to test this patch on a system where /bin/sh is busybox sh, and one where /bin/sh is mksh. zw
On 27/03/23 16:50, Zack Weinberg via Libc-alpha wrote: > On Mon, Mar 27, 2023, at 3:34 PM, Cristian Rodríguez via Libc-alpha wrote: >> On Mon, Mar 27, 2023 at 3:56 PM Adhemerval Zanella Netto via Libc- >> alpha <libc-alpha@sourceware.org> wrote: >> >>> I am not sure if it should be ok to break such environments, at least >>> there are available shells in some environments. >> >> Isn't a posix compliant shell already required as "/bin/sh" ? if it is >> not in writing..it is already de facto required. > > I wouldn't go so far as to say that, _but_ setting a version of the C > shell as /bin/sh is definitely an improper system configuration and will > break tons of other things. > >> A lot of work has already been done to get basic components running >> with dash which has strict compliance as goals. > > You're thinking about this patch from the wrong end of things; whether > this is a safe change to make is not about other programs that might not > work with _no more than_ the features specified by POSIX for /bin/sh. > Rather, it is a question of whether glibc can safely assume availability > of _all of_ the features specified by POSIX for /bin/sh, in particular > unusual corners of command line option handling. > > I think it would be a good idea to test this patch on a system where > /bin/sh is busybox sh, and one where /bin/sh is mksh. > > zw Indeed and and I am leaning to make glibc assume that _PATH_BSHELL should support at least some minimum POSIX features, which includes '--' as mark the end of the options.
On Mär 27 2023, Cristian Rodríguez via Libc-alpha wrote: > On Mon, Mar 27, 2023 at 3:56 PM Adhemerval Zanella Netto via > Libc-alpha <libc-alpha@sourceware.org> wrote: > >> I am not sure if it should be ok to break such environments, at least there are >> available shells in some environments. > > Isn't a posix compliant shell already required as "/bin/sh" ? It must be Bourne-compatible anyway, so (t)csh is out.
On Mon, Mar 27, 2023 at 4:56 PM Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org> wrote: > Indeed and and I am leaning to make glibc assume that _PATH_BSHELL should > support at least some minimum POSIX features, which includes '--' as mark > the end of the options. Other than bash.. ash and dash are the other common default system shells. Either I am not old enough or inexperienced but I have never seen a system on which glibc runs using anything else as /bin/sh by *default* (as opposed to the user's favourite shell which can be anything)
On Mon, Mar 27, 2023 at 03:50:35PM -0400, Zack Weinberg via Libc-alpha wrote: > On Mon, Mar 27, 2023, at 3:34 PM, Cristian Rodríguez via Libc-alpha wrote: > > On Mon, Mar 27, 2023 at 3:56 PM Adhemerval Zanella Netto via Libc- > > alpha <libc-alpha@sourceware.org> wrote: > > > >> I am not sure if it should be ok to break such environments, at least > >> there are available shells in some environments. > > > > Isn't a posix compliant shell already required as "/bin/sh" ? if it is > > not in writing..it is already de facto required. > > I wouldn't go so far as to say that, _but_ setting a version of the C > shell as /bin/sh is definitely an improper system configuration and will > break tons of other things. > > > A lot of work has already been done to get basic components running > > with dash which has strict compliance as goals. > > You're thinking about this patch from the wrong end of things; whether > this is a safe change to make is not about other programs that might not > work with _no more than_ the features specified by POSIX for /bin/sh. > Rather, it is a question of whether glibc can safely assume availability > of _all of_ the features specified by POSIX for /bin/sh, in particular > unusual corners of command line option handling. > > I think it would be a good idea to test this patch on a system where > /bin/sh is busybox sh, and one where /bin/sh is mksh. I tested both busybox and mksh with the patch and they both accept the '--' after '-c' and behave as expected. Joe
On 28/03/23 09:52, Joe Simmons-Talbott via Libc-alpha wrote: > On Mon, Mar 27, 2023 at 03:50:35PM -0400, Zack Weinberg via Libc-alpha wrote: >> On Mon, Mar 27, 2023, at 3:34 PM, Cristian Rodríguez via Libc-alpha wrote: >>> On Mon, Mar 27, 2023 at 3:56 PM Adhemerval Zanella Netto via Libc- >>> alpha <libc-alpha@sourceware.org> wrote: >>> >>>> I am not sure if it should be ok to break such environments, at least >>>> there are available shells in some environments. >>> >>> Isn't a posix compliant shell already required as "/bin/sh" ? if it is >>> not in writing..it is already de facto required. >> >> I wouldn't go so far as to say that, _but_ setting a version of the C >> shell as /bin/sh is definitely an improper system configuration and will >> break tons of other things. >> >>> A lot of work has already been done to get basic components running >>> with dash which has strict compliance as goals. >> >> You're thinking about this patch from the wrong end of things; whether >> this is a safe change to make is not about other programs that might not >> work with _no more than_ the features specified by POSIX for /bin/sh. >> Rather, it is a question of whether glibc can safely assume availability >> of _all of_ the features specified by POSIX for /bin/sh, in particular >> unusual corners of command line option handling. >> >> I think it would be a good idea to test this patch on a system where >> /bin/sh is busybox sh, and one where /bin/sh is mksh. > > I tested both busybox and mksh with the patch and they both accept the > '--' after '-c' and behave as expected. > > Joe > As Andreas added, we already assume a Bourne-compatible shell. So the patch is ok, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
diff --git a/libio/iopopen.c b/libio/iopopen.c index d0545ad5ea..eef6d1ef18 100644 --- a/libio/iopopen.c +++ b/libio/iopopen.c @@ -89,7 +89,7 @@ spawn_process (posix_spawn_file_actions_t *fa, FILE *fp, const char *command, } err = __posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, fa, 0, - (char *const[]){ (char*) "sh", (char*) "-c", + (char *const[]){ (char*) "sh", (char*) "-c", (char*) "--", (char *) command, NULL }, __environ); if (err != 0) return err; diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c index 47a0afe6bf..3a55ec2791 100644 --- a/stdlib/tst-system.c +++ b/stdlib/tst-system.c @@ -146,6 +146,20 @@ do_test (void) TEST_COMPARE_STRING (result.out.buffer, "...\n"); } + { + struct support_capture_subprocess result; + const char *cmd = "-echo"; + result = support_capture_subprocess (call_system, + &(struct args) { cmd, 127 }); + support_capture_subprocess_check (&result, "system", 0, sc_allow_stderr | + sc_allow_stdout); + char *returnerr = xasprintf ("%s: execing -echo failed: " + "No such file or directory", + basename(_PATH_BSHELL)); + TEST_COMPARE_STRING (result.err.buffer, returnerr); + free (returnerr); + } + { struct support_capture_subprocess result; result = support_capture_subprocess (call_system, diff --git a/support/shell-container.c b/support/shell-container.c index b1f9e793c1..28437e4206 100644 --- a/support/shell-container.c +++ b/support/shell-container.c @@ -455,7 +455,12 @@ main (int argc, const char **argv) dprintf (stderr, " argv[%d] is `%s'\n", i, argv[i]); if (strcmp (argv[1], "-c") == 0) - run_command_string (argv[2], argv+3); + { + if (strcmp (argv[2], "--") == 0) + run_command_string (argv[3], argv+4); + else + run_command_string (argv[2], argv+3); + } else run_script (argv[1], argv+2); diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c index d77720a625..488b95163b 100644 --- a/sysdeps/posix/system.c +++ b/sysdeps/posix/system.c @@ -147,6 +147,7 @@ do_system (const char *line) ret = __posix_spawn (&pid, SHELL_PATH, 0, &spawn_attr, (char *const[]){ (char *) SHELL_NAME, (char *) "-c", + (char *) "--", (char *) line, NULL }, __environ); __posix_spawnattr_destroy (&spawn_attr);
Prevent sh from interpreting a user string as shell options if it starts with '-' or '+'. Since the version of /bin/sh used for testing system() is different from the full-fledged system /bin/sh add support to it for handling "--" after "-c". Add a testcase to ensure the expected behavior. Signed-off-by: Joe Simmons-Talbott <josimmon@redhat.com> --- libio/iopopen.c | 2 +- stdlib/tst-system.c | 14 ++++++++++++++ support/shell-container.c | 7 ++++++- sysdeps/posix/system.c | 1 + 4 files changed, 22 insertions(+), 2 deletions(-)