[PATCH] Fix sigaltstack corruption among cloned threads

[PATCH] Fix sigaltstack corruption among cloned threads

[GOTO Masanori]

This patch fixes alternate signal stack corruption among cloned threads with
CLONE_SIGHAND (and CLONE_VM) for linux-2.6.16-rc6.

The value of alternate signal stack is currently inherited after a call of
clone(... CLONE_SIGHAND | CLONE_VM).  But if sigaltstack is set by a parent
thread, and then if multiple cloned child threads (+ parent threads) call
signal handler at the same time, some threads may be conflicted - because they
share to use the same alternative signal stack region.  Finally they get
sigsegv.  It's an undesirable race condition.  Note that child threads created
from NPTL pthread_create() also hit this conflict when the parent thread uses
sigaltstack, without my patch.

To fix this problem, this patch clears the child threads' sigaltstack
information like exec().  This behavior follows the SUSv3 specification.  In
SUSv3, pthread_create() says "The alternate stack shall not be inherited (when
new threads are initialized)".  It means that sigaltstack should be cleared
when sigaltstack memory space is shared by cloned threads with CLONE_SIGHAND.

Note that I chose "if (clone_flags & CLONE_SIGHAND)" line because:
  - If clone_flags line is not existed, fork() does not inherit sigaltstack.
  - CLONE_VM is another choice, but vfork() does not inherit sigaltstack.
  - CLONE_SIGHAND implies CLONE_VM, and it looks suitable.
  - CLONE_THREAD is another candidate, and includes CLONE_SIGHAND + CLONE_VM,
    but this flag has a bit different semantics.
I decided to use CLONE_SIGHAND.

-- gotom

Signed-Off-By: GOTO Masanori <gotom@sanori.org>

--- linux-2.6.16-rc6.gotom/kernel/fork.c	2006-03-13 14:45:50.686049000 +0900
+++ linux-2.6.16-rc6/kernel/fork.c	2006-03-13 14:47:24.162839240 +0900
@@ -1062,6 +1062,13 @@ static task_t *copy_process(unsigned lon
 	p->clear_child_tid = (clone_flags & CLONE_CHILD_CLEARTID) ? child_tidptr: NULL;
 
 	/*
+	 * sigaltstack should be cleared when CLONE_SIGHAND (and CLONE_VM) is
+	 * specified.
+	 */
+	if (clone_flags & CLONE_SIGHAND)
+		p->sas_ss_sp = p->sas_ss_size = 0;
+
+	/*
 	 * Syscall tracing should be turned off in the child regardless
 	 * of CLONE_PTRACE.
 	 */

Re: [PATCH] Fix sigaltstack corruption among cloned threads

[Ulrich Drepper]

On 3/13/06, GOTO Masanori <gotom@sanori.org> wrote:
> +        * sigaltstack should be cleared when CLONE_SIGHAND (and CLONE_VM) is
> +        * specified.
> +        */
> +       if (clone_flags & CLONE_SIGHAND)
> +               p->sas_ss_sp = p->sas_ss_size = 0;

I agree in general, but why base it on CLONE_SIGHAND? The problem
results from using the same address space.  So it should be

  if (clone_flags & CLONE_VM)

The fact that both these flags are used at the same time in all cases
today shouldn't hide the real reason for this requirement which is
sharing the address space.

Re: [PATCH] Fix sigaltstack corruption among cloned threads

[Jakub Jelinek]

On Mon, Mar 13, 2006 at 07:16:17AM -0800, Ulrich Drepper wrote:
> On 3/13/06, GOTO Masanori <gotom@sanori.org> wrote:
> > +        * sigaltstack should be cleared when CLONE_SIGHAND (and CLONE_VM) is
> > +        * specified.
> > +        */
> > +       if (clone_flags & CLONE_SIGHAND)
> > +               p->sas_ss_sp = p->sas_ss_size = 0;
> 
> I agree in general, but why base it on CLONE_SIGHAND? The problem
> results from using the same address space.  So it should be
> 
>   if (clone_flags & CLONE_VM)
> 
> The fact that both these flags are used at the same time in all cases
> today shouldn't hide the real reason for this requirement which is
> sharing the address space.

Because vfork also sets CLONE_VM and vfork isn't supposed to reset
alternate stack setting.  For vfork that's not a problem, as the parent task
will not continue until the vfork child execve's.  So, if you want to use
CLONE_VM bit, you'd need to use
	if ((clone_flags & (CLONE_VM | CLONE_VFORK)) == CLONE_VM)
		p->sas_ss_sp = p->sas_ss_size = 0;

	Jakub

Re: [PATCH] Fix sigaltstack corruption among cloned threads

[Ulrich Drepper]

On 3/13/06, Jakub Jelinek <jakub@redhat.com> wrote:
> Because vfork also sets CLONE_VM and vfork isn't supposed to reset
> alternate stack setting.

I'd say this is rather an oversight than a real requirement (we fixed
quite a few such problems in the POSIX spec of vfork) but it should be
as safe as any other solution.  So, I'm OK with the extended CLONE_VM
test.