Message ID | 20201211190459.1128661-1-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | posix: Fix return value of system if shell can not be executed [BZ #27053] | expand |
* 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
diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c index eddea33f4c..f477aeb267 100644 --- a/stdlib/tst-system.c +++ b/stdlib/tst-system.c @@ -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; 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); +} 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)