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
>