On Tue, Jul 9, 2019 at 3:30 PM Rich Felker <dalias@libc.org> wrote:

> On Tue, Jul 09, 2019 at 03:01:50PM -0400, James Y Knight wrote:
> > Previously, musl would reject the call, because the main stack has
> > ss_size == 0 and ss_flags == SS_DISABLE.
> >
> > We could condition on ss_flags not containing SS_DISABLE, but instead,
> > simply remove the ss_size check, as the kernel performs the same check,
> > anyhow.
>
> Are you sure the kernel does? I'm pretty sure the reason the code is
> here is that the kernel either does not check it, or does not perform
> the check correctly in some special case. Sadly the commit messages in
> musl were not as good back at the time when the code was written.
>

As far back as the first git version (2.6.12-rc2), the kernel checks this
condition. I haven't looked back any further...

However, I note now that musl uses different values for MINSIGSTKSIZE than
the kernel does, on some architectures.

The usual value in the kernel is 2048. Only a few architectures set a
differing value:
alpha:4096
arm64:5120
ia64:131027
sparc:4096

Musl usually uses 2048 as well, but sets other values on these
architectures:
arm64:6144
powerpc:4096
powerpc64:4096
s390x:4096

(Musl doesn't support alpha, ia64, or sparc, so it's not using a lower
value than the kernel anywhere, at least).

If it's important that stacks smaller than musl's MINSIGSTKSIZE be
rejected, despite them being large enough for the kernel, then I suppose
the check should be retained. Let me know -- I'll make a new patch
implementing your suggestion if you still think that's the way to go.

Unless we have good evidence the test isn't needed, I think the right
> check is just making the error conditional on !(ss_flags &
> SS_DISABLE). POSIX specifies: "If it is set to SS_DISABLE, the stack
> is disabled and ss_sp and ss_size are ignored." Here, "set to" is
> probably something the resolution of Austin Group issue 1187 failed to
> fix; it should probably be "includes" rather than "is set to". But I'm
> not sure it makes sense to have any flags set alongside SS_DISABLE
> anyway.


>
> [[Note: I've also included the corresponding patch to libc-tests. I'm not
> > sure if this is OK, or if I should send it separately?]]
>
> I think that's fine.
>
> Rich
>
>
>
> > From fe70a508fe945cb1a44f8a6bbd87ee295637447b Mon Sep 17 00:00:00 2001
> > From: James Y Knight <jyknight@google.com>
> > Date: Tue, 9 Jul 2019 14:59:01 -0400
> > Subject: [PATCH] Fix the use of sigaltstack to return to the saved main
> stack.
> >
> > Previously, musl would reject the call, because the main stack has
> > ss_size == 0 and ss_flags == SS_DISABLE.
> >
> > We could condition on ss_flags not containing SS_DISABLE, but instead,
> > simply remove the ss_size check, as the kernel performs the same check,
> > anyhow.
> > ---
> >  src/signal/sigaltstack.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/signal/sigaltstack.c b/src/signal/sigaltstack.c
> > index cfa3f5c1..d8e8eb0b 100644
> > --- a/src/signal/sigaltstack.c
> > +++ b/src/signal/sigaltstack.c
> > @@ -4,15 +4,9 @@
> >
> >  int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
> >  {
> > -     if (ss) {
> > -             if (ss->ss_size < MINSIGSTKSZ) {
> > -                     errno = ENOMEM;
> > -                     return -1;
> > -             }
> > -             if (ss->ss_flags & SS_ONSTACK) {
> > -                     errno = EINVAL;
> > -                     return -1;
> > -             }
> > +     if (ss && (ss->ss_flags & SS_ONSTACK)) {
> > +             errno = EINVAL;
> > +             return -1;
> >       }
> >       return syscall(SYS_sigaltstack, ss, old);
> >  }
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
>
> > From 9272ed8666f930dc2f24d05b1fd248584bbf495f Mon Sep 17 00:00:00 2001
> > From: James Y Knight <jyknight@google.com>
> > Date: Tue, 9 Jul 2019 14:31:24 -0400
> > Subject: [PATCH] Verify that returning to the original stack doesn't
> return an
> >  error (e.g. ENOMEM).
> >
> > ---
> >  src/regression/sigaltstack.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/regression/sigaltstack.c b/src/regression/sigaltstack.c
> > index bfdc44a..6847454 100644
> > --- a/src/regression/sigaltstack.c
> > +++ b/src/regression/sigaltstack.c
> > @@ -30,7 +30,7 @@ static void handler(int sig)
> >
> >  int main(void)
> >  {
> > -     stack_t ss;
> > +     stack_t ss, oldss;
> >       struct sigaction sa;
> >
> >       ss.ss_sp = stack;
> > @@ -39,7 +39,7 @@ int main(void)
> >       sa.sa_handler = handler;
> >       sa.sa_flags = SA_ONSTACK;
> >
> > -     T(sigaltstack(&ss, 0));
> > +     T(sigaltstack(&ss, &oldss));
> >       T(sigfillset(&sa.sa_mask));
> >       T(sigaction(SIGUSR1, &sa, 0));
> >       T(raise(SIGUSR1));
> > @@ -56,7 +56,7 @@ int main(void)
> >               t_error("sigaltstack with bad ss_flags should have failed
> with EINVAL, "
> >                       "got %s\n", strerror(errno));
> >       errno = 0;
> > -     T(sigaltstack(0, 0));
> > +     T(sigaltstack(oldss, 0));
>

(And sorry -- just noticed that I forgot to commit the last fix in this
repo, before generating this patch. That should've said "&oldss".)

>
> >       return t_status;
> >  }
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
>
>