posix: Fix system error return value [BZ #25715]
Commit Message
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(-)
Comments
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.
@@ -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>
@@ -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;
}