diff mbox series

posix: Fix system error return value [BZ #25715]

Message ID 20200323183109.16201-1-adhemerval.zanella@linaro.org
State New
Headers show
Series posix: Fix system error return value [BZ #25715] | expand

Commit Message

dilfridge--- via Libc-alpha March 23, 2020, 6:31 p.m. UTC
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

dilfridge--- via Libc-alpha March 23, 2020, 6:54 p.m. UTC | #1
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;
>  }
>  
>
Andreas Schwab March 23, 2020, 7:12 p.m. UTC | #2
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.
dilfridge--- via Libc-alpha March 23, 2020, 7:30 p.m. UTC | #3
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.
dilfridge--- via Libc-alpha March 24, 2020, 1:53 p.m. UTC | #4
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);
> +  }
Florian Weimer March 24, 2020, 2:05 p.m. UTC | #5
* 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.
dilfridge--- via Libc-alpha March 24, 2020, 2:32 p.m. UTC | #6
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 mbox series

Patch

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;
 }