This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 1/4] nptl: Fix Race conditions in pthread cancellation, (BZ#12683)
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- Cc: "GNU C. Library" <libc-alpha at sourceware dot org>
- Date: Fri, 19 Sep 2014 16:58:22 +0000
- Subject: Re: [PATCH 1/4] nptl: Fix Race conditions in pthread cancellation, (BZ#12683)
- Authentication-results: sourceware.org; auth=none
- References: <541C2901 dot 1050609 at linux dot vnet dot ibm dot com> <541C2DC8 dot 3010505 at linux dot vnet dot ibm dot com>
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