librt: add test (bug 28213)

  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.
  

Hello, any update?

On Wed, Aug 11, 2021, 16:26 Никита Попов <npv1310@gmail.com> 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.
>
  

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
  

Thank you for your feedback!
I'm submitting a fixed version. It has slightly changed logic though.
The results are:
(before)
# cat rt/tst-bz28213.test-result
FAIL: rt/tst-bz28213
original exit status 1
# cat rt/tst-bz28213.out
Didn't expect signal from child: got `Segmentation fault'
(after)
# cat rt/tst-bz28213.test-result
PASS: rt/tst-bz28213
original exit status 0
# cat rt/tst-bz28213.out

чт, 12 авг. 2021 г. в 09:19, 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
>
  

On 8/12/21 2:30 PM, Никита Попов wrote:
> Thank you for your feedback!
> I'm submitting a fixed version. It has slightly changed logic though.
> The results are:
> (before)
> # cat rt/tst-bz28213.test-result
> FAIL: rt/tst-bz28213
> original exit status 1
> # cat rt/tst-bz28213.out
> Didn't expect signal from child: got `Segmentation fault'
> (after)
> # cat rt/tst-bz28213.test-result
> PASS: rt/tst-bz28213
> original exit status 0
> # cat rt/tst-bz28213.out

Thanks for the update, this logic is certainly more robust.  We're very 
close now, just some more nits:

> From 35600a79df9f3dedaca35ad8ec050be80e13fb12 Mon Sep 17 00:00:00 2001
> From: Nikita Popov <npv1310@gmail.com>
> Date: Thu, 12 Aug 2021 12:07:00 +0500
> Subject: [PATCH] librt: add test (bug 28213)
> To: libc-alpha@sourceware.org
> 
> This test implements following logic:
> 1) Create POSIX message queue.
>    Register a notification with mq_notify (using NULL thread attributes).
>    Then immediately unregister the notification with mq_notify.
>    Helper thread in a vulnerable version of glibc should cause NULL pointer dereference after these steps.
> 2) Once again, register the same notification. Try to send a dummy message.
>    Test is considered successful if the dummy message is successfully received by the callback function.

Please keep line lengths in git commit logs at 74 chars or less.

> 
> Signed-off-by: Nikita Popov <npv1310@gmail.com>
> ---
>  rt/Makefile      |   1 +
>  rt/tst-bz28213.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 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..b020e3d7dc
> --- /dev/null
> +++ b/rt/tst-bz28213.c
> @@ -0,0 +1,102 @@
> +/* Bug 28213: test for NULL pointer dereference in mq_notify.
> +   Copyright (C) 2021-2021 The GNU Toolchain Authors.

No copyright years needed.  Oh and congratulations, you're the first 
contributor to add this :)

> +   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 <fcntl.h>
> +#include <unistd.h>
> +#include <mqueue.h>
> +#include <signal.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <support/test-driver.h>
> +#include <support/check.h>
> +
> +static mqd_t m = -1;
> +static const char msg[] = "hello";
> +
> +static void
> +check_bz28213_cb (union sigval sv)
> +{
> +  char buf[sizeof (msg)];
> +
> +  (void) sv;
> +
> +  TEST_VERIFY_EXIT ((size_t) mq_receive (m, buf, sizeof (buf), NULL) == sizeof (buf));

Please wrap at 79 chars.  Please review the style and conventions guide 
here:

https://sourceware.org/glibc/wiki/Style_and_Conventions

> +  TEST_VERIFY_EXIT (memcmp (buf, msg, sizeof (buf)) == 0);
> +
> +  exit (0); > +}
> +
> +static void
> +check_bz28213 (void)
> +{
> +  struct sigevent sev;
> +
> +  memset (&sev, '\0', sizeof (sev));
> +  sev.sigev_notify = SIGEV_THREAD;
> +  sev.sigev_notify_function = check_bz28213_cb;
> +
> +  /* Step 1: Register & unregister notifier.
> +     Helper thread should receive NOTIFY_REMOVED notification.
> +     In a vulnerable version of glibc, NULL pointer dereference follows... */
> +  TEST_VERIFY_EXIT (mq_notify (m, &sev) == 0);
> +  TEST_VERIFY_EXIT (mq_notify (m, NULL) == 0);

OK.

> +
> +  /* Step 2: Once again, register notification.
> +     Try to send one message.
> +     Test is considered successful, if the callback does exit (0). */
> +  TEST_VERIFY_EXIT (mq_notify (m, &sev) == 0);
> +  TEST_VERIFY_EXIT (mq_send (m, msg, sizeof (msg), 1) == 0);

OK.

> +
> +  /* Wait... */
> +  sleep (DEFAULT_TIMEOUT);
> +  FAIL_EXIT1 ("Operation timed out\n");

