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

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

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