Re: Pending patches for MT-fork stuff

Submitted by Jesse Hathaway on March 3, 2021, 2:40 p.m.

Details

Message ID CANSNSoUw79L1XSMBqrg6SbNdgBOztkAp1h_KcDhfTsi3wt7NBA@mail.gmail.com
State New
Series "Re: Pending patches for MT-fork stuff"
Headers show

Commit Message

Jesse Hathaway March 3, 2021, 2:40 p.m.
On Wed, Sep 30, 2020 at 10:55 AM Rich Felker <dalias@libc.org> wrote:
> > > Here is the offending code:
> > >
> > > https://github.com/golang/go/blob/a413908dd064de6e3ea5b8d95d707a532bd3f4c8/src/runtime/signal_unix.go#L866
> > >
> > > It should be calling sigfillset() (from libc) to get the starting
> > > sigset_t rather than using its own all-one-bits initializer.
> > >
> > > There may be other places in the runtime where the same error is made.
> > > It looks like blockableSig (line 1132) is intended to do something
> > > here, but has hard-coded (somewhere else) glibc knowledge rather than
> > > probing via (libc's) sigaddset whether the signal number is valid.
> > > This might be a preferred point to fix it at.

Unfortunately I failed to submit a bug report for this issue. However, the
bug was reported by someone else and fixed with this commit:

  commit 49b017fe59bf628795f2c4fdbcb5db942e865fa9
  Author: George Tsilias <tsiliasg@gmail.com>
  Date:   Thu Jun 4 23:11:56 2020 +0300

      runtime: handle signal 34 for musl setgid

      It has been observed that setgid hangs when using cgo with musl.
      This fix ensures that signal 34 gets handled in an appropriate way,
      like signal 33 when using glibc.

      Fixes #39343

Unfortunately, it did not use sigaddset as you suggested, but instead hard-coded
the signal:

        /*  35 */ {_SigNotify, "signal 35"},
        /*  36 */ {_SigNotify, "signal 36"},
        /*  37 */ {_SigNotify, "signal 37"},

Patch hide | download patch | download mbox

diff --git a/src/runtime/sigtab_linux_generic.go
b/src/runtime/sigtab_linux_generic.go
index b26040b803..38d686544f 100644
--- a/src/runtime/sigtab_linux_generic.go
+++ b/src/runtime/sigtab_linux_generic.go
@@ -45,7 +45,7 @@  var sigtable = [...]sigTabT{
        /* 31 */ {_SigThrow, "SIGSYS: bad system call"},
        /* 32 */ {_SigSetStack + _SigUnblock, "signal 32"}, /*
SIGCANCEL; see issue 6997 */
        /* 33 */ {_SigSetStack + _SigUnblock, "signal 33"}, /*
SIGSETXID; see issues 3871, 9400, 12498 */
-       /* 34 */ {_SigNotify, "signal 34"},
+       /* 34 */ {_SigSetStack + _SigUnblock, "signal 34"}, /* musl
SIGSYNCCALL; see issue 39343 */
        /* 35 */ {_SigNotify, "signal 35"},
        /* 36 */ {_SigNotify, "signal 36"},
        /* 37 */ {_SigNotify, "signal 37"},
diff --git a/src/runtime/sigtab_linux_mipsx.go
b/src/runtime/sigtab_linux_mipsx.go
index 81dd2314c5..51ef470ce7 100644
--- a/src/runtime/sigtab_linux_mipsx.go
+++ b/src/runtime/sigtab_linux_mipsx.go
@@ -42,7 +42,7 @@  var sigtable = [...]sigTabT{
        /*  31 */ {_SigNotify, "SIGXFSZ: file size limit exceeded"},
        /*  32 */ {_SigSetStack + _SigUnblock, "signal 32"}, /*
SIGCANCEL; see issue 6997 */
        /*  33 */ {_SigSetStack + _SigUnblock, "signal 33"}, /*
SIGSETXID; see issues 3871, 9400, 12498 */
-       /*  34 */ {_SigNotify, "signal 34"},
+       /*  34 */ {_SigSetStack + _SigUnblock, "signal 34"}, /* musl
SIGSYNCCALL; see issue 39343 */

Comments

Rich Felker March 3, 2021, 2:43 p.m.
On Wed, Mar 03, 2021 at 08:40:57AM -0600, Jesse Hathaway wrote:
> On Wed, Sep 30, 2020 at 10:55 AM Rich Felker <dalias@libc.org> wrote:
> > > > Here is the offending code:
> > > >
> > > > https://github.com/golang/go/blob/a413908dd064de6e3ea5b8d95d707a532bd3f4c8/src/runtime/signal_unix.go#L866
> > > >
> > > > It should be calling sigfillset() (from libc) to get the starting
> > > > sigset_t rather than using its own all-one-bits initializer.
> > > >
> > > > There may be other places in the runtime where the same error is made.
> > > > It looks like blockableSig (line 1132) is intended to do something
> > > > here, but has hard-coded (somewhere else) glibc knowledge rather than
> > > > probing via (libc's) sigaddset whether the signal number is valid.
> > > > This might be a preferred point to fix it at.
> 
> Unfortunately I failed to submit a bug report for this issue. However, the
> bug was reported by someone else and fixed with this commit:
> 
>   commit 49b017fe59bf628795f2c4fdbcb5db942e865fa9
>   Author: George Tsilias <tsiliasg@gmail.com>
>   Date:   Thu Jun 4 23:11:56 2020 +0300
> 
>       runtime: handle signal 34 for musl setgid
> 
>       It has been observed that setgid hangs when using cgo with musl.
>       This fix ensures that signal 34 gets handled in an appropriate way,
>       like signal 33 when using glibc.
> 
>       Fixes #39343
> 
> Unfortunately, it did not use sigaddset as you suggested, but instead hard-coded
> the signal:
> 
> diff --git a/src/runtime/sigtab_linux_generic.go
> b/src/runtime/sigtab_linux_generic.go
> index b26040b803..38d686544f 100644
> --- a/src/runtime/sigtab_linux_generic.go
> +++ b/src/runtime/sigtab_linux_generic.go
> @@ -45,7 +45,7 @@ var sigtable = [...]sigTabT{
>         /* 31 */ {_SigThrow, "SIGSYS: bad system call"},
>         /* 32 */ {_SigSetStack + _SigUnblock, "signal 32"}, /*
> SIGCANCEL; see issue 6997 */
>         /* 33 */ {_SigSetStack + _SigUnblock, "signal 33"}, /*
> SIGSETXID; see issues 3871, 9400, 12498 */
> -       /* 34 */ {_SigNotify, "signal 34"},
> +       /* 34 */ {_SigSetStack + _SigUnblock, "signal 34"}, /* musl
> SIGSYNCCALL; see issue 39343 */
>         /* 35 */ {_SigNotify, "signal 35"},
>         /* 36 */ {_SigNotify, "signal 36"},
>         /* 37 */ {_SigNotify, "signal 37"},
> diff --git a/src/runtime/sigtab_linux_mipsx.go
> b/src/runtime/sigtab_linux_mipsx.go
> index 81dd2314c5..51ef470ce7 100644
> --- a/src/runtime/sigtab_linux_mipsx.go
> +++ b/src/runtime/sigtab_linux_mipsx.go
> @@ -42,7 +42,7 @@ var sigtable = [...]sigTabT{
>         /*  31 */ {_SigNotify, "SIGXFSZ: file size limit exceeded"},
>         /*  32 */ {_SigSetStack + _SigUnblock, "signal 32"}, /*
> SIGCANCEL; see issue 6997 */
>         /*  33 */ {_SigSetStack + _SigUnblock, "signal 33"}, /*
> SIGSETXID; see issues 3871, 9400, 12498 */
> -       /*  34 */ {_SigNotify, "signal 34"},
> +       /*  34 */ {_SigSetStack + _SigUnblock, "signal 34"}, /* musl
> SIGSYNCCALL; see issue 39343 */
>         /*  35 */ {_SigNotify, "signal 35"},
>         /*  36 */ {_SigNotify, "signal 36"},
>         /*  37 */ {_SigNotify, "signal 37"},

Anyone want to submit a bug report for this (the signal numbers are
not public or stable) or should I make a note to do it?

Rich
Jesse Hathaway March 3, 2021, 8:14 p.m.
On Wed, Mar 3, 2021 at 8:43 AM Rich Felker <dalias@libc.org> wrote:
> Anyone want to submit a bug report for this (the signal numbers are
> not public or stable) or should I make a note to do it?

Given my inability to talk authoritatively on the flaws in their current
implementation, my preference would be for you to submit a bug
report. Yours kindly, Jesse