diff mbox

[BZ,#18080] S390: Fix setcontext/swapcontext which are not restoring sigmask.

Message ID md6q4p$556$1@ger.gmane.org
State Superseded
Headers show

Commit Message

Stefan Liebler March 4, 2015, 11:26 a.m. UTC
Hi,

on s390/s390x, a call to setcontext or swapcontext does not restore the 
signal mask.
This can be reproduced with the following pseudocode:
-getcontext()
-block signal with sigprocmask
-setcontext()
-signal shouldn´t be blocked anymore.

The corresponding sigprocmask calls in files:
sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S
sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S
sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S
sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S
are using SIG_BLOCK instead of SIG_SETMASK.

This patch fixes this behaviour and adds a new testcase,
which checks signal mask after setcontext()/swapcontext() calls
and checks, if a pending signal was delivered after those calls.

Testsuite runs without regression on s390 and checked if new testcase 
passes on x86_64.

Ok to commit?

Bye
Stefan

---
2015-03-04  Stefan Liebler  <stli@linux.vnet.ibm.com>

	[BZ #18080]
	* sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S
	(__setcontext): Use SIG_SETMASK instead of SIG_BLOCK.
	* sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S
	(__swapcontext): Likewise.
	* sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S
	(__setcontext): Likewise.
	* sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S
	(__swapcontext): Likewise.
	* stdlib/Makefile (tests): Add new testcase tst-setcontext2.
	* stdlib/tst-setcontext2.c: New file.

Comments

Andreas Schwab March 4, 2015, 11:48 a.m. UTC | #1
Stefan Liebler <stli@linux.vnet.ibm.com> writes:

> diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S b/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S
> index c40f465..c385110 100644
> --- a/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S
> +++ b/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S
> @@ -75,7 +75,7 @@ ENTRY(__swapcontext)
>  	stm     %r0,%r15,SC_GPRS(%r1)
>  
>  	/* sigprocmask (SIG_SETMASK, &sc->sc_mask, NULL).  */
> -	la      %r2,SIG_BLOCK
> +	la      %r2,SIG_SETMASK
>  	lr	%r5,%r0
>  	la	%r3,SC_MASK(%r5)
>  	slr	%r4,%r4

There is no need to call sigprocmask twice, you can swap the mask with a
single call.  Also, %r2 is clobbered here.

Andreas.
Florian Weimer March 4, 2015, 11:49 a.m. UTC | #2
On 03/04/2015 12:26 PM, Stefan Liebler wrote:
> +volatile int handlerCalled;

I think this should use sigatomic_t.

There are a few missing spaces in function calls:

> +check(const char *funcName)
> +  sigprocmask(SIG_BLOCK, NULL, &set);
> +  if (sigismember(&set, SIGUSR2) != 0)
> +  if (sigismember(&set, SIGUSR1) == 0)
> +  if (sigismember(&set, SIGUSR2) == 0)
> +  if (sigismember(&oldctx.uc_sigmask, SIGUSR2) == 0)
> +  if (sigismember(&oldctx.uc_sigmask, SIGUSR1) == 0)

Sorry, no other comments at this time.
Joseph Myers March 4, 2015, 2:11 p.m. UTC | #3
The first line of a new file (stdlib/tst-setcontext2.c in this case) 
should be the comment describing that file.  The copyright notice should 
come after that, not be the first line.
diff mbox

Patch

diff --git a/stdlib/Makefile b/stdlib/Makefile
index 92ceca7..c78fdb5 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -66,11 +66,11 @@  test-srcs	:= tst-fmtmsg
 tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   test-canon test-canon2 tst-strtoll tst-environ	    \
 		   tst-xpg-basename tst-random tst-random2 tst-bsearch	    \
-		   tst-limits tst-rand48 bug-strtod tst-setcontext	    \
-		   test-a64l tst-qsort tst-system testmb2 bug-strtod2	    \
-		   tst-atof1 tst-atof2 tst-strtod2 tst-strtod3 tst-rand48-2 \
-		   tst-makecontext tst-strtod4 tst-strtod5 tst-qsort2	    \
-		   tst-makecontext2 tst-strtod6 tst-unsetenv1		    \
+		   tst-limits tst-rand48 bug-strtod tst-setcontext          \
+		   tst-setcontext2 test-a64l tst-qsort tst-system testmb2   \
+		   bug-strtod2 tst-atof1 tst-atof2 tst-strtod2 tst-strtod3  \
+		   tst-rand48-2 tst-makecontext tst-strtod4 tst-strtod5     \
+		   tst-qsort2 tst-makecontext2 tst-strtod6 tst-unsetenv1    \
 		   tst-makecontext3 bug-getcontext bug-fmtmsg1		    \
 		   tst-secure-getenv tst-strtod-overflow tst-strtod-round   \
 		   tst-tininess tst-strtod-underflow tst-tls-atexit
diff --git a/stdlib/tst-setcontext2.c b/stdlib/tst-setcontext2.c
new file mode 100644
index 0000000..22566d8
--- /dev/null
+++ b/stdlib/tst-setcontext2.c
@@ -0,0 +1,219 @@ 
+/* Copyright (C) 2015 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <signal.h>
+#include <ucontext.h>
+#include <unistd.h>
+
+volatile int global;
+volatile int handlerCalled;
+
+static void
+check(const char *funcName)
+{
+  sigset_t set;
+
+  /* check if SIGUSR2 is unblocked after setcontext-call.  */
+  sigprocmask(SIG_BLOCK, NULL, &set);
+
+  if (sigismember(&set, SIGUSR2) != 0)
+    {
+      printf ("FAIL: SIGUSR2 is blocked after %s.\n", funcName);
+      exit (1);
+    }
+
+  if (sigismember(&set, SIGUSR1) == 0)
+    {
+      printf ("FAIL: SIGUSR1 is not blocked after %s.\n", funcName);
+      exit (1);
+    }
+}
+
+static void
+signalmask (int how, int signum)
+{
+  sigset_t set;
+  sigemptyset (&set);
+  sigaddset (&set, signum);
+  if (sigprocmask (how, &set, NULL) != 0)
+    {
+      printf ("FAIL: sigprocmaks (%d, %d, NULL): %m\n", how, signum);
+      exit (1);
+    }
+}
+
+static void
+signalpending (int signum, const char *msg)
+{
+  sigset_t set;
+  sigemptyset (&set);
+  if (sigpending (&set) != 0)
+    {
+      printf ("FAIL: sigpending: %m\n");
+      exit (1);
+    }
+  if (sigismember(&set, SIGUSR2) == 0)
+    {
+      printf ("FAIL: Signal %d is not pending %s\n", signum, msg);
+      exit (1);
+    }
+}
+
+static void
+handler (int __attribute__ ((unused)) signum)
+{
+  handlerCalled ++;
+}
+
+static int
+do_test (void)
+{
+  /* This test checks, if setcontext, swapcontext restores sig-mask.  */
+
+  ucontext_t ctx, oldctx;
+  struct sigaction action;
+  pid_t pid;
+
+  pid = getpid ();
+
+  /* unblock SIGUSR2 */
+  signalmask (SIG_UNBLOCK, SIGUSR2);
+
+  /* block SIGUSR1 */
+  signalmask (SIG_BLOCK, SIGUSR1);
+
+  /* register handler for SIGUSR2  */
+  action.sa_flags = 0;
+  action.sa_handler = handler;
+  sigemptyset (&action.sa_mask);
+  sigaction (SIGUSR2, &action, NULL);
+
+  if (getcontext (&ctx) != 0)
+    {
+      printf ("FAIL: getcontext: %m\n");
+      exit (1);
+    }
+
+  global++;
+
+  if (global == 1)
+    {
+      puts ("after getcontext");
+
+      /* block SIGUSR2  */
+      signalmask (SIG_BLOCK, SIGUSR2);
+
+      /* send SIGUSR2 to me  */
+      handlerCalled = 0;
+      kill (pid, SIGUSR2);
+
+      /* was SIGUSR2 handler called?  */
+      if (handlerCalled != 0)
+	{
+	  puts ("FAIL: signal handler was called, but signal was blocked.");
+	  exit (1);
+	}
+
+      /* is SIGUSR2 pending?  */
+      signalpending (SIGUSR2, "before setcontext");
+
+      /* SIGUSR2 will be unblocked by setcontext-call.  */
+      if (setcontext (&ctx) != 0)
+	{
+	  printf ("FAIL: setcontext: %m\n");
+	  exit (1);
+	}
+    }
+  else if (global == 2)
+    {
+      puts ("after setcontext");
+
+      /* check SIGUSR1/2  */
+      check ("setcontext");
+
+      /* was SIGUSR2 handler called? */
+      if (handlerCalled != 1)
+	{
+	  puts ("FAIL: signal handler was not called after setcontext.");
+	  exit (1);
+	}
+
+      /* block SIGUSR2 */
+      signalmask (SIG_BLOCK, SIGUSR2);
+
+      /* send SIGUSR2 to me  */
+      handlerCalled = 0;
+      kill (pid, SIGUSR2);
+
+      /* was SIGUSR2 handler called?  */
+      if (handlerCalled != 0)
+	{
+	  puts ("FAIL: signal handler was called, but signal was blocked.");
+	  exit (1);
+	}
+
+      /* is SIGUSR2 pending?  */
+      signalpending (SIGUSR2, "before swapcontext");
+
+      if (swapcontext (&oldctx, &ctx) != 0)
+	{
+	  printf ("FAIL: swapcontext: %m\n");
+	  exit (1);
+	}
+
+      puts ("FAIL: returned from swapcontext");
+      exit (1);
+    }
+  else if ( global != 3 )
+    {
+      puts ("FAIL: 'global' not incremented three times");
+      exit (1);
+    }
+
+  puts ("after swapcontext");
+  /* check SIGUSR1/2  */
+  check ("swapcontext");
+
+  /* was SIGUSR2 handler called? */
+  if (handlerCalled != 1)
+    {
+      puts ("FAIL: signal handler was not called after swapcontext.");
+      exit (1);
+    }
+
+  /* check sigmask in old context of swapcontext-call  */
+  if (sigismember(&oldctx.uc_sigmask, SIGUSR2) == 0)
+    {
+      puts ("FAIL: SIGUSR2 is not blocked in oldctx.uc_sigmask.");
+      exit (1);
+    }
+
+  if (sigismember(&oldctx.uc_sigmask, SIGUSR1) == 0)
+    {
+      puts ("FAIL: SIGUSR1 is not blocked in oldctx.uc_sigmaks.");
+      exit (1);
+    }
+
+  puts ("test succeeded");
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S b/sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S
index 22fb3ae..59cf4a8 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S
+++ b/sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S
@@ -34,7 +34,7 @@  ENTRY(__setcontext)
 	lr	%r1,%r2
 
 	/* rt_sigprocmask (SIG_SETMASK, &sc->sc_mask, NULL, sigsetsize).  */
-	la      %r2,SIG_BLOCK
+	la      %r2,SIG_SETMASK
 	la	%r3,SC_MASK(%r1)
 	slr	%r4,%r4
 	lhi	%r5,_NSIG8
diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S b/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S
index c40f465..c385110 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S
+++ b/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S
@@ -75,7 +75,7 @@  ENTRY(__swapcontext)
 	stm     %r0,%r15,SC_GPRS(%r1)
 
 	/* sigprocmask (SIG_SETMASK, &sc->sc_mask, NULL).  */
-	la      %r2,SIG_BLOCK
+	la      %r2,SIG_SETMASK
 	lr	%r5,%r0
 	la	%r3,SC_MASK(%r5)
 	slr	%r4,%r4
diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S b/sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S
index d83fa42..388c7d6 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S
+++ b/sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S
@@ -34,7 +34,7 @@  ENTRY(__setcontext)
 	lgr	%r1,%r2
 
 	/* sigprocmask (SIG_SETMASK, &sc->sc_mask, NULL).  */
-	la      %r2,SIG_BLOCK
+	la      %r2,SIG_SETMASK
 	la	%r3,SC_MASK(%r1)
 	slgr	%r4,%r4
 	lghi	%r5,_NSIG8
diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S b/sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S
index d36a623..bd7d9cd 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S
+++ b/sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S
@@ -75,7 +75,7 @@  ENTRY(__swapcontext)
 	stmg    %r0,%r15,SC_GPRS(%r1)
 
 	/* rt_sigprocmask (SIG_SETMASK, &sc->sc_mask, NULL, sigsetsize).  */
-	la      %r2,SIG_BLOCK
+	la      %r2,SIG_SETMASK
 	lgr	%r5,%r0
 	la	%r3,SC_MASK(%r5)
 	lghi	%r5,_NSIG8