Re: [PATCH] librt: add test (bug 28213)

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 8/11/21 4:56 PM, Никита Попов via Libc-alpha wrote:
> Hello. I'm submitting a test case for recent bug 28213. It shows the
> following results.
> Before fix of bug 28213:
> # cat ./rt/tst-bz28213.out
> Child caused segmentation fault
> # cat ./rt/tst-bz28213.test-result
> FAIL: rt/tst-bz28213
> original exit status 1
> 
> After fix of bug 28213:
> # cat ./rt/tst-bz28213.out
> # cat ./rt/tst-bz28213.test-result
> PASS: rt/tst-bz28213
> original exit status 0
> I managed to get libc cause segmentation fault.
> Looking forward to receiving feedback from you.

Thank you for sharing this.  My detailed review comments are inline. 
Overall I think this can be done without forking a new process since the 
test infrastructure already does that and is capable of reporting 
unexpected signals.  All you need to test is that the program runs to 
completion and ensure that the helper thread is called, perhaps by 
simply sleeping for a while.

> From eb51dcf05befe6e839102e0cb622b1afdbf72a7a Mon Sep 17 00:00:00 2001
> From: Nikita Popov <npv1310@gmail.com>
> Date: Wed, 11 Aug 2021 14:36:50 +0500
> Subject: [PATCH] librt: add test (bug 28213)
> To: libc-alpha@sourceware.org
> 
> This test implements following logic:
> 1) Create dummy message queue, register it with mq_notify (using NULL attributes),
>    immediately close the queue.
>    Helper thread should cause NULL pointer dereference by this moment.
> 2) Create another queue and try to send a message via the helper thread.
>    Test is considered successful if the callback function receives the same message as was sent.
> 
> Signed-off-by: Nikita Popov <npv1310@gmail.com>
> ---
>  rt/Makefile      |   1 +
>  rt/tst-bz28213.c | 174 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 175 insertions(+)
>  create mode 100644 rt/tst-bz28213.c
> 
> diff --git a/rt/Makefile b/rt/Makefile
> index 113cea03a5..910e775995 100644
> --- a/rt/Makefile
> +++ b/rt/Makefile
> @@ -74,6 +74,7 @@ tests := tst-shm tst-timer tst-timer2 \
>  	 tst-aio7 tst-aio8 tst-aio9 tst-aio10 \
>  	 tst-mqueue1 tst-mqueue2 tst-mqueue3 tst-mqueue4 \
>  	 tst-mqueue5 tst-mqueue6 tst-mqueue7 tst-mqueue8 tst-mqueue9 \
> +	 tst-bz28213 \
>  	 tst-timer3 tst-timer4 tst-timer5 \
>  	 tst-cpuclock2 tst-cputimer1 tst-cputimer2 tst-cputimer3 \
>  	 tst-shm-cancel \
> diff --git a/rt/tst-bz28213.c b/rt/tst-bz28213.c
> new file mode 100644
> index 0000000000..6c043a9017
> --- /dev/null
> +++ b/rt/tst-bz28213.c
> @@ -0,0 +1,174 @@
> +/* Bug 28213: test for NULL pointer dereference in mq_notify.
> +   Copyright (C) 2018-2021 Free Software Foundation, Inc.

Please make this copyright "The GNU Toolchain Authors" since you don't 
have a copyright assignment with the FSF.

> +   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 <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <mqueue.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <support/xunistd.h>
> +
> +static const char check_bz28213_name[] = "/bz28213_queue";
> +static const char check_bz28213_msg[] = "dummy";
> +
> +static void
> +check_bz28213_cb (union sigval sv)
> +{
> +  char buf[sizeof (check_bz28213_msg)];
> +  mqd_t m = sv.sival_int;
> +  ssize_t n;
> +
> +  if ((n = mq_receive (m, buf, sizeof (buf), NULL)) < 0L ||
> +      (size_t) n != sizeof (buf))
> +    exit (1);
> +
> +  if (memcmp (buf, check_bz28213_msg, sizeof (buf)) != 0)
> +    exit (1);
> +
> +  exit (0);
> +}

Instead of exiting with error, please use the TEST_VERIFY_EXIT macro in 
support/check.h.

> +
> +static void
> +check_bz28213 (void)
> +{
> +  mqd_t m;
> +  struct sigevent sev;
> +  struct mq_attr attr;
> +  unsigned int i;
> +
> +  /* First iteration should lead to undefined behavior due to NULL pointer dereference.
> +     Second iteration tests whether helper thread still works. */
> +  for (i = 0U; i < 2U; i++)
> +    {
> +      memset (&attr, '\0', sizeof (attr));
> +      attr.mq_maxmsg = 1;
> +      attr.mq_msgsize = sizeof (check_bz28213_msg);
> +
> +      if ((m = mq_open (check_bz28213_name,
> +                        O_RDWR | O_CREAT | O_EXCL,
> +                        0600,
> +                        &attr)) < 0)
> +        exit (1);
> +
> +      if (mq_unlink (check_bz28213_name) < 0)
> +        exit (1);
> +
> +      memset (&sev, '\0', sizeof (sev));
> +      sev.sigev_notify = SIGEV_THREAD;
> +      sev.sigev_value.sival_int = m;
> +      sev.sigev_notify_function = check_bz28213_cb;
> +
> +      if (mq_notify (m, &sev) < 0)
> +        exit (1);
> +
> +      if (i && mq_send (m, check_bz28213_msg, sizeof (check_bz28213_msg), 1) < 0)
> +        exit (1);

Likewise for these exits, please use TEST_VERIFY_EXIT instead.

> +
> +      if (!i)
> +        mq_close (m);
> +    }
> +
> +  alarm (10);
> +
> +  while (1)
> +    pause ();

