posix: Fix return value of system if shell can not be executed [BZ #27053]
Commit Message
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.
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
avoid setting non parallel execution. Another possibility if
concurrent container tests issues the minimum shell would be to
add a advisory lock.
Checked on x86_64-linux-gnu.
---
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
Comments
* Adhemerval Zanella via Libc-alpha:
> diff --git a/support/Makefile b/support/Makefile
> index f5f59bf8d2..c89b16bbcb 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -90,6 +90,7 @@ libsupport-routines = \
> xchroot \
> xclock_gettime \
> xclose \
> + xchmod \
> xconnect \
> xcopy_file_range \
> xdlfcn \
> diff --git a/support/xchmod.c b/support/xchmod.c
> new file mode 100644
> index 0000000000..5e403c7cc2
> --- /dev/null
> +++ b/support/xchmod.c
> @@ -0,0 +1,30 @@
> +/* chmod with error checking.
> + Copyright (C) 2020 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 <support/xunistd.h>
> +#include <support/check.h>
> +
> +#include <sys/stat.h>
> +
> +void
> +xchmod (const char *pathname, mode_t mode)
> +{
> + int r = chmod (pathname, mode);
> + if (r < 0)
> + FAIL_EXIT1 ("chmod (%s, %d): %m", pathname, mode);
Please use 0%o. Thanks. 8-)
> +}
> 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);
Okay with the above change in a separate commit.
Florian
* 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).
> 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.
> ---
> 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
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
>
* 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?
It can report process termination by a signal, whatever that means in
the context of system.
Because of the non-atomic nature of execve, there is a point where the
kernel cannot return ENOMEM anymore, and has to send a signal to the
process to terminate it. Then the system function will have a PID for
that failed process, yet execve has not succeeded in any meaningful
sense of the word.
Thanks,
Florian
On 11/01/2021 10:50, Florian Weimer wrote:
> * 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?
>
> It can report process termination by a signal, whatever that means in
> the context of system.
>
> Because of the non-atomic nature of execve, there is a point where the
> kernel cannot return ENOMEM anymore, and has to send a signal to the
> process to terminate it. Then the system function will have a PID for
> that failed process, yet execve has not succeeded in any meaningful
> sense of the word.
But in this case the issue will be independently whether we use
posix_spawn or fork, right?
* Adhemerval Zanella:
> On 11/01/2021 10:50, Florian Weimer wrote:
>> * 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?
>>
>> It can report process termination by a signal, whatever that means in
>> the context of system.
>>
>> Because of the non-atomic nature of execve, there is a point where the
>> kernel cannot return ENOMEM anymore, and has to send a signal to the
>> process to terminate it. Then the system function will have a PID for
>> that failed process, yet execve has not succeeded in any meaningful
>> sense of the word.
>
> But in this case the issue will be independently whether we use
> posix_spawn or fork, right?
Yes, it's just the way how Linux execve behaves.
Thanks,
Florian
@@ -26,6 +26,7 @@
#include <support/check.h>
#include <support/temp_file.h>
#include <support/support.h>
+#include <support/xunistd.h>
static char *tmpdir;
static long int namemax;
@@ -138,6 +139,22 @@ do_test (void)
support_capture_subprocess_check (&result, "system", 0, sc_allow_none);
}
+ {
+ struct stat64 st;
+ xstat (_PATH_BSHELL, &st);
+ mode_t mode = st.st_mode;
+ xchmod (_PATH_BSHELL, mode & ~(S_IXUSR | S_IXGRP | S_IXOTH));
+
+ struct support_capture_subprocess result;
+ result = support_capture_subprocess (call_system,
+ &(struct args) {
+ "exit 1", 127, 0
+ });
+ support_capture_subprocess_check (&result, "system", 0, sc_allow_none);
+
+ xchmod (_PATH_BSHELL, st.st_mode);
+ }
+
TEST_COMPARE (system (""), 0);
return 0;
@@ -90,6 +90,7 @@ libsupport-routines = \
xchroot \
xclock_gettime \
xclose \
+ xchmod \
xconnect \
xcopy_file_range \
xdlfcn \
new file mode 100644
@@ -0,0 +1,30 @@
+/* chmod with error checking.
+ Copyright (C) 2020 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 <support/xunistd.h>
+#include <support/check.h>
+
+#include <sys/stat.h>
+
+void
+xchmod (const char *pathname, mode_t mode)
+{
+ int r = chmod (pathname, mode);
+ if (r < 0)
+ FAIL_EXIT1 ("chmod (%s, %d): %m", pathname, mode);
+}
@@ -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);
@@ -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)