With this new logic, you only need to pause() here.  The test driver has 
timeout functionality that will automatically fail the test if it stays 
stuck here for DEFAULT_TIMEOUT seconds.

> +}
> +
> +static int
> +do_test (void)
> +{
> +  static const char m_name[] = "/bz28213_queue";
> +  struct mq_attr m_attr;
> +
> +  memset (&m_attr, '\0', sizeof (m_attr));
> +  m_attr.mq_maxmsg = 1;
> +  m_attr.mq_msgsize = sizeof (msg);
> +
> +  m = mq_open (m_name,
> +               O_RDWR | O_CREAT | O_EXCL,
> +               0600,
> +               &m_attr);
> +
> +  if (m < 0)
> +    {
> +      if (errno == ENOSYS)
> +        FAIL_UNSUPPORTED ("POSIX message queues are not implemented\n");

OK.

> +      FAIL_EXIT1 ("Failed to create POSIX message queue: %m\n");
> +    }
> +
> +  TEST_VERIFY_EXIT (mq_unlink (m_name) == 0);
> +
> +  check_bz28213 ();
> +
> +  return 0;
> +}

OK.

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

чт, 12 авг. 2021 г. в 14:34, Siddhesh Poyarekar <siddhesh@gotplt.org>:
>
> On 8/12/21 2:30 PM, Никита Попов wrote:
> > Thank you for your feedback!
> > I'm submitting a fixed version. It has slightly changed logic though.
> > The results are:
> > (before)
> > # cat rt/tst-bz28213.test-result
> > FAIL: rt/tst-bz28213
> > original exit status 1
> > # cat rt/tst-bz28213.out
> > Didn't expect signal from child: got `Segmentation fault'
> > (after)
> > # cat rt/tst-bz28213.test-result
> > PASS: rt/tst-bz28213
> > original exit status 0
> > # cat rt/tst-bz28213.out
>
> Thanks for the update, this logic is certainly more robust.  We're very
> close now, just some more nits:
>
> > From 35600a79df9f3dedaca35ad8ec050be80e13fb12 Mon Sep 17 00:00:00 2001
> > From: Nikita Popov <npv1310@gmail.com>
> > Date: Thu, 12 Aug 2021 12:07:00 +0500
> > Subject: [PATCH] librt: add test (bug 28213)
> > To: libc-alpha@sourceware.org
> >
> > This test implements following logic:
> > 1) Create POSIX message queue.
> >    Register a notification with mq_notify (using NULL thread attributes).
> >    Then immediately unregister the notification with mq_notify.
> >    Helper thread in a vulnerable version of glibc should cause NULL pointer dereference after these steps.
> > 2) Once again, register the same notification. Try to send a dummy message.
> >    Test is considered successful if the dummy message is successfully received by the callback function.
>
> Please keep line lengths in git commit logs at 74 chars or less.
>
> >
> > Signed-off-by: Nikita Popov <npv1310@gmail.com>
> > ---
> >  rt/Makefile      |   1 +
> >  rt/tst-bz28213.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 103 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..b020e3d7dc
> > --- /dev/null
> > +++ b/rt/tst-bz28213.c
> > @@ -0,0 +1,102 @@
> > +/* Bug 28213: test for NULL pointer dereference in mq_notify.
> > +   Copyright (C) 2021-2021 The GNU Toolchain Authors.
>
> No copyright years needed.  Oh and congratulations, you're the first
> contributor to add this :)
>
> > +   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 <fcntl.h>
> > +#include <unistd.h>
> > +#include <mqueue.h>
> > +#include <signal.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <support/test-driver.h>
> > +#include <support/check.h>
> > +
> > +static mqd_t m = -1;
> > +static const char msg[] = "hello";
> > +
> > +static void
> > +check_bz28213_cb (union sigval sv)
> > +{
> > +  char buf[sizeof (msg)];
> > +
> > +  (void) sv;
> > +
> > +  TEST_VERIFY_EXIT ((size_t) mq_receive (m, buf, sizeof (buf), NULL) == sizeof (buf));
>
> Please wrap at 79 chars.  Please review the style and conventions guide
> here:
>
> https://sourceware.org/glibc/wiki/Style_and_Conventions
>
> > +  TEST_VERIFY_EXIT (memcmp (buf, msg, sizeof (buf)) == 0);
> > +
> > +  exit (0); > +}
> > +
> > +static void
> > +check_bz28213 (void)
> > +{
> > +  struct sigevent sev;
> > +
> > +  memset (&sev, '\0', sizeof (sev));
> > +  sev.sigev_notify = SIGEV_THREAD;
> > +  sev.sigev_notify_function = check_bz28213_cb;
> > +
> > +  /* Step 1: Register & unregister notifier.
> > +     Helper thread should receive NOTIFY_REMOVED notification.
> > +     In a vulnerable version of glibc, NULL pointer dereference follows... */
> > +  TEST_VERIFY_EXIT (mq_notify (m, &sev) == 0);
> > +  TEST_VERIFY_EXIT (mq_notify (m, NULL) == 0);
>
> OK.
>
> > +
> > +  /* Step 2: Once again, register notification.
> > +     Try to send one message.
> > +     Test is considered successful, if the callback does exit (0). */
> > +  TEST_VERIFY_EXIT (mq_notify (m, &sev) == 0);
> > +  TEST_VERIFY_EXIT (mq_send (m, msg, sizeof (msg), 1) == 0);
>
> OK.
>
> > +
> > +  /* Wait... */
> > +  sleep (DEFAULT_TIMEOUT);
> > +  FAIL_EXIT1 ("Operation timed out\n");
>
> With this new logic, you only need to pause() here.  The test driver has
> timeout functionality that will automatically fail the test if it stays
> stuck here for DEFAULT_TIMEOUT seconds.
>
> > +}
> > +
> > +static int
> > +do_test (void)
> > +{
> > +  static const char m_name[] = "/bz28213_queue";
> > +  struct mq_attr m_attr;
> > +
> > +  memset (&m_attr, '\0', sizeof (m_attr));
> > +  m_attr.mq_maxmsg = 1;
> > +  m_attr.mq_msgsize = sizeof (msg);
> > +
> > +  m = mq_open (m_name,
> > +               O_RDWR | O_CREAT | O_EXCL,
> > +               0600,
> > +               &m_attr);
> > +
> > +  if (m < 0)
> > +    {
> > +      if (errno == ENOSYS)
> > +        FAIL_UNSUPPORTED ("POSIX message queues are not implemented\n");
>
> OK.
>
> > +      FAIL_EXIT1 ("Failed to create POSIX message queue: %m\n");
> > +    }
> > +
> > +  TEST_VERIFY_EXIT (mq_unlink (m_name) == 0);
> > +
> > +  check_bz28213 ();
> > +
> > +  return 0;
> > +}
>
> OK.
>
> > +
> > +#include <support/test-driver.c>
> > --
> > 2.17.1
  

