threads/154893: pthread_sigmask don't work if mask and oldmask are passed the same pointer

[KOSAKI Motohiro]

Hi

> Note that POSIX requires the following prototypes for
> the sigprocmask and pthread_sigmask:
>
> int pthread_sigmask(int how, const sigset_t *restrict set,
>       sigset_t *restrict oset);
> int sigprocmask(int how, const sigset_t *restrict set,
>      sigset_t *restrict oset);

Ouch. right you are.

> The restrict keyword explicitely disallows the case of set == 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 == 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 !=
> 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 == 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=0; 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=0; 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 at pooma.home>
> Date:   Sat Feb 19 20:41:46 2011 +0200
>
>    Add 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   raise(int);
>  #if __POSIX_VISIBLE || __XSI_VISIBLE
>  int    kill(__pid_t, int);
>  int    pthread_kill(__pthread_t, int);
> -int    pthread_sigmask(int, const __sigset_t *, __sigset_t *);
> +int    pthread_sigmask(int, const __sigset_t *__restrict,
> +           __sigset_t * __restrict);
>  int    sigaction(int, const struct sigaction * __restrict,
>            struct sigaction * __restrict);
>  int    sigaddset(sigset_t *, int);
> diff --git a/share/man/man3/pthread_sigmask.3 b/share/man/man3/pthread_sigmask.3
> index c412543..013ba7c 100644
> --- a/share/man/man3/pthread_sigmask.3
> +++ b/share/man/man3/pthread_sigmask.3
> @@ -26,7 +26,7 @@
>  .\" EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  .\"
>  .\" $FreeBSD$
> -.Dd April 27, 2000
> +.Dd February 19, 2011
>  .Dt PTHREAD_SIGMASK 3
>  .Os
>  .Sh NAME
> @@ -38,7 +38,8 @@
>  .In pthread.h
>  .In signal.h
>  .Ft int
> -.Fn pthread_sigmask "int how" "const sigset_t *set" "sigset_t *oset"
> +.Fn pthread_sigmask "int how" "const sigset_t * restrict set" \
> +    "sigset_t * restrict oset"
>  .Sh DESCRIPTION
>  The
>  .Fn pthread_sigmask

Looks good.