2004-01-05 13:23:46

by Marcelo Tosatti

Subject: Re: [PATCH] fix broken 2.4.x rt_sigprocmask error handling

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(&current->blocked, &old_set, &new_set);
> break;
> case SIG_UNBLOCK:
> - signandsets(&new_set, &old_set, &new_set);
> + signandsets(&current->blocked, &old_set, &new_set);
> break;
> case SIG_SETMASK:
> + current->blocked = new_set;
> break;
> }
>
> - current->blocked = new_set;
> recalc_sigpending(current);
> spin_unlock_irq(&current->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(&current->sigmask_lock);
- if (error)
- goto out;
if (oset)
goto set_old;
} else if (oset) {

2004-01-05 13:56:37

by Erik Andersen

Subject: Re: [PATCH] fix broken 2.4.x rt_sigprocmask error handling

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(&current->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--