Hello. I have some concerns about the recent commit
4cc79c217744743077bf7a0ec5e0a4318f1e6641. In rt/tst-bz28213.c we use
one call to pause (). Should it return for some reason 'return 0' from
do_test () follows. Maybe it is better to wrap pause () call here into
an infinite loop like this? [while (1) pause ();] Then we can be sure
that the only success in this test can be reached via exit (0) from
the callback function.

чт, 12 авг. 2021 г. в 15:20, Никита Попов <npv1310@gmail.com>:

>
> чт, 12 авг. 2021 г. в 14:34, Siddhesh Poyarekar <siddhesh@gotplt.org>:
> >
> > On 8/12/21 2:30 PM, Никита Попов wrote:
> > > Thank you for your feedback!
> > > I'm submitting a fixed version. It has slightly changed logic though.
> > > The results are:
> > > (before)
> > > # cat rt/tst-bz28213.test-result
> > > FAIL: rt/tst-bz28213
> > > original exit status 1
> > > # cat rt/tst-bz28213.out
> > > Didn't expect signal from child: got `Segmentation fault'
> > > (after)
> > > # cat rt/tst-bz28213.test-result
> > > PASS: rt/tst-bz28213
> > > original exit status 0
> > > # cat rt/tst-bz28213.out
> >
> > Thanks for the update, this logic is certainly more robust.  We're very
> > close now, just some more nits:
> >
> > > From 35600a79df9f3dedaca35ad8ec050be80e13fb12 Mon Sep 17 00:00:00 2001
> > > From: Nikita Popov <npv1310@gmail.com>
> > > Date: Thu, 12 Aug 2021 12:07:00 +0500
> > > Subject: [PATCH] librt: add test (bug 28213)
> > > To: libc-alpha@sourceware.org
> > >
> > > This test implements following logic:
> > > 1) Create POSIX message queue.
> > >    Register a notification with mq_notify (using NULL thread attributes).
> > >    Then immediately unregister the notification with mq_notify.
> > >    Helper thread in a vulnerable version of glibc should cause NULL pointer dereference after these steps.
> > > 2) Once again, register the same notification. Try to send a dummy message.
> > >    Test is considered successful if the dummy message is successfully received by the callback function.
> >
> > Please keep line lengths in git commit logs at 74 chars or less.
> >
> > >
> > > Signed-off-by: Nikita Popov <npv1310@gmail.com>
> > > ---
> > >  rt/Makefile      |   1 +
> > >  rt/tst-bz28213.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 103 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..b020e3d7dc
> > > --- /dev/null
> > > +++ b/rt/tst-bz28213.c
> > > @@ -0,0 +1,102 @@
> > > +/* Bug 28213: test for NULL pointer dereference in mq_notify.
> > > +   Copyright (C) 2021-2021 The GNU Toolchain Authors.
> >
> > No copyright years needed.  Oh and congratulations, you're the first
> > contributor to add this :)
> >
> > > +   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 <fcntl.h>
> > > +#include <unistd.h>
> > > +#include <mqueue.h>
> > > +#include <signal.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <support/test-driver.h>
> > > +#include <support/check.h>
> > > +
> > > +static mqd_t m = -1;
> > > +static const char msg[] = "hello";
> > > +
> > > +static void
> > > +check_bz28213_cb (union sigval sv)
> > > +{
> > > +  char buf[sizeof (msg)];
> > > +
> > > +  (void) sv;
> > > +
> > > +  TEST_VERIFY_EXIT ((size_t) mq_receive (m, buf, sizeof (buf), NULL) == sizeof (buf));
> >
> > Please wrap at 79 chars.  Please review the style and conventions guide
> > here:
> >
> > https://sourceware.org/glibc/wiki/Style_and_Conventions
> >
> > > +  TEST_VERIFY_EXIT (memcmp (buf, msg, sizeof (buf)) == 0);
> > > +
> > > +  exit (0); > +}
> > > +
> > > +static void
> > > +check_bz28213 (void)
> > > +{
> > > +  struct sigevent sev;
> > > +
> > > +  memset (&sev, '\0', sizeof (sev));
> > > +  sev.sigev_notify = SIGEV_THREAD;
> > > +  sev.sigev_notify_function = check_bz28213_cb;
> > > +
> > > +  /* Step 1: Register & unregister notifier.
> > > +     Helper thread should receive NOTIFY_REMOVED notification.
> > > +     In a vulnerable version of glibc, NULL pointer dereference follows... */
> > > +  TEST_VERIFY_EXIT (mq_notify (m, &sev) == 0);
> > > +  TEST_VERIFY_EXIT (mq_notify (m, NULL) == 0);
> >
> > OK.
> >
> > > +
> > > +  /* Step 2: Once again, register notification.
> > > +     Try to send one message.
> > > +     Test is considered successful, if the callback does exit (0). */
> > > +  TEST_VERIFY_EXIT (mq_notify (m, &sev) == 0);
> > > +  TEST_VERIFY_EXIT (mq_send (m, msg, sizeof (msg), 1) == 0);
> >
> > OK.
> >
> > > +
> > > +  /* Wait... */
> > > +  sleep (DEFAULT_TIMEOUT);
> > > +  FAIL_EXIT1 ("Operation timed out\n");
> >
> > With this new logic, you only need to pause() here.  The test driver has
> > timeout functionality that will automatically fail the test if it stays
> > stuck here for DEFAULT_TIMEOUT seconds.
> >
> > > +}
> > > +
> > > +static int
> > > +do_test (void)
> > > +{
> > > +  static const char m_name[] = "/bz28213_queue";
> > > +  struct mq_attr m_attr;
> > > +
> > > +  memset (&m_attr, '\0', sizeof (m_attr));
> > > +  m_attr.mq_maxmsg = 1;
> > > +  m_attr.mq_msgsize = sizeof (msg);
> > > +
> > > +  m = mq_open (m_name,
> > > +               O_RDWR | O_CREAT | O_EXCL,
> > > +               0600,
> > > +               &m_attr);
> > > +
> > > +  if (m < 0)
> > > +    {
> > > +      if (errno == ENOSYS)
> > > +        FAIL_UNSUPPORTED ("POSIX message queues are not implemented\n");
> >
> > OK.
> >
> > > +      FAIL_EXIT1 ("Failed to create POSIX message queue: %m\n");
> > > +    }
> > > +
> > > +  TEST_VERIFY_EXIT (mq_unlink (m_name) == 0);
> > > +
> > > +  check_bz28213 ();
> > > +
> > > +  return 0;
> > > +}
> >
> > OK.
> >
> > > +
> > > +#include <support/test-driver.c>
> > > --
> > > 2.17.1
  

On 8/13/21 8:58 AM, Никита Попов wrote:
> Hello. I have some concerns about the recent commit
> 4cc79c217744743077bf7a0ec5e0a4318f1e6641. In rt/tst-bz28213.c we use
> one call to pause (). Should it return for some reason 'return 0' from
> do_test () follows. Maybe it is better to wrap pause () call here into
> an infinite loop like this? [while (1) pause ();] Then we can be sure
> that the only success in this test can be reached via exit (0) from
> the callback function.

The only way pause() can return is through a signal handler.  The only 
signal we handle in this test is SIGALRM (in the test driver for test 
timeout) and it results in a test failure.  All other signals will 
result in immediate process termination, which will also result in a 
test failure.

Siddhesh