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

[Phil Blundell]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-nptl-tst-cancel27.c-nptl-tst-cancel28.c-New-tests.patch
Type: text/x-patch
Size: 4903 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20150907/1565fb22/attachment.bin>