Re: [PATCH] malloc/tst-mallocfork2: Kill lingering process for unexpected failures

[Paul Clarke <pc at us dot ibm dot com>]

On 2/20/20 11:58 AM, Adhemerval Zanella wrote:
> If the test fails due some unexpected failure after the children
> creation, either in the signal handler by calling abort or in the main
> loop; the created children might not be killed properly.
> 
> This patches fixes it by:
> 
>   * Avoid aborting in the signal handler by setting a flag that
>     an error has occured and add a check in the main loop.
> 
>   * Add a atfork handler to handle kill the signal sending
>     processes.

s/atfork/atexit/, per the code below.

"handle kill the signal sending processes" doesn't read well.
Perhaps "kill child processes"?

> Checked on x86_64-linux-gnu.
> ---
>  malloc/tst-mallocfork2.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/malloc/tst-mallocfork2.c b/malloc/tst-mallocfork2.c
> index 0602a94895..4caf61489f 100644
> --- a/malloc/tst-mallocfork2.c
> +++ b/malloc/tst-mallocfork2.c
> @@ -62,6 +62,9 @@ static volatile sig_atomic_t sigusr1_received;
>     progress.  Checked by liveness_signal_handler.  */
>  static volatile sig_atomic_t progress_indicator = 1;
> 
> +/* Set to 1 if an error occurs in the signal handler.  */
> +static volatile sig_atomic_t error_indicator = 0;
> +
>  static void
>  sigusr1_handler (int signo)
>  {
> @@ -72,7 +75,8 @@ sigusr1_handler (int signo)
>    if (pid == -1)
>      {
>        write_message ("error: fork\n");
> -      abort ();
> +      error_indicator = 1;
> +      return;
>      }
>    if (pid == 0)
>      _exit (0);
> @@ -81,12 +85,14 @@ sigusr1_handler (int signo)
>    if (ret < 0)
>      {
>        write_message ("error: waitpid\n");
> -      abort ();
> +      error_indicator = 1;
> +      return;
>      }
>    if (status != 0)
>      {
>        write_message ("error: unexpected exit status from subprocess\n");
> -      abort ();
> +      error_indicator = 1;
> +      return;
>      }
>  }
> 

OK

> @@ -122,9 +128,25 @@ signal_sender (int signo, bool sleep)
>      }
>  }
> 
> +/* Children processes.  */
> +static pid_t sigusr1_sender_pids[5] = { -1 };

Doesn't this set [0] to -1 and [1..4] to 0 ?

> +static pid_t sigusr2_sender_pid = -1;
> +
> +static void
> +kill_children (void)
> +{
> +  for (size_t i = 0; i < array_length (sigusr1_sender_pids); ++i)
> +    if (sigusr1_sender_pids[i] != -1)
> +      kill (sigusr1_sender_pids[i], SIGKILL);

...and this will perform kill (0, SIGKILL) if not otherwise initialized, which will kill this task immediately, if I understand correctly.  I presume that's not what you want.

Why not use 0 as your initial/uninitialized value, and test against that?  Also, that way, you'll never issue kill (0, ...).

> +  if (sigusr2_sender_pid != -1)
> +    kill (sigusr2_sender_pid, SIGKILL);

Same.

> +}
> +
>  static int
>  do_test (void)
>  {
> +  atexit (kill_children);
> +
>    /* shared->barrier is intialized along with sigusr1_sender_pids
>       below.  */
>    shared = support_shared_allocate (sizeof (*shared));
> @@ -148,14 +170,13 @@ do_test (void)
>        return 1;
>      }
> 
> -  pid_t sigusr2_sender_pid = xfork ();
> +  sigusr2_sender_pid = xfork ();
>    if (sigusr2_sender_pid == 0)
>      signal_sender (SIGUSR2, true);
> 
>    /* Send SIGUSR1 signals from several processes.  Hopefully, one
>       signal will hit one of the ciritical functions.  Use a barrier to
>       avoid sending signals while not running fork/free/malloc.  */
> -  pid_t sigusr1_sender_pids[5];
>    {
>      pthread_barrierattr_t attr;
>      xpthread_barrierattr_init (&attr);
> @@ -166,7 +187,7 @@ do_test (void)
>    }
>    for (size_t i = 0; i < array_length (sigusr1_sender_pids); ++i)
>      {
> -      sigusr1_sender_pids[i] = fork ();
> +      sigusr1_sender_pids[i] = xfork ();
>        if (sigusr1_sender_pids[i] == 0)
>          signal_sender (SIGUSR1, false);
>      }
> @@ -211,7 +232,7 @@ do_test (void)
>          ++malloc_signals;
>        xpthread_barrier_wait (&shared->barrier);
> 
> -      if (objects[slot] == NULL)
> +      if (objects[slot] == NULL || error_indicator != 0)
>          {
>            printf ("error: malloc: %m\n");
>            for (size_t i = 0; i < array_length (sigusr1_sender_pids); ++i)
> @@ -225,10 +246,6 @@ do_test (void)
>    for (int slot = 0; slot < malloc_objects; ++slot)
>      free (objects[slot]);
> 
> -  for (size_t i = 0; i < array_length (sigusr1_sender_pids); ++i)
> -    kill (sigusr1_sender_pids[i], SIGKILL);
> -  kill (sigusr2_sender_pid, SIGKILL);
> -

OK

PC