code review 5696043: runtime: linux signal masking

runtime: linux signal masking

Fixes issue 3101 (Linux).

[rsc, 2012-02-23 04:26:16]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2012-02-23 04:32:02]

LGTM

don't care about the return value of sigprocmask?

On Thu, Feb 23, 2012 at 3:26 PM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> runtime: linux signal masking
>
> The Linux half of issue 3101.
>
> Please review this at
http://codereview.appspot.com/**5696043/<http://codereview.appspot.com/5696043/>
>
> Affected files:
>  M src/pkg/runtime/os_linux.h
>  M src/pkg/runtime/sys_linux_386.**s
>  M src/pkg/runtime/sys_linux_**amd64.s
>  M src/pkg/runtime/sys_linux_arm.**s
>  M src/pkg/runtime/thread_linux.c
>
>
> Index: src/pkg/runtime/os_linux.h
> ==============================**==============================**=======
> --- a/src/pkg/runtime/os_linux.h
> +++ b/src/pkg/runtime/os_linux.h
> @@ -22,3 +22,14 @@
>
>  #define        NSIG    65
>  #define        SI_USER 0
> +
> +// It's hard to tease out exactly how big a Sigset is, but
> +// rt_sigprocmask crashes if we get it wrong, so if binaries
> +// are running, this is right.
> +typedef struct Sigset Sigset;
> +struct Sigset
> +{
> +       uint32 mask[2];
> +};
> +void   runtime·rtsigprocmask(int32, Sigset*, Sigset*, int32);
> +#define SIG_SETMASK 2
> Index: src/pkg/runtime/sys_linux_386.**s
> ==============================**==============================**=======
> --- a/src/pkg/runtime/sys_linux_**386.s
> +++ b/src/pkg/runtime/sys_linux_**386.s
> @@ -136,6 +136,18 @@
>        MOVL    DX, 4(DI)
>        RET
>
> +TEXT runtime·rtsigprocmask(SB),7,$0
> +       MOVL    $175, AX                // syscall entry
> +       MOVL    4(SP), BX
> +       MOVL    8(SP), CX
> +       MOVL    12(SP), DX
> +       MOVL    16(SP), SI
> +       CALL    *runtime·_vdso(SB)
> +       CMPL    AX, $0xfffff001
> +       JLS     2(PC)
> +       INT $3
> +       RET
> +
>  TEXT runtime·rt_sigaction(SB),7,$0
>        MOVL    $174, AX                // syscall - rt_sigaction
>        MOVL    4(SP), BX
> Index: src/pkg/runtime/sys_linux_**amd64.s
> ==============================**==============================**=======
> --- a/src/pkg/runtime/sys_linux_**amd64.s
> +++ b/src/pkg/runtime/sys_linux_**amd64.s
> @@ -123,6 +123,18 @@
>        ADDQ    DX, AX
>        RET
>
> +TEXT runtime·rtsigprocmask(SB),7,$**0-32
> +       MOVL    8(SP), DI
> +       MOVQ    16(SP), SI
> +       MOVQ    24(SP), DX
> +       MOVL    32(SP), R10
> +       MOVL    $14, AX                 // syscall entry
> +       SYSCALL
> +       CMPQ    AX, $0xfffffffffffff001
> +       JLS     2(PC)
> +       CALL    runtime·notok(SB)
> +       RET
> +
>  TEXT runtime·rt_sigaction(SB),7,$0-**32
>        MOVL    8(SP), DI
>        MOVQ    16(SP), SI
> Index: src/pkg/runtime/sys_linux_arm.**s
> ==============================**==============================**=======
> --- a/src/pkg/runtime/sys_linux_**arm.s
> +++ b/src/pkg/runtime/sys_linux_**arm.s
> @@ -23,6 +23,7 @@
>  #define SYS_clone (SYS_BASE + 120)
>  #define SYS_rt_sigreturn (SYS_BASE + 173)
>  #define SYS_rt_sigaction (SYS_BASE + 174)
> +#define SYS_rt_sigprocmask (SYS_BASE + 175)
>  #define SYS_sigaltstack (SYS_BASE + 186)
>  #define SYS_mmap2 (SYS_BASE + 192)
>  #define SYS_futex (SYS_BASE + 240)
> @@ -292,6 +293,15 @@
>
>        RET
>
> +TEXT runtime·rtsigprocmask(SB),7,$0
> +       MOVW    0(FP), R0
> +       MOVW    4(FP), R1
> +       MOVW    8(FP), R2
> +       MOVW    12(FP), R3
> +       MOVW    $SYS_rt_sigprocmask, R7
> +       SWI     $0
> +       RET
> +
>  TEXT runtime·rt_sigaction(SB),7,$0
>        MOVW    0(FP), R0
>        MOVW    4(FP), R1
> Index: src/pkg/runtime/thread_linux.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/thread_**linux.c
> +++ b/src/pkg/runtime/thread_**linux.c
> @@ -13,6 +13,9 @@
>  int32 runtime·close(int32);
>  int32 runtime·read(int32, void*, int32);
>
> +static Sigset sigset_all = { ~(uint32)0, ~(uint32)0 };
> +static Sigset sigset_none;
> +
>  // Linux futex.
>  //
>  //     futexsleep(uint32 *addr, uint32 val)
> @@ -135,6 +138,7 @@
>  {
>        int32 ret;
>        int32 flags;
> +       Sigset oset;
>
>        /*
>         * note: strace gets confused if we use CLONE_PTRACE here.
> @@ -152,7 +156,13 @@
>                        stk, m, g, fn, runtime·clone, m->id, m->tls[0], &m);
>        }
>
> -       if((ret = runtime·clone(flags, stk, m, g, fn)) < 0) {
> +       // Disable signals during clone, so that the new thread starts
> +       // with signals disabled.  It will enable them in minit.
> +       runtime·rtsigprocmask(SIG_**SETMASK, &sigset_all, &oset, sizeof
> oset);
> +       ret = runtime·clone(flags, stk, m, g, fn);
> +       runtime·rtsigprocmask(SIG_**SETMASK, &oset, nil, sizeof oset);
> +
> +       if(ret < 0) {
>                runtime·printf("runtime: failed to create new OS thread
> (have %d already; errno=%d)\n", runtime·mcount(), -ret);
>                runtime·throw("runtime.**newosproc");
>        }
> @@ -177,6 +187,7 @@
>        // Initialize signal handling.
>        m->gsignal = runtime·malg(32*1024);     // OS X wants >=8K, Linux
> >=2K
>        runtime·signalstack(m->**gsignal->stackguard - StackGuard,
> 32*1024);
> +       runtime·rtsigprocmask(SIG_**SETMASK, &sigset_none, nil, sizeof
> sigset_none);
>  }
>
>  void
>
>
>

[bradfitz, 2012-02-23 04:32:39]

On Thu, Feb 23, 2012 at 3:32 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> LGTM
>
> don't care about the return value of sigprocmask?
>

oh, nevermind, it only fails if `how' is wrong.

[rsc, 2012-02-23 04:34:32]

On Wed, Feb 22, 2012 at 23:32, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> don't care about the return value of sigprocmask?

i do care.
if sigprocmask retruns an error the assembly
jumps to notok, which crashes the program.

[bradfitz, 2012-02-23 04:41:03]

On Thu, Feb 23, 2012 at 3:34 PM, Russ Cox <rsc@golang.org> wrote:

> On Wed, Feb 22, 2012 at 23:32, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > don't care about the return value of sigprocmask?
>
> i do care.
> if sigprocmask retruns an error the assembly
> jumps to notok, which crashes the program.
>

Ah, I saw that for amd64 but not 386 or arm, so I assumed it was some
amd64-specific calling convention thing, just skimming those parts.

I don't see how/where arm does it, though.

[rsc, 2012-02-23 04:48:11]

On Wed, Feb 22, 2012 at 23:41, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I don't see how/where arm does it, though.

It is true - arm doesn't do it.  The other architectures
have always had that 'crash on unexpected syscall failure'
but for whatever reason arm never has.  Rather than try to
write one from scratch (I could just copy the others) I left
this one like all the others on arm.

Could be done as a separate CL but honestly I am happy
to just back slowly away from the arm.

Russ

[bradfitz, 2012-02-23 04:55:13]

On Thu, Feb 23, 2012 at 3:48 PM, Russ Cox <rsc@golang.org> wrote:

> On Wed, Feb 22, 2012 at 23:41, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > I don't see how/where arm does it, though.
>
> It is true - arm doesn't do it.  The other architectures
> have always had that 'crash on unexpected syscall failure'
> but for whatever reason arm never has.  Rather than try to
> write one from scratch (I could just copy the others) I left
> this one like all the others on arm.
>

LGTM

Could be done as a separate CL but honestly I am happy
> to just back slowly away from the arm.


Isn't the whole point of this CL to make the arm build more stable?  :-)

I though that was the only builder where we're suspecting this is biting us.

[rsc, 2012-02-23 04:56:45]

On Wed, Feb 22, 2012 at 23:55, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Isn't the whole point of this CL to make the arm build more stable?  :-)

If it gets more stable, I will have succeeded.  :-)

> I though that was the only builder where we're suspecting this is biting us.

It can theoretically happen on all systems with signals.  We've seen it on
three builders - freebsd/* and linux/arm - so I'm fixing it everywhere.

Russ

[minux1, 2012-02-23 09:58:30]

On Thu, Feb 23, 2012 at 12:48 PM, Russ Cox <rsc@golang.org> wrote:

> On Wed, Feb 22, 2012 at 23:41, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > I don't see how/where arm does it, though.
>
> It is true - arm doesn't do it.  The other architectures
> have always had that 'crash on unexpected syscall failure'
> but for whatever reason arm never has.  Rather than try to
> write one from scratch (I could just copy the others) I left
> this one like all the others on arm.
>
> Could be done as a separate CL but honestly I am happy
> to just back slowly away from the arm.

Do you mind me submitting a CL for this? Adding all the guard code will
make us discover
unexpected syscall failure earlier, which is a good thing.

[rsc, 2012-02-23 16:51:10]

> Do you mind me submitting a CL for this? Adding all the guard code will make
> us discover
> unexpected syscall failure earlier, which is a good thing.

Sure, but since you have the builder, please test it there
before sending it to us.  :-)

Russ

[rsc, 2012-02-23 19:44:00]

*** Submitted as http://code.google.com/p/go/source/detail?r=0a066e2b5836 ***

runtime: linux signal masking

Fixes issue 3101 (Linux).

R=golang-dev, bradfitz, minux.ma
CC=golang-dev
http://codereview.appspot.com/5696043