Date: Sun, 20 Feb 2011 04:22:46 +0900 From: KOSAKI Motohiro <kosaki.motohiro@gmail.com> To: Kostik Belousov <kostikbel@gmail.com> Cc: freebsd-threads@freebsd.org Subject: Re: threads/154893: pthread_sigmask don't work if mask and oldmask are passed the same pointer Message-ID: <AANLkTinEZwBOoDaU2-7Y9svch1nd4EAyoYyH%2B4V2v_Od@mail.gmail.com> In-Reply-To: <20110219184348.GK78089@deviant.kiev.zoral.com.ua> References: <201102191809.p1JI9rZi063561@red.freebsd.org> <20110219184348.GK78089@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi
> Note that POSIX requires the following prototypes for
> the sigprocmask and pthread_sigmask:
>
> int pthread_sigmask(int how, const sigset_t *restrict set,
> =A0 =A0 =A0 sigset_t *restrict oset);
> int sigprocmask(int how, const sigset_t *restrict set,
> =A0 =A0 =A0sigset_t *restrict oset);
Ouch. right you are.
> The restrict keyword explicitely disallows the case of set =3D=3D oset,
> so I think the behaviour is undefined by POSIX, and our implementation
> is correct.
I agree.
> On the other hand, pthread_sigmask(3) manpage needs update to add
> restrict, and include/signal.h also ought to be changed.
OK, thanks.
>> Then, if set =3D=3D oset, set argument was override before use it at (1)=
.
>> To introduce temporary variable fix this issue easily.
> libc_r is unused. Real implementation is in lib/libthr, that already
> has a twist that would not allow the override for case of action !=3D
> SIG_UNBLOCK. Moreover, the kernel side of sigprocmask(2) implementation
> first copies new set into kernel VA, and only then copies old mask out.
Hmhm.
> Your example does not supply oset =3D=3D set to the pthread_sigmask,
> meantime. I really do not see anything wrong with the output of
> the program that supposed to illustrate the issue.
Oh, I'm sorry. It's cut-n-paste mistake. I did paste old version
unintensionally.
void* func2(void* arg)
{
sigset_t old;
sigset_t add;
int i;
sigemptyset(&old);
pthread_sigmask(SIG_BLOCK, NULL, &old);
printf("before: ");
for (i=3D0; i<4; i++)
printf(" %08x", old.__bits[i]);
printf("\n");
sigemptyset(&add);
sigaddset(&add, SIGUSR1);
pthread_sigmask(SIG_BLOCK, &add, &add);
pthread_sigmask(SIG_BLOCK, NULL, &old);
printf("after: ");
for (i=3D0; i<4; i++)
printf(" %08x", old.__bits[i]);
printf("\n");
return 0;
}
The result is,
correct case:
before: 00000000 00000000 00000000 00000000
after: 20000000 00000000 00000000 00000000
incorrect case:
before: 00000000 00000000 00000000 00000000
after: 00000000 00000000 00000000 00000000
difference between func and func2 are
- pthread_sigmask(SIG_BLOCK, &add, NULL);
+ pthread_sigmask(SIG_BLOCK, &add, &add);
That said, To add oset changed sigprocmask() behavior significantly.
> The func() adds SIGUSR1 to the mask with second call to
> pthread_sigmask(SIG_BLOCK, &add, NULL);. Then, third call
> correctly returns SIGUSR1 in the mask.
>
> On the other hand, func2() sets SIGUSR1 as blocked and retrieves
> the previous mask in single atomic action. Since SIGUSR1 was not
> blocked before the call, old sigset correctly indicates it as
> "not blocked".
(snip)
> The only change that I see as needed now is the following cosmetics:
>
> commit 49287c24fc46f342b46db1fae3fe9982bfbf7ed9
> Author: Konstantin Belousov <kostik@pooma.home>
> Date: =A0 Sat Feb 19 20:41:46 2011 +0200
>
> =A0 =A0Add restrict keyword to pthread_sigmask prototype and manpage
>
> diff --git a/include/signal.h b/include/signal.h
> index 4a4cd17..52a611c 100644
> --- a/include/signal.h
> +++ b/include/signal.h
> @@ -69,7 +69,8 @@ int =A0 raise(int);
> =A0#if __POSIX_VISIBLE || __XSI_VISIBLE
> =A0int =A0 =A0kill(__pid_t, int);
> =A0int =A0 =A0pthread_kill(__pthread_t, int);
> -int =A0 =A0pthread_sigmask(int, const __sigset_t *, __sigset_t *);
> +int =A0 =A0pthread_sigmask(int, const __sigset_t *__restrict,
> + =A0 =A0 =A0 =A0 =A0 __sigset_t * __restrict);
> =A0int =A0 =A0sigaction(int, const struct sigaction * __restrict,
> =A0 =A0 =A0 =A0 =A0 =A0struct sigaction * __restrict);
> =A0int =A0 =A0sigaddset(sigset_t *, int);
> diff --git a/share/man/man3/pthread_sigmask.3 b/share/man/man3/pthread_si=
gmask.3
> index c412543..013ba7c 100644
> --- a/share/man/man3/pthread_sigmask.3
> +++ b/share/man/man3/pthread_sigmask.3
> @@ -26,7 +26,7 @@
> =A0.\" EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> =A0.\"
> =A0.\" $FreeBSD$
> -.Dd April 27, 2000
> +.Dd February 19, 2011
> =A0.Dt PTHREAD_SIGMASK 3
> =A0.Os
> =A0.Sh NAME
> @@ -38,7 +38,8 @@
> =A0.In pthread.h
> =A0.In signal.h
> =A0.Ft int
> -.Fn pthread_sigmask "int how" "const sigset_t *set" "sigset_t *oset"
> +.Fn pthread_sigmask "int how" "const sigset_t * restrict set" \
> + =A0 =A0"sigset_t * restrict oset"
> =A0.Sh DESCRIPTION
> =A0The
> =A0.Fn pthread_sigmask
Looks good.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTinEZwBOoDaU2-7Y9svch1nd4EAyoYyH%2B4V2v_Od>