Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

[Rich Felker <dalias@libc.org>]

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.

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));
>  
>  	return t_status;
>  }
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
>