[PATCH] posix: Fix return value of system if shell can not be executed [BZ #27053]

[Adhemerval Zanella]

On 11/01/2021 07:01, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> POSIX states that system returned code for failure to execute the shell
>> shall be as if the shell had terminated using _exit(127).  This
>> behaviour was removed with 5fb7fc96350575.
>>
>> This issue should not be possible for pclose because if the shell can
>> not be executed, popen would not return a valid FILE object.  Other
>> process execution failures (such as killed by a signal) would be
>> returned through waitpid from pclose call.
> 
> Note that execve is not atomic and a late resource allocation failure
> will be reported as a termination by a signal (because the previous
> process image is gone at that point).

But afaiu the failure in shell execution by the posix_spawn will be
readily reported since execve would fail.  I think any late resource
failure allocation would be reported as pipe close (_IO_fileno (fp)).

Are you saying that spawn_process in libio/iopopen.c maybe return
true even if "sh" can not be executed?

> 
>> The new tst-system has an underlying issue once new containers tests
>> are added that might issue the minimal container shell, since it
>> changes the shell execution mode.  It is not an issue now, since it is
>> the only container tests for stdlib subforder, and I would like to
> 
> Typo: “subforder”
> 
>> avoid setting non parallel execution.  Another possibility if
>> concurrent container tests issues the minimum shell would be to
>> add a advisory lock.
> 
> I think you should replace “issue[s]” with different words in a few
> places.  Right now, the wording is quite confusing.
> 
> If the test is destructive to the testroot, I think there is already a
> way to mark it as such.

I simplified the commit message to just:
  
    POSIX states that system returned code for failure to execute the shell
    shall be as if the shell had terminated using _exit(127).  This
    behavior was removed with 5fb7fc96350575.
    
> 
>> ---
>>  stdlib/tst-system.c    | 17 +++++++++++++++++
>>  support/Makefile       |  1 +
>>  support/xchmod.c       | 30 ++++++++++++++++++++++++++++++
>>  support/xunistd.h      |  1 +
>>  sysdeps/posix/system.c |  4 ++++
>>  5 files changed, 53 insertions(+)
>>  create mode 100644 support/xchmod.c
> 
> support/ changes should be committed separately.
> 
>> diff --git a/support/xunistd.h b/support/xunistd.h
>> index b299db77ba..29b9a028b0 100644
>> --- a/support/xunistd.h
>> +++ b/support/xunistd.h
>> @@ -45,6 +45,7 @@ long xsysconf (int name);
>>  long long xlseek (int fd, long long offset, int whence);
>>  void xftruncate (int fd, long long length);
>>  void xsymlink (const char *target, const char *linkpath);
>> +void xchmod (const char *pathname, mode_t mode);
>>  
>>  /* Equivalent of "mkdir -p".  */
>>  void xmkdirp (const char *, mode_t);
>> diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c
>> index a03f478fc7..b86fc9b756 100644
>> --- a/sysdeps/posix/system.c
>> +++ b/sysdeps/posix/system.c
>> @@ -175,6 +175,10 @@ do_system (const char *line)
>>        __libc_cleanup_region_end (0);
>>  #endif
>>      }
>> +  else
>> +   /* POSIX states that failure to execute the shell should return
>> +      as if the shell had terminated using _exit(127).  */
>> +   status = W_EXITCODE (127, 0);
>>  
>>    DO_LOCK ();
>>    if (SUB_REF () == 0)
> 
> This still sets errno, but I think this is okay.
> 
> Thanks,
> Florian
>