[PATCH v3 1/2] linux: wait4: Fix incorrect return value comparison

[PATCH v3 1/2] linux: wait4: Fix incorrect return value comparison

[Alistair Francis]

Patch 600f00b "linux: Use long time_t for wait4/getrusage" introduced
two bugs:
 - The usage32 struct was set if the wait4 syscall had an error.
 - For 32-bit systems the usage struct was set even if it was specified
   as NULL.

This patch fixes the two issues.
---
 sysdeps/unix/sysv/linux/wait4.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/wait4.c b/sysdeps/unix/sysv/linux/wait4.c
index d14bd4da27..21eb154b72 100644
--- a/sysdeps/unix/sysv/linux/wait4.c
+++ b/sysdeps/unix/sysv/linux/wait4.c
@@ -29,14 +29,18 @@ __wait4_time64 (pid_t pid, int *stat_loc, int options, struct __rusage64 *usage)
 # if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64
   return SYSCALL_CANCEL (wait4, pid, stat_loc, options, usage);
 # else
-  struct __rusage32 usage32;
-  pid_t ret = SYSCALL_CANCEL (wait4, pid, stat_loc, options, &usage32);
-
-  if (ret != 0)
-    return ret;
+  pid_t ret;
 
   if (usage != NULL)
-    rusage32_to_rusage64 (&usage32, usage);
+    {
+      struct __rusage32 usage32;
+        ret = SYSCALL_CANCEL (wait4, pid, stat_loc, options, &usage32);
+
+      if (ret > 0)
+        rusage32_to_rusage64 (&usage32, usage);
+    }
+  else
+    ret = SYSCALL_CANCEL (wait4, pid, stat_loc, options, NULL);
 
   return ret;
 # endif
@@ -114,15 +118,19 @@ libc_hidden_def (__wait4_time64)
 pid_t
 __wait4 (pid_t pid, int *stat_loc, int options, struct rusage *usage)
 {
-  pid_t ret ;
-  struct __rusage64 usage64;
+  pid_t ret;
 
-  ret = __wait4_time64 (pid, stat_loc, options, &usage64);
+  if (usage != NULL)
+    {
+      struct __rusage64 usage64;
 
-  if (ret != 0)
-    return ret;
+      ret = __wait4_time64 (pid, stat_loc, options, &usage64);
 
-  rusage64_to_rusage (&usage64, usage);
+      if (ret > 0)
+        rusage64_to_rusage (&usage64, usage);
+    }
+  else
+    ret = __wait4_time64 (pid, stat_loc, options, NULL);
 
   return ret;
 }
-- 
2.26.0

[PATCH v3 2/2] linux: Add wait4 test case

[Alistair Francis]

Add a simple test that checks if the wait4 syscall works without an
rusage argument. This test would have caught the bugs introduced in
commit 600f00b "linux: Use long time_t for wait4/getrusage".
---
 sysdeps/unix/sysv/linux/Makefile    |  2 +-
 sysdeps/unix/sysv/linux/tst-wait4.c | 51 +++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-wait4.c

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 60dc5cf9e5..d5fdb4369a 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -98,7 +98,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
 	 tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
 	 test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
 	 tst-rlimit-infinity tst-ofdlocks tst-gettid tst-gettid-kill \
-	 tst-tgkill
+	 tst-tgkill tst-wait4
 tests-internal += tst-ofdlocks-compat tst-sigcontext-get_pc
 
 CFLAGS-tst-sigcontext-get_pc.c = -fasynchronous-unwind-tables
