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

[Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org>]

Ping (x2).

On 10/01/23 12:21, Adhemerval Zanella Netto wrote:
> 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>