From owner-freebsd-threads@FreeBSD.ORG Sat Feb 19 18:43:53 2011 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B6084106564A for ; Sat, 19 Feb 2011 18:43:53 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id 20B1F8FC19 for ; Sat, 19 Feb 2011 18:43:52 +0000 (UTC) Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id p1JIhnHD064595 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 19 Feb 2011 20:43:49 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4) with ESMTP id p1JIhnDe028810; Sat, 19 Feb 2011 20:43:49 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4/Submit) id p1JIhmun028809; Sat, 19 Feb 2011 20:43:48 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Sat, 19 Feb 2011 20:43:48 +0200 From: Kostik Belousov To: KOSAKI Motohiro Message-ID: <20110219184348.GK78089@deviant.kiev.zoral.com.ua> References: <201102191809.p1JI9rZi063561@red.freebsd.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="6dVRKsw9Fpbr3VPY" Content-Disposition: inline In-Reply-To: <201102191809.p1JI9rZi063561@red.freebsd.org> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.4 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00, DNS_FROM_OPENWHOIS autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: freebsd-threads@freebsd.org Subject: Re: threads/154893: pthread_sigmask don't work if mask and oldmask are passed the same pointer X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 19 Feb 2011 18:43:53 -0000 --6dVRKsw9Fpbr3VPY Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Feb 19, 2011 at 06:09:53PM +0000, KOSAKI Motohiro wrote: >=20 > >Number: 154893 > >Category: threads > >Synopsis: pthread_sigmask don't work if mask and oldmask are passe= d the same pointer > >Confidential: no > >Severity: non-critical > >Priority: low > >Responsible: freebsd-threads > >State: open > >Quarter: =20 > >Keywords: =20 > >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 root@mason.cse.buffalo.edu:/\ > 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. >=20 > But, FreeBSD doesn't. Its pthread_sigmask behave the same as=20 > pthread_sigmask(SIG_SETMASK, NULL, &msk). It is very strange to me. >=20 > Sidenote: > man sigprocmask says its type is below. >=20 > int > sigprocmask(int how, const sigset_t * restrict set, > sigset_t * restrict oset); >=20 > It is not POSIX compliant nor user friendly. But the man page > clealy describe set=3D=3Doset is invalid. > At least, pthread_sigmask's man page shold be fixed if uthread maintainers > woun't fix this issue. 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); 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. On the other hand, pthread_sigmask(3) manpage needs update to add restrict, and include/signal.h also ought to be changed. >=20 >=20 > 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 >=20 > ------------------------------------------------------ > #include > #include > #include >=20 > void* func(void* arg) > { > sigset_t old; > sigset_t add; > int i; >=20 > sigemptyset(&old); > pthread_sigmask(SIG_BLOCK, NULL, &old); >=20 > printf("before: "); > for (i=3D0; i<4; i++) > printf(" %08x", old.__bits[i]); > printf("\n"); >=20 > sigemptyset(&add); > sigaddset(&add, SIGUSR1); > pthread_sigmask(SIG_BLOCK, &add, NULL); > pthread_sigmask(SIG_BLOCK, NULL, &old); >=20 > printf("after: "); > for (i=3D0; i<4; i++) > printf(" %08x", old.__bits[i]); > printf("\n"); >=20 > return 0; > } >=20 > void* func2(void* arg) > { > sigset_t old; > sigset_t add; > int i; >=20 > sigemptyset(&old); > pthread_sigmask(SIG_BLOCK, NULL, &old); >=20 > printf("before: "); > for (i=3D0; i<4; i++) > printf(" %08x", old.__bits[i]); > printf("\n"); >=20 > sigemptyset(&add); > sigaddset(&add, SIGUSR1); > pthread_sigmask(SIG_BLOCK, &add, &old); >=20 > printf("after: "); > for (i=3D0; i<4; i++) > printf(" %08x", old.__bits[i]); > printf("\n"); >=20 > return 0; > } >=20 > int main(void) > { > pthread_t thr; > void* ret; >=20 > printf("correct case: \n"); > pthread_create(&thr, NULL, func, NULL); > pthread_join(thr, &ret); >=20 > printf("incorrect case: \n"); > pthread_create(&thr, NULL, func2, NULL); > pthread_join(thr, &ret); >=20 >=20 > return 0; > } >=20 >=20 > >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 =3D _get_curthread(); > sigset_t sigset; > int ret =3D 0; >=20 > /* Check if the existing signal process mask is to be returned: */ > if (oset !=3D NULL) { > /* Return the current mask: */ > *oset =3D curthread->sigmask; // (1) > } > /* Check if a new signal set was provided by the caller: */ > if (set !=3D NULL) { >=20 > (snip) >=20 > } > ---------------------------------------------------- >=20 > 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. 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. 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 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_sigm= ask.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 --6dVRKsw9Fpbr3VPY Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iEYEARECAAYFAk1gD2QACgkQC3+MBN1Mb4hA4ACfT9c/CjA8sySNsniK0KBnDKSN ne8AnjgDnRM4SC0mskWyh3+CdlnNw7zJ =h3U2 -----END PGP SIGNATURE----- --6dVRKsw9Fpbr3VPY--