diff --git a/sysdeps/unix/sysv/linux/tst-wait4.c b/sysdeps/unix/sysv/linux/tst-wait4.c
new file mode 100644
index 0000000000..44287797a7
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-wait4.c
@@ -0,0 +1,51 @@
+/* Smoke test for the wait4 system call.
+   Copyright (C) 2019-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/check.h>
+#include <support/namespace.h>
+#include <sys/wait.h>
+#include <sys/resource.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+/* Check that wait4 works with no rusage arg.  */
+static void
+test_wait4_no_usage (void)
+{
+  pid_t child = fork ();
+
+  if (child == 0)
+    {
+      pause ();
+      exit (0);
+    }
+
+  wait4 (child, &(int){0}, WNOHANG, NULL);
+  kill (child, SIGKILL);
+}
+
+static int
+do_test (void)
+{
+  test_wait4_no_usage();
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.26.0

Re: [PATCH v3 1/2] linux: wait4: Fix incorrect return value comparison

[Adhemerval Zanella]

On 09/04/2020 17:11, Alistair Francis via Libc-alpha wrote:
> Patch 600f00b "linux: Use long time_t for wait4/getrusage" introduced
> two bugs:
>  - The usage32 struct was set if the wait4 syscall had an error.
>  - For 32-bit systems the usage struct was set even if it was specified
>    as NULL.
> 
> This patch fixes the two issues.

Ok with the changes below.

> ---
>  sysdeps/unix/sysv/linux/wait4.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/wait4.c b/sysdeps/unix/sysv/linux/wait4.c
> index d14bd4da27..21eb154b72 100644
> --- a/sysdeps/unix/sysv/linux/wait4.c
> +++ b/sysdeps/unix/sysv/linux/wait4.c
> @@ -29,14 +29,18 @@ __wait4_time64 (pid_t pid, int *stat_loc, int options, struct __rusage64 *usage)
>  # if __KERNEL_OLD_TIMEVAL_MATCHES_TIMEVAL64
>    return SYSCALL_CANCEL (wait4, pid, stat_loc, options, usage);
>  # else
> -  struct __rusage32 usage32;
> -  pid_t ret = SYSCALL_CANCEL (wait4, pid, stat_loc, options, &usage32);
> -
> -  if (ret != 0)
> -    return ret;
> +  pid_t ret;
>  
>    if (usage != NULL)
> -    rusage32_to_rusage64 (&usage32, usage);
> +    {
> +      struct __rusage32 usage32;
> +        ret = SYSCALL_CANCEL (wait4, pid, stat_loc, options, &usage32);

Indentation seems off.

> +
> +      if (ret > 0)
> +        rusage32_to_rusage64 (&usage32, usage);> +    }
> +  else
> +    ret = SYSCALL_CANCEL (wait4, pid, stat_loc, options, NULL);

I think there is no much gain in optimizing stack usage here,
I would prefer to simplify to just:

  struct __rusage32 usage32;
  ret = SYSCALL_CANCEL (wait4, pid, stat_loc, options,
                        usage != NULL ? &usage32 : NULL);
  if (ret > 0 && usage != NULL)
    rusage32_to_rusage64 (&usage32, usage);


>  
>    return ret;
>  # endif
> @@ -114,15 +118,19 @@ libc_hidden_def (__wait4_time64)
>  pid_t
>  __wait4 (pid_t pid, int *stat_loc, int options, struct rusage *usage)
>  {
> -  pid_t ret ;
> -  struct __rusage64 usage64;
> +  pid_t ret;
>  
> -  ret = __wait4_time64 (pid, stat_loc, options, &usage64);
> +  if (usage != NULL)
> +    {
> +      struct __rusage64 usage64;
>  
> -  if (ret != 0)
> -    return ret;
> +      ret = __wait4_time64 (pid, stat_loc, options, &usage64);
>  
> -  rusage64_to_rusage (&usage64, usage);
> +      if (ret > 0)
> +        rusage64_to_rusage (&usage64, usage);
> +    }
> +  else
> +    ret = __wait4_time64 (pid, stat_loc, options, NULL);
>  
>    return ret;
>  }
> 

Same as before:

  struct __rusage64 usage64;
  ret = __wait4_time64 (pid, stat_loc, options,
                        usage != NULL ? &usage64 : NULL);
  if (ret > 0 && usage != 0)
    rusage64_to_rusage (&usage64, usage);

Re: [PATCH v3 2/2] linux: Add wait4 test case

[Adhemerval Zanella]

On 09/04/2020 17:11, Alistair Francis via Libc-alpha wrote:
> Add a simple test that checks if the wait4 syscall works without an
> rusage argument. This test would have caught the bugs introduced in
> commit 600f00b "linux: Use long time_t for wait4/getrusage".
> ---
>  sysdeps/unix/sysv/linux/Makefile    |  2 +-
>  sysdeps/unix/sysv/linux/tst-wait4.c | 51 +++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+), 1 deletion(-)
>  create mode 100644 sysdeps/unix/sysv/linux/tst-wait4.c
> 
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 60dc5cf9e5..d5fdb4369a 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -98,7 +98,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
>  	 tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
>  	 test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
>  	 tst-rlimit-infinity tst-ofdlocks tst-gettid tst-gettid-kill \
> -	 tst-tgkill
> +	 tst-tgkill tst-wait4
>  tests-internal += tst-ofdlocks-compat tst-sigcontext-get_pc

wait4 is exported as gnu extension, so there is no need to make the test
linux specific.

