* [PATCH] fix broken 2.4.x rt_sigprocmask error handling
@ 2003-12-31 11:45 Erik Andersen
2004-01-05 13:23 ` Marcelo Tosatti
0 siblings, 1 reply; 3+ messages in thread
From: Erik Andersen @ 2003-12-31 11:45 UTC (permalink / raw)
To: linux-kernel; +Cc: Marcelo Tosatti
[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]
SuSv3 says:
"The argument 'how' indicates the way in which the set is
changed, and the application shall ensure it consists of
one of [SIG_BLOCK, SIG_UNBLOCK, SIG_SETMASK] ... If
sigprocmask() fails, the thread's signal mask shall not
be changed."
The sigprocmask(2) syscall implements this correctly, however the
rt_sigprocmask(2) syscall in 2.4.x does not comply with SuSv3.
When this syscall is provided with a valid 'set' value, and a
bogus value for 'how', the process signal mask is still changed.
The attached test application demonstrates the problem. This
patch below fixes it. Please apply!
--- linux/kernel/signal.c.orig 2003-12-31 04:01:59.000000000 -0700
+++ linux/kernel/signal.c 2003-12-31 04:38:06.000000000 -0700
@@ -879,16 +879,16 @@
error = -EINVAL;
break;
case SIG_BLOCK:
- sigorsets(&new_set, &old_set, &new_set);
+ sigorsets(¤t->blocked, &old_set, &new_set);
break;
case SIG_UNBLOCK:
- signandsets(&new_set, &old_set, &new_set);
+ signandsets(¤t->blocked, &old_set, &new_set);
break;
case SIG_SETMASK:
+ current->blocked = new_set;
break;
}
- current->blocked = new_set;
recalc_sigpending(current);
spin_unlock_irq(¤t->sigmask_lock);
if (error)
--- linux/CREDITS 2003-12-02 13:04:50.000000000 -0700
+++ linux/CREDITS 2003-12-02 13:04:50.000000000 -0700
@@ -82,13 +82,13 @@
S: USA
N: Erik Andersen
-E: andersee@debian.org
-W: http://www.xmission.com/~andersen
-P: 1024/FC4CFFED 78 3C 6A 19 FA 5D 92 5A FB AC 7B A5 A5 E1 FF 8E
+E: andersen@codepoet.org
+W: http://www.codepoet.org/
+P: 1024D/30D39057 1BC4 2742 E885 E4DE 9301 0C82 5F9B 643E 30D3 9057
D: Maintainer of ide-cd and Uniform CD-ROM driver,
D: ATAPI CD-Changer support, Major 2.1.x CD-ROM update.
-S: 4538 South Carnegie Tech Street
-S: Salt Lake City, Utah 84120
+S: 352 North 525 East
+S: Springville, Utah 84663
S: USA
N: Michael Ang
-Erik
--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--
[-- Attachment #2: test-setprocmask.c --]
[-- Type: text/x-csrc, Size: 1996 bytes --]
#define _GNU_SOURCE
#include <stdio.h>
#include <signal.h>
#include <errno.h>
#include <sys/syscall.h>
#if 0
/* The sigprocmask syscall works correctly */
_syscall3(int, sigprocmask, int, how, const sigset_t *, set,
sigset_t *, oldset);
#else
/* The rt_sigprocmask syscall currently fails */
_syscall4(int, rt_sigprocmask, int, how, const sigset_t *, set,
sigset_t *, oldset, size_t, size);
int sigprocmask(int how, const sigset_t *set, sigset_t *oldset)
{
return rt_sigprocmask(how, set, oldset, _NSIG/8);
}
#endif
int main(void)
{
sigset_t set;
sigset_t oset;
int r;
sigemptyset(&set);
sigemptyset(&oset);
/* Block no signals since set is empty.
* oset will be set to the app default. */
errno = 0;
r = sigprocmask(SIG_SETMASK, &set, &oset);
if (r != 0) {
printf("r=%d : %m\n", r);
}
/* Block no signals since set is empty.
* oset will now also be empty. */
errno = 0;
r = sigprocmask(SIG_SETMASK, &set, &oset);
if (r != 0) {
printf("r=%d : %m\n", r);
}
/* * SuSv3 says:
* "The * argument how indicates the way in which the set is
* changed, and the application shall ensure it consists of one
* of [SIG_BLOCK, SIG_UNBLOCK, SIG_SETMASK] ... If sigprocmask()
* fails, the thread's signal mask shall not be changed."
*
* Attempt to block SIGHUP with a bogus 'how'. This should fail,
* and return EINVAL, and oset should should still be empty. */
errno = 0;
r = sigaddset(&set, SIGHUP);
if (r != 0) {
printf("r=%d : %m\n", r);
}
errno = 0;
r = sigprocmask(0xdeadbeef, &set, &oset);
if (r != 0) {
printf("Expected failure: %m\n");
}
/* Query the value of oset. It should still be empty */
errno = 0;
r = sigprocmask(SIG_SETMASK, (sigset_t *)0, &oset);
if (r != 0) {
printf("r=%d : %m\n", r);
}
/* But oset is not empty! This is contrary to SuSv3! */
if (sigismember(&oset, SIGHUP)==1) {
printf("FAIL -- oset was changed when we supplied an invalid 'how'\n");
return 1;
} else {
printf("PASS\n");
}
return 0;
}
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] fix broken 2.4.x rt_sigprocmask error handling
2003-12-31 11:45 [PATCH] fix broken 2.4.x rt_sigprocmask error handling Erik Andersen
@ 2004-01-05 13:23 ` Marcelo Tosatti
2004-01-05 13:56 ` Erik Andersen
0 siblings, 1 reply; 3+ messages in thread
From: Marcelo Tosatti @ 2004-01-05 13:23 UTC (permalink / raw)
To: Erik Andersen; +Cc: linux-kernel, Marcelo Tosatti
On Wed, 31 Dec 2003, Erik Andersen wrote:
> SuSv3 says:
>
> "The argument 'how' indicates the way in which the set is
> changed, and the application shall ensure it consists of
> one of [SIG_BLOCK, SIG_UNBLOCK, SIG_SETMASK] ... If
> sigprocmask() fails, the thread's signal mask shall not
> be changed."
>
> The sigprocmask(2) syscall implements this correctly, however the
> rt_sigprocmask(2) syscall in 2.4.x does not comply with SuSv3.
> When this syscall is provided with a valid 'set' value, and a
> bogus value for 'how', the process signal mask is still changed.
>
> The attached test application demonstrates the problem. This
> patch below fixes it. Please apply!
>
>
> --- linux/kernel/signal.c.orig 2003-12-31 04:01:59.000000000 -0700
> +++ linux/kernel/signal.c 2003-12-31 04:38:06.000000000 -0700
> @@ -879,16 +879,16 @@
> error = -EINVAL;
> break;
> case SIG_BLOCK:
> - sigorsets(&new_set, &old_set, &new_set);
> + sigorsets(¤t->blocked, &old_set, &new_set);
> break;
> case SIG_UNBLOCK:
> - signandsets(&new_set, &old_set, &new_set);
> + signandsets(¤t->blocked, &old_set, &new_set);
> break;
> case SIG_SETMASK:
> + current->blocked = new_set;
> break;
> }
>
> - current->blocked = new_set;
> recalc_sigpending(current);
> spin_unlock_irq(¤t->sigmask_lock);
> if (error)
Hi Erik,
For how long the behaviour has been this? It is broken, but its brokenness
is harmless.
Maybe some app rely on this behaviour? (I understand its wrong, but
still).
Is there any major distribution which includes this fix?
Anyway, isnt it easier to move the "if (error)" up like this?
--- signal.c.orig 2004-01-05 11:13:17.000000000 -0200
+++ signal.c 2004-01-05 11:14:09.000000000 -0200
@@ -888,11 +888,12 @@
break;
}
+ if (error)
+ goto out;
+
current->blocked = new_set;
recalc_sigpending(current);
spin_unlock_irq(¤t->sigmask_lock);
- if (error)
- goto out;
if (oset)
goto set_old;
} else if (oset) {
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] fix broken 2.4.x rt_sigprocmask error handling
2004-01-05 13:23 ` Marcelo Tosatti
@ 2004-01-05 13:56 ` Erik Andersen
0 siblings, 0 replies; 3+ messages in thread
From: Erik Andersen @ 2004-01-05 13:56 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: linux-kernel
On Mon Jan 05, 2004 at 11:23:02AM -0200, Marcelo Tosatti wrote:
> Hi Erik,
>
> For how long the behaviour has been this? It is broken, but its brokenness
> is harmless.
Not sure really. It is correct in 2.2.x, and 2.6.x, but is
wrong in all 2.4.x kernels starting with 2.4.0, but I havn't
bothered finding exactly which kernel broke things.
> Maybe some app rely on this behaviour? (I understand its wrong, but
> still).
>
> Is there any major distribution which includes this fix?
I highly doubt anything relies on this. Glibc checks for this
case before the syscall is made. I found the bug while running
some the ltp testsuite on uClibc. I have since added a check to
workaround the bug. but I would prefer to have it fixed properly
in the kernel.
> Anyway, isnt it easier to move the "if (error)" up like this?
>
> --- signal.c.orig 2004-01-05 11:13:17.000000000 -0200
> +++ signal.c 2004-01-05 11:14:09.000000000 -0200
> @@ -888,11 +888,12 @@
> break;
> }
>
> + if (error)
> + goto out;
> +
> current->blocked = new_set;
> recalc_sigpending(current);
> spin_unlock_irq(¤t->sigmask_lock);
> - if (error)
> - goto out;
> if (oset)
> goto set_old;
> } else if (oset) {
Only if you plan to ignore the locked spinlock. :-)
The 2.2.x code does this:
-current->blocked = new_set;
+if (!error)
current->blocked = new_set;
I condiered that, but I thought it better to make the code
more closely match 2.6, which looked a bit nicer.
-Erik
--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2004-01-05 13:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2003-12-31 11:45 [PATCH] fix broken 2.4.x rt_sigprocmask error handling Erik Andersen
2004-01-05 13:23 ` Marcelo Tosatti
2004-01-05 13:56 ` Erik Andersen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox