Message ID | 20221207182438.172691-1-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | [v2] stdio: Do not ignore posix_spawn error on popen (BZ #29016) | expand |
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>
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>
I've already ok'd it 9 weeks ago.
On 14/02/23 12:13, Andreas Schwab wrote: > I've already ok'd it 9 weeks ago. > I haven't seeing, thanks for the heads up.
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>