>  
>  CFLAGS-tst-sigcontext-get_pc.c = -fasynchronous-unwind-tables
> diff --git a/sysdeps/unix/sysv/linux/tst-wait4.c b/sysdeps/unix/sysv/linux/tst-wait4.c
> new file mode 100644
> index 0000000000..44287797a7
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-wait4.c
> @@ -0,0 +1,51 @@
> +/* Smoke test for the wait4 system call.
> +   Copyright (C) 2019-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/check.h>
> +#include <support/namespace.h>
> +#include <sys/wait.h>
> +#include <sys/resource.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +
> +/* Check that wait4 works with no rusage arg.  */
> +static void
> +test_wait4_no_usage (void)
> +{
> +  pid_t child = fork ();
> +
> +  if (child == 0)
> +    {
> +      pause ();
> +      exit (0);
> +    }
> +
> +  wait4 (child, &(int){0}, WNOHANG, NULL);
> +  kill (child, SIGKILL);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  test_wait4_no_usage();
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> 

Below it is a more comprehensible tests based on tst-waitid.
It should trigger the issue and add some more coverage to
wait4 call:

--

/* Tests for wait4.
   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 <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/wait.h>
#include <sys/resource.h>
#include <signal.h>
#include <time.h>
#include <stdatomic.h>
#include <stdbool.h>

#include <support/xsignal.h>
#include <support/xunistd.h>
#include <support/check.h>
#include <support/process_state.h>

static void
test_child (void)
{
  /* First thing, we stop ourselves.  */
  raise (SIGSTOP);

  /* Hey, we got continued!  */
  while (1)
    pause ();
}

#ifndef WEXITED
# define WEXITED	0
# define WCONTINUED	0
# define WSTOPPED	WUNTRACED
#endif

/* Set with only SIGCHLD on do_test_waitid.  */
static sigset_t chldset;

#ifdef SA_SIGINFO
static void
sigchld (int signo, siginfo_t *info, void *ctx)
{
}
#endif

static void
check_sigchld (int code, int status, pid_t pid)
{
#ifdef SA_SIGINFO
  siginfo_t siginfo;
  TEST_COMPARE (sigwaitinfo (&chldset, &siginfo), SIGCHLD);

  TEST_COMPARE (siginfo.si_signo, SIGCHLD);
  TEST_COMPARE (siginfo.si_code, code);
  TEST_COMPARE (siginfo.si_status, status);
  TEST_COMPARE (siginfo.si_pid, pid);
#endif
}

static int
do_test_wait4 (pid_t pid)
{
  /* Adding process_state_tracing_stop ('t') allows the test to work under
     trace programs such as ptrace.  */
  enum support_process_state stop_state = support_process_state_stopped
					  | support_process_state_tracing_stop;

  support_process_state_wait (pid, stop_state);

  check_sigchld (CLD_STOPPED, SIGSTOP, pid);

  pid_t ret;
  int wstatus;
  struct rusage rusage;

  ret = wait4 (pid, &wstatus, WUNTRACED|WCONTINUED|WNOHANG, NULL);
  if (ret == -1 && errno == ENOTSUP)
    FAIL_RET ("waitid WNOHANG on stopped: %m");
  TEST_COMPARE (ret, pid);
  TEST_VERIFY (WIFSTOPPED (wstatus));

  /* Issue again but with struct rusage input.  */
  ret = wait4 (pid, &wstatus, WUNTRACED|WCONTINUED|WNOHANG, &rusage);
  /* With WNOHANG and WUNTRACED, if the children has not changes its state
     since previous call the expected result it 0.  */
  TEST_COMPARE (ret, 0);

  /* Some sanity tests to check if 'wtatus' and 'rusage' possible
     input values.  */
  ret = wait4 (pid, NULL, WUNTRACED|WCONTINUED|WNOHANG, &rusage);
  TEST_COMPARE (ret, 0);
  ret = wait4 (pid, NULL, WUNTRACED|WCONTINUED|WNOHANG, NULL);
  TEST_COMPARE (ret, 0);

  if (kill (pid, SIGCONT) != 0)
    FAIL_RET ("kill (%d, SIGCONT): %m\n", pid);

  /* Wait for the child to have continued.  */
  support_process_state_wait (pid, support_process_state_sleeping);

#if WCONTINUED != 0
  check_sigchld (CLD_CONTINUED, SIGCONT, pid);

  ret = wait4 (pid, &wstatus, WCONTINUED|WNOHANG, NULL);
  TEST_COMPARE (ret, pid);
  TEST_VERIFY (WIFCONTINUED (wstatus));

  /* Issue again but with struct rusage input.  */
  ret = wait4 (pid, &wstatus, WUNTRACED|WCONTINUED|WNOHANG, &rusage);
  /* With WNOHANG and WUNTRACED, if the children has not changes its state
     since previous call the expected result it 0.  */
  TEST_COMPARE (ret, 0);

  /* Now stop him again and test waitpid with WCONTINUED.  */
  if (kill (pid, SIGSTOP) != 0)
    FAIL_RET ("kill (%d, SIGSTOP): %m\n", pid);

  /* Wait the child stop.  The waitid call below will block until it has
     stopped, but if we are real quick and enter the waitid system call
     before the SIGCHLD has been generated, then it will be discarded and
     never delivered.  */
  support_process_state_wait (pid, stop_state);

  ret = wait4 (pid, &wstatus, WUNTRACED|WNOHANG, &rusage);
  TEST_COMPARE (ret, pid);

  check_sigchld (CLD_STOPPED, SIGSTOP, pid);

  if (kill (pid, SIGCONT) != 0)
    FAIL_RET ("kill (%d, SIGCONT): %m\n", pid);

  /* Wait for the child to have continued.  */
  support_process_state_wait (pid, support_process_state_sleeping);

  check_sigchld (CLD_CONTINUED, SIGCONT, pid);

  ret = wait4 (pid, &wstatus, WCONTINUED|WNOHANG, NULL);
  TEST_COMPARE (ret, pid);
  TEST_VERIFY (WIFCONTINUED (wstatus));
#endif

  /* Die, child, die!  */
  if (kill (pid, SIGKILL) != 0)
    FAIL_RET ("kill (%d, SIGKILL): %m\n", pid);

  support_process_state_wait (pid, support_process_state_zombie);

  ret = wait4 (pid, &wstatus, 0, &rusage);
  TEST_COMPARE (ret, pid);
  TEST_VERIFY (WIFSIGNALED (wstatus));
  TEST_VERIFY (WTERMSIG (wstatus) == SIGKILL);

  check_sigchld (CLD_KILLED, SIGKILL, pid);

  return 0;
}

