This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/4] nptl: Fix Race conditions in pthread cancellation, (BZ#12683)


On Fri, 19 Sep 2014, Adhemerval Zanella wrote:

> +long int
> +__syscall_cancel (long int nr, long int arg1, long int arg2, long int arg3,
> +		  long int arg4, long int arg5, long int arg6)
> +{
> +  INTERNAL_SYSCALL_DECL (err);
> +  return INTERNAL_SYSCALL_NCS (nr, err, 6, arg1, arg2, arg3, arg4, arg5, arg6);
> +}

Avoid hardcoding "long" as syscall argument type.  It ought to be possible 
to use the same code for cases using "long long" (MIPS n32 at least) if 
you allow architectures to override the choice of type.

> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index aeba1ff..810deec 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -18,14 +18,12 @@
>  
>  #include <errno.h>
>  #include <signal.h>
> -#include "pthreadP.h"
> -#include "atomic.h"
> +#include <pthreadP.h>
> +#include <atomic.h>

Don't mix this sort of thing with substantive changes; send any patch for 
such #include style changes separately.  This will reduce the size of your 
patches....

>  #include <sysdep.h>
>  
> -
>  int
> -pthread_cancel (th)
> -     pthread_t th;
> +pthread_cancel (pthread_t th)
>  {

Likewise, send any such changes of style for function definitions 
separately.

> diff --git a/sysdeps/unix/sysv/linux/accept4.c b/sysdeps/unix/sysv/linux/accept4.c
> index a017224..a02dc35 100644
> --- a/sysdeps/unix/sysv/linux/accept4.c
> +++ b/sysdeps/unix/sysv/linux/accept4.c
> @@ -37,16 +37,8 @@
>  int
>  accept4 (int fd, __SOCKADDR_ARG addr, socklen_t *addr_len, int flags)
>  {
> -  if (SINGLE_THREAD_P)
> -    return INLINE_SYSCALL (accept4, 4, fd, addr.__sockaddr__, addr_len, flags);
> -
> -  int oldtype = LIBC_CANCEL_ASYNC ();
> -
> -  int result = INLINE_SYSCALL (accept4, 4, fd, addr.__sockaddr__, addr_len,
> +  int result = SYSCALL_CANCEL (accept4, fd, addr.__sockaddr__, addr_len,
>  			       flags);
> -
> -  LIBC_CANCEL_RESET (oldtype);
> -

For cases like this, it seems to me that you should be able to do a 
preliminary patch that defines SYSCALL_CANCEL in a way that expands to the 
existing code using LIBC_CANCEL_ASYNC, INLINE_SYSCALL and 
LIBC_CANCEL_RESET.  If you can define some of the new macros like that, 
then you can change most or all such code for all architectures to the new 
idiom (reasonably safely without being able to test on all the 
architectures), reducing the size of the per-architecture patches needed 
later and of the patches with the changes that actually fix the main 
issue.

> diff --git a/sysdeps/unix/sysv/linux/not-cancel.h b/sysdeps/unix/sysv/linux/not-cancel.h
> index 14f5e8b..a20aa01 100644
> --- a/sysdeps/unix/sysv/linux/not-cancel.h
> +++ b/sysdeps/unix/sysv/linux/not-cancel.h

These also look like changes that don't depend on the main changes.  
Anything not depending on the main changes should be separated out and go 
first.

-- 
Joseph S. Myers
joseph@codesourcery.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]