Date: Sat, 19 Feb 2011 20:43:48 +0200 From: Kostik Belousov <kostikbel@gmail.com> To: KOSAKI Motohiro <kosaki.motohiro@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: <20110219184348.GK78089@deviant.kiev.zoral.com.ua> In-Reply-To: <201102191809.p1JI9rZi063561@red.freebsd.org> References: <201102191809.p1JI9rZi063561@red.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <pthread.h>
> #include <signal.h>
> #include <stdio.h>
>=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 <kostik@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_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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110219184348.GK78089>