static int
do_test (void)
{
#ifdef SA_SIGINFO
  {
    struct sigaction sa;
    sa.sa_flags = SA_SIGINFO | SA_RESTART;
    sa.sa_sigaction = sigchld;
    sigemptyset (&sa.sa_mask);
    xsigaction (SIGCHLD, &sa, NULL);
  }
#endif

  sigemptyset (&chldset);
  sigaddset (&chldset, SIGCHLD);

  /* The SIGCHLD shall has blocked at the time of the call to sigwait;
     otherwise, the behavior is undefined.  */
  sigprocmask (SIG_BLOCK, &chldset, NULL);

  pid_t pid = xfork ();
  if (pid == 0)
    {
      test_child ();
      _exit (127);
    }

  do_test_wait4 (pid);

  xsignal (SIGCHLD, SIG_IGN);
  kill (pid, SIGKILL);		/* Make sure it's dead if we bailed early.  */

  return 0;
}

#include <support/test-driver.c>

[PATCH v3 2/2] linux: Add wait4 test case

[Alistair Francis]

Add a simple test that checks if the wait4 syscall works without an
rusage argument. This test would have caught the bugs introduced in
commit 600f00b "linux: Use long time_t for wait4/getrusage".
---
 sysdeps/unix/sysv/linux/Makefile    |  2 +-
 sysdeps/unix/sysv/linux/tst-wait4.c | 51 +++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-wait4.c

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 60dc5cf9e5..d5fdb4369a 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -98,7 +98,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
 	 tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
 	 test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
 	 tst-rlimit-infinity tst-ofdlocks tst-gettid tst-gettid-kill \
-	 tst-tgkill
+	 tst-tgkill tst-wait4
 tests-internal += tst-ofdlocks-compat tst-sigcontext-get_pc
 
 CFLAGS-tst-sigcontext-get_pc.c = -fasynchronous-unwind-tables
diff --git a/sysdeps/unix/sysv/linux/tst-wait4.c b/sysdeps/unix/sysv/linux/tst-wait4.c
new file mode 100644
index 0000000000..44287797a7
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-wait4.c
@@ -0,0 +1,51 @@
+/* Smoke test for the wait4 system call.
+   Copyright (C) 2019-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/check.h>
+#include <support/namespace.h>
+#include <sys/wait.h>
+#include <sys/resource.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+/* Check that wait4 works with no rusage arg.  */
+static void
+test_wait4_no_usage (void)
+{
+  pid_t child = fork ();
+
+  if (child == 0)
+    {
+      pause ();
+      exit (0);
+    }
+
+  wait4 (child, &(int){0}, WNOHANG, NULL);
+  kill (child, SIGKILL);
+}
+
+static int
+do_test (void)
+{
+  test_wait4_no_usage();
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.26.0