Re: [PATCH 08/08] nptl: arm: Fix Race conditions in pthread cancellation (BZ#12683)

[Phil Blundell <pb at pbcl dot net>]

On Mon, 2015-08-31 at 18:11 -0300, Adhemerval Zanella wrote:
> This patch adds the ARM modifications required for the BZ#12683 fix.
> It basically removes the enable_asynccancel/disable_asynccancel function
> usage on code, provide a arch-specific symbol that contains global
> markers to be used in SIGCANCEL handler.

I looked at this in a bit more detail.  Here are some further comments:

>  	.fnstart;	/* matched by the .fnend in UNDOARGS below.  */	\
> -	DOCARGS_##args;	/* save syscall args etc. around CENABLE.  */	\
> -	CENABLE;							\
> -	mov ip, r0;		/* put mask in safe place.  */		\
> -	UNDOCARGS_##args;	/* restore syscall args.  */		\
> -	ldr	r7, =SYS_ify (syscall_name);				\
> -	swi	0x0;		/* do the call.  */			\
> -	mov	r7, r0;		/* save syscall return value.  */	\
> -	mov	r0, ip;		/* get mask back.  */			\
> -	CDISABLE;							\
> -	mov	r0, r7;		/* retrieve return value.  */		\
> -	RESTORE_LR_##args;						\

After this change, DOCARGS(), UNDOCARGS() and RESTORE_LR() are all dead
code.  Please delete them.

> -	UNDOARGS_##args;						\
> +	push	{r4, r5, lr};						\
> +	.save   {r4, r5, lr};						\
> +	PSEUDO_CANCEL_BEFORE;						\
> +	movw	r0, SYS_ify (syscall_name);				\

This fails when compiling for non-thumb2.  Please use "ldr r0, =..."
instead.

> +	PSEUDO_CANCEL_AFTER;						\
> +	pop	{r4, r5, pc};						\

Popping {pc} here causes an immediate return, which means that the errno
handling code which follows is never executed.  Somewhat embarrassingly
it seems that there is no existing glibc test which catches this
failure.  I've attached a proof of concept which demonstrates it, but I
rather wonder whether we should extend the test harness so that
some/most of the existing glibc tests are run both single-threaded (as
now) and with an additional dummy thread created at startup in order to
force this code down the multi-threaded path.

Also note that the two testcases in the attached patch give slightly
different results and I think they would continue to do so (in a
different way) if the bug above was fixed.  It's not entirely clear to
me that this part of __syscall_cancel() from your other patch is
correct:

+  /* If cancellation is not enabled, call the syscall directly.  */
+  if (pd->cancelhandling & CANCELSTATE_BITMASK)
+    {
+      INTERNAL_SYSCALL_DECL (err);
+      result = INTERNAL_SYSCALL_NCS (nr, err, 6, a1, a2, a3, a4, a5, a6);
+      return INTERNAL_SYSCALL_ERROR_P (result, err) ? -result : result;
+    }

p.

From f1bf4248ba69931442f045a9b6b71c0c3dfcced4 Mon Sep 17 00:00:00 2001
From: Phil Blundell <philb@gnu.org>
Date: Mon, 7 Sep 2015 17:01:27 +0000
Subject: [PATCH] nptl/tst-cancel27.c, nptl/tst-cancel28.c: New tests

---
 nptl/Makefile       |  2 +-
 nptl/tst-cancel27.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++
 nptl/tst-cancel28.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 141 insertions(+), 1 deletion(-)
 create mode 100644 nptl/tst-cancel27.c
 create mode 100644 nptl/tst-cancel28.c

diff --git a/nptl/Makefile b/nptl/Makefile
index 4bc6608..05a4872 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -255,7 +255,7 @@ tests = tst-typesizes \
 	tst-cancel11 tst-cancel12 tst-cancel13 tst-cancel14 tst-cancel15 \
 	tst-cancel16 tst-cancel17 tst-cancel18 tst-cancel19 tst-cancel20 \
 	tst-cancel21 tst-cancel22 tst-cancel23 tst-cancel24 tst-cancel25 \
-	tst-cancel26 \
+	tst-cancel26 tst-cancel27 tst-cancel28 \
 	tst-cancel-self tst-cancel-self-cancelstate \
 	tst-cancel-self-canceltype tst-cancel-self-testcancel \
 	tst-cleanup0 tst-cleanup1 tst-cleanup2 tst-cleanup3 tst-cleanup4 \
diff --git a/nptl/tst-cancel27.c b/nptl/tst-cancel27.c
new file mode 100644
index 0000000..2d71a7f
--- /dev/null
+++ b/nptl/tst-cancel27.c
@@ -0,0 +1,69 @@
+/* Copyright (C) 2015 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+static void *
+tf (void *arg)
+{
+  int e, r;
+
+  errno = 0;
+
+  /* This is a cancellation point, but we should not be cancelled.  */
+  r = write (-1, 0, 0);
+  e = errno;
+
+  /* Check that the cancelling syscall wrapper has handled the error correctly.  */
+  if (r != -1 || errno != EBADF)
+    {
+      printf ("write returned %d, errno %d\n", r, e);
+      exit (1);
+    }
+
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_t th;
+
+  if (pthread_create (&th, NULL, tf, NULL) != 0)
+    {
+      puts ("create failed");
+      exit (1);
+    }
+
+  void *r;
+  if (pthread_join (th, &r) != 0)
+    {
+      puts ("join failed");
+      exit (1);
+    }
+
+  return 0;
+}
+
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/nptl/tst-cancel28.c b/nptl/tst-cancel28.c
new file mode 100644
index 0000000..4dccae2
--- /dev/null
+++ b/nptl/tst-cancel28.c
@@ -0,0 +1,71 @@
+/* Copyright (C) 2015 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+static void *
+tf (void *arg)
+{
+  int e, r;
+
+  errno = 0;
+
+  pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
+
+  /* This is a cancellation point, but we should not be cancelled.  */
+  r = write (-1, 0, 0);
+  e = errno;
+
+  /* Check that the cancelling syscall wrapper has handled the error correctly.  */
+  if (r != -1 || errno != EBADF)
+    {
+      printf ("write returned %d, errno %d\n", r, e);
+      exit (1);
+    }
+
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_t th;
+
+  if (pthread_create (&th, NULL, tf, NULL) != 0)
+    {
+      puts ("create failed");
+      exit (1);
+    }
+
+  void *r;
+  if (pthread_join (th, &r) != 0)
+    {
+      puts ("join failed");
+      exit (1);
+    }
+
+  return 0;
+}
+
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
-- 
2.1.4