[PATCH] system: Add "--" after "-c" for sh (BZ #28519)

[PATCH] system: Add "--" after "-c" for sh (BZ #28519)

[Joe Simmons-Talbott]

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(-)

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);
-- 
2.39.2

Re: [PATCH] system: Add "--" after "-c" for sh (BZ #28519)

[Adhemerval Zanella Netto]

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);

Re: [PATCH] system: Add "--" after "-c" for sh (BZ #28519)

[Cristian Rodríguez]

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.

Re: [PATCH] system: Add "--" after "-c" for sh (BZ #28519)

[Zack Weinberg]

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

Re: [PATCH] system: Add "--" after "-c" for sh (BZ #28519)

[Adhemerval Zanella Netto]

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.

Re: [PATCH] system: Add "--" after "-c" for sh (BZ #28519)

[Cristian Rodríguez]

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)

Re: [PATCH] system: Add "--" after "-c" for sh (BZ #28519)

[Joe Simmons-Talbott]

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

Re: [PATCH] system: Add "--" after "-c" for sh (BZ #28519)

[Adhemerval Zanella Netto]

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>

Re: [PATCH] system: Add "--" after "-c" for sh (BZ #28519)

[Andreas Schwab]

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."