Message ID | 20200323183109.16201-1-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | posix: Fix system error return value [BZ #25715] | expand |
On 3/23/20 2:31 PM, Adhemerval Zanella via Libc-alpha wrote: > It fixes 5fb7fc9635 when posix_spawn fails. > > Checked on x86_64-linux-gnu and i686-linux-gnu. LGTM. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > --- > stdlib/tst-system.c | 122 +++++++++++++++++++++++++++++++++++++++-- > sysdeps/posix/system.c | 18 +++--- > 2 files changed, 129 insertions(+), 11 deletions(-) > > diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c > index b6c5aea08f..04c87a1c6b 100644 > --- a/stdlib/tst-system.c > +++ b/stdlib/tst-system.c > @@ -17,14 +17,128 @@ > <https://www.gnu.org/licenses/>. */ > > #include <stdlib.h> > +#include <unistd.h> > +#include <string.h> > +#include <signal.h> > +#include <paths.h> > OK. > +#include <support/capture_subprocess.h> > +#include <support/check.h> > +#include <support/temp_file.h> > +#include <support/support.h> > + > +static char *tmpdir; > +static long int namemax; > + > +static void > +do_prepare (int argc, char *argv[]) > +{ > + tmpdir = support_create_temp_directory ("tst-system-"); > + /* Include the last '/0'. */ > + namemax = pathconf (tmpdir, _PC_NAME_MAX) + 1; > + TEST_VERIFY_EXIT (namemax != -1); > +} > +#define PREPARE do_prepare > + > +struct args > +{ > + const char *command; > + int exit_status; > + int term_sig; > + const char *path; > +}; > + > +static void > +call_system (void *closure) > +{ > + struct args *args = (struct args *) closure; > + int ret; > + > + if (args->path != NULL) > + TEST_COMPARE (setenv ("PATH", args->path, 1), 0); > + ret = system (args->command); > + if (args->term_sig == 0) > + { > + /* Expect regular termination. */ > + TEST_COMPARE (WIFEXITED (ret), 1); > + TEST_COMPARE (WEXITSTATUS (ret), args->exit_status); > + } > + else > + { > + /* status_or_signal < 0. Expect termination by signal. */ > + TEST_COMPARE (WIFSIGNALED (ret), 1); > + TEST_COMPARE (WTERMSIG (ret), args->term_sig); > + } > +} > > static int > do_test (void) > { > - return system (":"); > -} > + TEST_VERIFY (system (NULL) != 0); > > + { > + char cmd[namemax]; > + memset (cmd, 'a', sizeof(cmd)); > + cmd[sizeof(cmd) - 1] = '\0'; > + > + struct support_capture_subprocess result; > + result = support_capture_subprocess (call_system, > + &(struct args) { > + cmd, 127, 0, tmpdir > + }); > + support_capture_subprocess_check (&result, "system", 0, sc_allow_stderr); > + > + char *returnerr = xasprintf ("%s: 1: %s: not found\n", > + basename(_PATH_BSHELL), cmd); > + TEST_COMPARE_STRING (result.err.buffer, returnerr); OK. > + free (returnerr); > + } > + > + { > + char cmd[namemax + 1]; > + memset (cmd, 'a', sizeof(cmd)); > + cmd[sizeof(cmd) - 1] = '\0'; > + > + struct support_capture_subprocess result; > + result = support_capture_subprocess (call_system, > + &(struct args) { > + cmd, 127, 0, tmpdir > + }); > + support_capture_subprocess_check (&result, "system", 0, sc_allow_stderr); > + > + char *returnerr = xasprintf ("%s: 1: %s: File name too long\n", > + basename(_PATH_BSHELL), cmd); > + TEST_COMPARE_STRING (result.err.buffer, returnerr); OK. > + free (returnerr); > + } > + > + { > + struct support_capture_subprocess result; > + result = support_capture_subprocess (call_system, > + &(struct args) { > + "kill -USR1 $$", 0, SIGUSR1 > + }); > + support_capture_subprocess_check (&result, "system", 0, sc_allow_none); > + } > + > + { > + struct support_capture_subprocess result; > + result = support_capture_subprocess (call_system, > + &(struct args) { "echo ...", 0 }); > + support_capture_subprocess_check (&result, "system", 0, sc_allow_stdout); > + TEST_COMPARE_STRING (result.out.buffer, "...\n"); > + } > + > + { > + struct support_capture_subprocess result; > + result = support_capture_subprocess (call_system, > + &(struct args) { "exit 1", 1 }); > + support_capture_subprocess_check (&result, "system", 0, sc_allow_none); > + } > + > + TEST_COMPARE (system (":"), 0); > + > + return 0; > +} > > -#define TEST_FUNCTION do_test () > -#include "../test-skeleton.c" > +#include <support/test-driver.c> > diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c > index e613e6a344..a03f478fc7 100644 > --- a/sysdeps/posix/system.c > +++ b/sysdeps/posix/system.c > @@ -101,7 +101,8 @@ cancel_handler (void *arg) > static int > do_system (const char *line) > { > - int status; > + int status = -1; > + int ret; > pid_t pid; > struct sigaction sa; > #ifndef _LIBC_REENTRANT > @@ -144,14 +145,14 @@ do_system (const char *line) > __posix_spawnattr_setflags (&spawn_attr, > POSIX_SPAWN_SETSIGDEF | POSIX_SPAWN_SETSIGMASK); > > - status = __posix_spawn (&pid, SHELL_PATH, 0, &spawn_attr, > - (char *const[]){ (char*) SHELL_NAME, > - (char*) "-c", > - (char *) line, NULL }, > - __environ); > + ret = __posix_spawn (&pid, SHELL_PATH, 0, &spawn_attr, > + (char *const[]){ (char *) SHELL_NAME, > + (char *) "-c", > + (char *) line, NULL }, > + __environ); > __posix_spawnattr_destroy (&spawn_attr); > > - if (status == 0) > + if (ret == 0) > { > /* Cancellation results in cleanup handlers running as exceptions in > the block where they were installed, so it is safe to reference > @@ -186,6 +187,9 @@ do_system (const char *line) > } > DO_UNLOCK (); > > + if (ret != 0) > + __set_errno (ret); OK. > + > return status; > } > >
On Mär 23 2020, Adhemerval Zanella via Libc-alpha wrote: > + if (args->term_sig == 0) > + { > + /* Expect regular termination. */ > + TEST_COMPARE (WIFEXITED (ret), 1); > + TEST_COMPARE (WEXITSTATUS (ret), args->exit_status); > + } > + else > + { > + /* status_or_signal < 0. Expect termination by signal. */ > + TEST_COMPARE (WIFSIGNALED (ret), 1); > + TEST_COMPARE (WTERMSIG (ret), args->term_sig); There is no requirement that WIF* return pure boolean values. This should check for != 0, not == 1. Andreas.
On 23/03/2020 16:12, Andreas Schwab wrote: > On Mär 23 2020, Adhemerval Zanella via Libc-alpha wrote: > >> + if (args->term_sig == 0) >> + { >> + /* Expect regular termination. */ >> + TEST_COMPARE (WIFEXITED (ret), 1); >> + TEST_COMPARE (WEXITSTATUS (ret), args->exit_status); >> + } >> + else >> + { >> + /* status_or_signal < 0. Expect termination by signal. */ >> + TEST_COMPARE (WIFSIGNALED (ret), 1); >> + TEST_COMPARE (WTERMSIG (ret), args->term_sig); > > There is no requirement that WIF* return pure boolean values. This > should check for != 0, not == 1. > > Andreas. > Ack, I will change to TEST_VERIFY.
Hi Adhemerval, I've got two test fails as shown below on my Fedora 31/30 (s390x, x86_64) machines. I assume the error-messages differs depending on the used shell? Bye, Stefan On 3/23/20 7:31 PM, Adhemerval Zanella via Libc-alpha wrote: > It fixes 5fb7fc9635 when posix_spawn fails. > > Checked on x86_64-linux-gnu and i686-linux-gnu. > --- > stdlib/tst-system.c | 122 +++++++++++++++++++++++++++++++++++++++-- > sysdeps/posix/system.c | 18 +++--- > 2 files changed, 129 insertions(+), 11 deletions(-) > > diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c > index b6c5aea08f..04c87a1c6b 100644 > --- a/stdlib/tst-system.c > +++ b/stdlib/tst-system.c ... > + char cmd[namemax]; > + memset (cmd, 'a', sizeof(cmd)); > + cmd[sizeof(cmd) - 1] = '\0'; > + > + struct support_capture_subprocess result; > + result = support_capture_subprocess (call_system, > + &(struct args) { > + cmd, 127, 0, tmpdir > + }); > + support_capture_subprocess_check (&result, "system", 0, sc_allow_stderr); > + > + char *returnerr = xasprintf ("%s: 1: %s: not found\n", > + basename(_PATH_BSHELL), cmd); > + TEST_COMPARE_STRING (result.err.buffer, returnerr); tst-system.c:93: error: string comparison failed left string: 279 bytes right string: 274 bytes left (evaluated from result.err.buffer): "sh: aa...aa: command not found\n" right (evaluated from returnerr): "sh: 1: aa...aa: not found\n" > + free (returnerr); > + } > + > + { > + char cmd[namemax + 1]; > + memset (cmd, 'a', sizeof(cmd)); > + cmd[sizeof(cmd) - 1] = '\0'; > + > + struct support_capture_subprocess result; > + result = support_capture_subprocess (call_system, > + &(struct args) { > + cmd, 127, 0, tmpdir > + }); > + support_capture_subprocess_check (&result, "system", 0, sc_allow_stderr); > + > + char *returnerr = xasprintf ("%s: 1: %s: File name too long\n", > + basename(_PATH_BSHELL), cmd); > + TEST_COMPARE_STRING (result.err.buffer, returnerr); tst-system.c:111: error: string comparison failed left string: 280 bytes right string: 284 bytes left (evaluated from result.err.buffer): "sh: aa...aa: command not found\n" right (evaluated from returnerr): "sh: 1: aa...aa: File name too long\n" > + free (returnerr); > + }
* Stefan Liebler via Libc-alpha: > I've got two test fails as shown below on my Fedora 31/30 (s390x, > x86_64) machines. I assume the error-messages differs depending on the > used shell? Looks like it. It may be possible to turn this test into a container test, so that the minimal shell implementation from support/shell-container.c is used, which delivers consistent behavior.
On 24/03/2020 11:05, Florian Weimer wrote: > * Stefan Liebler via Libc-alpha: > >> I've got two test fails as shown below on my Fedora 31/30 (s390x, >> x86_64) machines. I assume the error-messages differs depending on the >> used shell? > > Looks like it. It may be possible to turn this test into a container > test, so that the minimal shell implementation from > support/shell-container.c is used, which delivers consistent behavior. > Indeed it depends on how the shell handles the arguments and the message output it shows. I will change to use be container test.
diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c index b6c5aea08f..04c87a1c6b 100644 --- a/stdlib/tst-system.c +++ b/stdlib/tst-system.c @@ -17,14 +17,128 @@ <https://www.gnu.org/licenses/>. */ #include <stdlib.h> +#include <unistd.h> +#include <string.h> +#include <signal.h> +#include <paths.h> +#include <support/capture_subprocess.h> +#include <support/check.h> +#include <support/temp_file.h> +#include <support/support.h> + +static char *tmpdir; +static long int namemax; + +static void +do_prepare (int argc, char *argv[]) +{ + tmpdir = support_create_temp_directory ("tst-system-"); + /* Include the last '/0'. */ + namemax = pathconf (tmpdir, _PC_NAME_MAX) + 1; + TEST_VERIFY_EXIT (namemax != -1); +} +#define PREPARE do_prepare + +struct args +{ + const char *command; + int exit_status; + int term_sig; + const char *path; +}; + +static void +call_system (void *closure) +{ + struct args *args = (struct args *) closure; + int ret; + + if (args->path != NULL) + TEST_COMPARE (setenv ("PATH", args->path, 1), 0); + ret = system (args->command); + if (args->term_sig == 0) + { + /* Expect regular termination. */ + TEST_COMPARE (WIFEXITED (ret), 1); + TEST_COMPARE (WEXITSTATUS (ret), args->exit_status); + } + else + { + /* status_or_signal < 0. Expect termination by signal. */ + TEST_COMPARE (WIFSIGNALED (ret), 1); + TEST_COMPARE (WTERMSIG (ret), args->term_sig); + } +} static int do_test (void) { - return system (":"); -} + TEST_VERIFY (system (NULL) != 0); + { + char cmd[namemax]; + memset (cmd, 'a', sizeof(cmd)); + cmd[sizeof(cmd) - 1] = '\0'; + + struct support_capture_subprocess result; + result = support_capture_subprocess (call_system, + &(struct args) { + cmd, 127, 0, tmpdir + }); + support_capture_subprocess_check (&result, "system", 0, sc_allow_stderr); + + char *returnerr = xasprintf ("%s: 1: %s: not found\n", + basename(_PATH_BSHELL), cmd); + TEST_COMPARE_STRING (result.err.buffer, returnerr); + free (returnerr); + } + + { + char cmd[namemax + 1]; + memset (cmd, 'a', sizeof(cmd)); + cmd[sizeof(cmd) - 1] = '\0'; + + struct support_capture_subprocess result; + result = support_capture_subprocess (call_system, + &(struct args) { + cmd, 127, 0, tmpdir + }); + support_capture_subprocess_check (&result, "system", 0, sc_allow_stderr); + + char *returnerr = xasprintf ("%s: 1: %s: File name too long\n", + basename(_PATH_BSHELL), cmd); + TEST_COMPARE_STRING (result.err.buffer, returnerr); + free (returnerr); + } + + { + struct support_capture_subprocess result; + result = support_capture_subprocess (call_system, + &(struct args) { + "kill -USR1 $$", 0, SIGUSR1 + }); + support_capture_subprocess_check (&result, "system", 0, sc_allow_none); + } + + { + struct support_capture_subprocess result; + result = support_capture_subprocess (call_system, + &(struct args) { "echo ...", 0 }); + support_capture_subprocess_check (&result, "system", 0, sc_allow_stdout); + TEST_COMPARE_STRING (result.out.buffer, "...\n"); + } + + { + struct support_capture_subprocess result; + result = support_capture_subprocess (call_system, + &(struct args) { "exit 1", 1 }); + support_capture_subprocess_check (&result, "system", 0, sc_allow_none); + } + + TEST_COMPARE (system (":"), 0); + + return 0; +} -#define TEST_FUNCTION do_test () -#include "../test-skeleton.c" +#include <support/test-driver.c> diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c index e613e6a344..a03f478fc7 100644 --- a/sysdeps/posix/system.c +++ b/sysdeps/posix/system.c @@ -101,7 +101,8 @@ cancel_handler (void *arg) static int do_system (const char *line) { - int status; + int status = -1; + int ret; pid_t pid; struct sigaction sa; #ifndef _LIBC_REENTRANT @@ -144,14 +145,14 @@ do_system (const char *line) __posix_spawnattr_setflags (&spawn_attr, POSIX_SPAWN_SETSIGDEF | POSIX_SPAWN_SETSIGMASK); - status = __posix_spawn (&pid, SHELL_PATH, 0, &spawn_attr, - (char *const[]){ (char*) SHELL_NAME, - (char*) "-c", - (char *) line, NULL }, - __environ); + ret = __posix_spawn (&pid, SHELL_PATH, 0, &spawn_attr, + (char *const[]){ (char *) SHELL_NAME, + (char *) "-c", + (char *) line, NULL }, + __environ); __posix_spawnattr_destroy (&spawn_attr); - if (status == 0) + if (ret == 0) { /* Cancellation results in cleanup handlers running as exceptions in the block where they were installed, so it is safe to reference @@ -186,6 +187,9 @@ do_system (const char *line) } DO_UNLOCK (); + if (ret != 0) + __set_errno (ret); + return status; }