threads/154893: pthread_sigmask don't work if mask and oldmask are passed the same pointer
Locked
 
|
|
On Sat, Feb 19, 2011 at 06:09:53PM +0000, KOSAKI Motohiro wrote:
> > >Number: 154893 > >Category: threads > >Synopsis: pthread_sigmask don't work if mask and oldmask are passed the same pointer > >Confidential: no > >Severity: non-critical > >Priority: low > >Responsible: freebsd-threads > >State: open > >Quarter: > >Keywords: > >Date-Required: > >Class: sw-bug > >Submitter-Id: current-users > >Arrival-Date: Sat Feb 19 18:10:12 UTC 2011 > >Closed-Date: > >Last-Modified: > >Originator: KOSAKI Motohiro > >Release: 8.1 > >Organization: > >Environment: > FreeBSD FreeBSD8 8.1-RELEASE FreeBSD 8.1-RELEASE #0: Mon Jul 19 02:36:49 UTC 2010 [hidden email]:/\ > usr/obj/usr/src/sys/GENERIC amd64 > >Description: > Programmers expect pthread_sigmask(SIG_SETMASK, &msk, &msk) mean > 1) rewritten signal mask by msk. > 2) and, return old signal mask to msk. > > But, FreeBSD doesn't. Its pthread_sigmask behave the same as > pthread_sigmask(SIG_SETMASK, NULL, &msk). It is very strange to me. > > Sidenote: > man sigprocmask says its type is below. > > int > sigprocmask(int how, const sigset_t * restrict set, > sigset_t * restrict oset); > > It is not POSIX compliant nor user friendly. But the man page > clealy describe set==oset is invalid. > At least, pthread_sigmask's man page shold be fixed if uthread maintainers > woun't fix this issue. 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); The restrict keyword explicitely disallows the case of set == oset, so I think the behaviour is undefined by POSIX, and our implementation is correct. On the other hand, pthread_sigmask(3) manpage needs update to add restrict, and include/signal.h also ought to be changed. > > > Sidenote2: > This is a source of signal breakage of ruby trunk. > http://redmine.ruby-lang.org/issues/show/4173 I would say that ruby has a bug then. > >How-To-Repeat: > run following program > > ------------------------------------------------------ > #include <pthread.h> > #include <signal.h> > #include <stdio.h> > > void* func(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, NULL); > pthread_sigmask(SIG_BLOCK, NULL, &old); > > printf("after: "); > for (i=0; i<4; i++) > printf(" %08x", old.__bits[i]); > printf("\n"); > > return 0; > } > > 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, &old); > > printf("after: "); > for (i=0; i<4; i++) > printf(" %08x", old.__bits[i]); > printf("\n"); > > return 0; > } > > int main(void) > { > pthread_t thr; > void* ret; > > printf("correct case: \n"); > pthread_create(&thr, NULL, func, NULL); > pthread_join(thr, &ret); > > printf("incorrect case: \n"); > pthread_create(&thr, NULL, func2, NULL); > pthread_join(thr, &ret); > > > return 0; > } > > > >Fix: > /usr/src/lib/libc_r/uthread/uthread_sigmask.c has following code. > ----------------------------------------------------------------- > int > _pthread_sigmask(int how, const sigset_t *set, sigset_t *oset) > { > struct pthread *curthread = _get_curthread(); > sigset_t sigset; > int ret = 0; > > /* Check if the existing signal process mask is to be returned: */ > if (oset != NULL) { > /* Return the current mask: */ > *oset = curthread->sigmask; // (1) > } > /* Check if a new signal set was provided by the caller: */ > if (set != NULL) { > > (snip) > > } > ---------------------------------------------------- > > Then, if set == oset, set argument was override before use it at (1). > To introduce temporary variable fix this issue easily. SIG_UNBLOCK. Moreover, the kernel side of sigprocmask(2) implementation first copies new set into kernel VA, and only then copies old mask out. 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. 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". The only change that I see as needed now is the following cosmetics: commit 49287c24fc46f342b46db1fae3fe9982bfbf7ed9 Author: Konstantin Belousov <[hidden email]> 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 |
attachment0 (203 bytes) |
|
|
|
On Sun, Feb 20, 2011 at 04:22:46AM +0900, KOSAKI Motohiro wrote:
> > 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. from the mail above, and get correct case: before: 00000000 00000000 00000000 00000000 after: 20000000 00000000 00000000 00000000 incorrect case: before: 00000000 00000000 00000000 00000000 after: 20000000 00000000 00000000 00000000 on amd64 machine, while code is compiled for 32 bit. I would say that the output is as expected by me. The old mask is copied out by kernel after new mask is copied in. Below is the exact source of the test I used. Could you, please, confirm that the test is right ? If not, please send me the exact source code that demonstrates your issue. Thanks. #include <pthread.h> #include <signal.h> #include <stdio.h> void* func(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, NULL); pthread_sigmask(SIG_BLOCK, NULL, &old); printf("after: "); for (i=0; i<4; i++) printf(" %08x", old.__bits[i]); printf("\n"); return 0; } 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; } int main(void) { pthread_t thr; void* ret; printf("correct case: \n"); pthread_create(&thr, NULL, func, NULL); pthread_join(thr, &ret); printf("incorrect case: \n"); pthread_create(&thr, NULL, func2, NULL); pthread_join(thr, &ret); return 0; } |
Loading...
| Powered by Nabble | Edit this page |