Instead you could define TIMEOUT to DEFAULT_TIMEOUT and then use a 
sleep(DEFAULT_TIMEOUT / 2) to wait long enough (10 seconds ought to be 
enough for anybody?) for the helper thread to run.  Architecture 
maintainers can later bump up TIMEOUT if they find that the test times 
out on their architecture.

> +}
> +
> +/* Skip entire testing if queues are not implemented */
> +static int
> +check_mq_api (void)
> +{
> +  struct mq_attr attr;
> +  mqd_t m;
> +  int rc = 0;
> +
> +  memset (&attr, '\0', sizeof (attr));
> +  attr.mq_maxmsg = 1;
> +  attr.mq_msgsize = 1;
> +
> +  if ((m = mq_open (check_bz28213_name,
> +                    O_RDWR | O_CREAT | O_EXCL,
> +                    0600,
> +                    &attr)) < 0)
> +    {
> +      if (errno == ENOSYS)
> +        {
> +          rc = 1;
> +          printf ("SKIP: not implemented\n");
> +        }
> +
> +      return rc;
> +    }
> +
> +  mq_unlink (check_bz28213_name);
> +  mq_close (m);
> +
> +  return rc;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  pid_t pid;
> +  int status, rc;
> +
> +  if (check_mq_api () == 1)
> +    return 0;

Please use FAIL_UNSUPPORTED() here.

> +
> +  if ((pid = xfork ()) == 0)
> +    check_bz28213 ();
> +
> +  if (TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)) != pid)
> +    {
> +      kill (pid, SIGKILL);
> +      printf ("waitpid failed\n");
> +      return 1;
> +    }
> +
> +  rc = 1;
> +  if (WIFEXITED (status))
> +    {
> +      int child_rc;
> +
> +      if ((child_rc = WEXITSTATUS (status)) == 0)
> +        rc = 0;
> +      else
> +        printf ("Child returned non-zero exit code [%d]\n", child_rc);
> +    }
> +  else if (WIFSIGNALED (status))
> +    {
> +      int child_signo;
> +
> +      if ((child_signo = WTERMSIG (status)) == SIGABRT)
> +        printf ("Child timed out\n");
> +      else if (child_signo == SIGSEGV)
> +        printf ("Child caused segmentation fault\n");
> +      else
> +        printf ("Child terminated with signal %d\n", child_signo);
> +    }

Forking should not be necessary since the test driver already reports 
unexpected signals.

> +
> +  return rc;

Return 0 in the end.

> +}
> +
> +#include <support/test-driver.c>
> -- 
> 2.17.1