Re: [PATCH v2] stdio: Do not ignore posix_spawn error on popen (BZ #29016)

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

Ping.

On 07/12/22 15:24, Adhemerval Zanella wrote:
> To correctly return error in case of default shell is not present.
> 
> Checked on x86_64-linux-gnu.
> ---
>  libio/iopopen.c           | 38 ++++++++++++++++++++++----------------
>  stdio-common/Makefile     |  3 +++
>  stdio-common/tst-popen3.c | 38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 16 deletions(-)
>  create mode 100644 stdio-common/tst-popen3.c
> 
> diff --git a/libio/iopopen.c b/libio/iopopen.c
> index 06778cf110..66f5936114 100644
> --- a/libio/iopopen.c
> +++ b/libio/iopopen.c
> @@ -66,11 +66,12 @@ unlock (void *not_used)
>     be close (by transversing the proc_file_chain list) and the insertion of a
>     new one after a successful posix_spawn this function should be called
>     with proc_file_chain_lock acquired.  */
> -static bool
> +static int
>  spawn_process (posix_spawn_file_actions_t *fa, FILE *fp, const char *command,
>  	       int do_cloexec, int pipe_fds[2], int parent_end, int child_end,
>  	       int child_pipe_fd)
>  {
> +  int err;
>  
>    for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next)
>      {
> @@ -79,15 +80,19 @@ spawn_process (posix_spawn_file_actions_t *fa, FILE *fp, const char *command,
>        /* If any stream from previous popen() calls has fileno
>  	 child_pipe_fd, it has been already closed by the adddup2 action
>  	 above.  */
> -      if (fd != child_pipe_fd
> -	  && __posix_spawn_file_actions_addclose (fa, fd) != 0)
> -	return false;
> +      if (fd != child_pipe_fd)
> +	{
> +	  err = __posix_spawn_file_actions_addclose (fa, fd);
> +	  if (err != 0)
> +	    return err;
> +	}
>      }
>  
> -  if (__posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, fa, 0,
> -		     (char *const[]){ (char*) "sh", (char*) "-c",
> -		     (char *) command, NULL }, __environ) != 0)
> -    return false;
> +  err = __posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, fa, 0,
> +		       (char *const[]){ (char*) "sh", (char*) "-c",
> +		       (char *) command, NULL }, __environ);
> +  if (err != 0)
> +    return err;
>  
>    __close_nocancel (pipe_fds[child_end]);
>  
> @@ -101,7 +106,7 @@ spawn_process (posix_spawn_file_actions_t *fa, FILE *fp, const char *command,
>    ((_IO_proc_file *) fp)->next = proc_file_chain;
>    proc_file_chain = (_IO_proc_file *) fp;
>  
> -  return true;
> +  return 0;
>  }
>  
>  FILE *
> @@ -112,7 +117,7 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>    int parent_end, child_end;
>    int pipe_fds[2];
>    int child_pipe_fd;
> -  bool spawn_ok;
> +  int err;
>  
>    int do_read = 0;
>    int do_write = 0;
> @@ -185,16 +190,17 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>        pipe_fds[child_end] = tmp;
>      }
>  
> -  if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[child_end],
> -      child_pipe_fd) != 0)
> +  err = __posix_spawn_file_actions_adddup2 (&fa, pipe_fds[child_end],
> +					    child_pipe_fd);
> +  if (err != 0)
>      goto spawn_failure;
>  
>  #ifdef _IO_MTSAFE_IO
>    _IO_cleanup_region_start_noarg (unlock);
>    _IO_lock_lock (proc_file_chain_lock);
>  #endif
> -  spawn_ok = spawn_process (&fa, fp, command, do_cloexec, pipe_fds,
> -			    parent_end, child_end, child_pipe_fd);
> +  err = spawn_process (&fa, fp, command, do_cloexec, pipe_fds, parent_end,
> +		       child_end, child_pipe_fd);
>  #ifdef _IO_MTSAFE_IO
>    _IO_lock_unlock (proc_file_chain_lock);
>    _IO_cleanup_region_end (0);
> @@ -202,12 +208,12 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>  
>    __posix_spawn_file_actions_destroy (&fa);
>  
> -  if (!spawn_ok)
> +  if (err != 0)
>      {
> +      __set_errno (err);
>      spawn_failure:
>        __close_nocancel (pipe_fds[child_end]);
>        __close_nocancel (pipe_fds[parent_end]);
> -      __set_errno (ENOMEM);
>        return NULL;
>      }
>  
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index fe57dbdf56..9a9a37ae08 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -215,6 +215,9 @@ tests := \
>    xbug \
>    # tests
>  
> +tests-container += \
> +  tst-popen3
> +
>  generated += \
>    errlist-data-aux-shared.S \
>    errlist-data-aux.S \
> diff --git a/stdio-common/tst-popen3.c b/stdio-common/tst-popen3.c
> new file mode 100644
> index 0000000000..47f4c8cd91
> --- /dev/null
> +++ b/stdio-common/tst-popen3.c
> @@ -0,0 +1,38 @@
> +/* Check if popen returns a correct error code if the default shell does not
> +   exist (BZ#29016).
> +
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <paths.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <support/check.h>
> +#include <support/xunistd.h>
> +
> +int
> +do_test (void)
> +{
> +  xunlink (_PATH_BSHELL);
> +
> +  FILE *f = popen ("/non-existent", "r");
> +  TEST_VERIFY (f == NULL);
> +  TEST_COMPARE (errno, ENOENT);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>