[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] coroutine-sigaltstack: Keep SIGUSR2 handler up
From: |
Eric Blake |
Subject: |
Re: [PATCH] coroutine-sigaltstack: Keep SIGUSR2 handler up |
Date: |
Fri, 22 Jan 2021 08:55:54 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 |
On 1/22/21 4:20 AM, Max Reitz wrote:
> Modifying signal handlers is a process-global operation. When two
> threads run coroutine-sigaltstack's qemu_coroutine_new() concurrently,
> they may interfere with each other: One of them may revert the SIGUSR2
> handler back to the default between the other thread setting up
> coroutine_trampoline() as the handler and raising SIGUSR2. That SIGUSR2
> will then lead to the process exiting.
>
> Outside of coroutine-sigaltstack, qemu does not use SIGUSR2. We can
> thus keep the signal handler installed all the time.
> CoroutineThreadState.tr_handler tells coroutine_trampoline() whether its
> stack is set up so a new coroutine is to be launched (i.e., it should
> invoke sigsetjmp()), or not (i.e., the signal came from an external
> source and we should just perform the default action, which is to exit
> the process).
Not just exit the process, but exit due to a signal. It matters...
>
> Note that in user-mode emulation, the guest can register signal handlers
> for any signal but SIGSEGV and SIGBUS, so if it registers a SIGUSR2
> handler, sigaltstack coroutines will break from then on. However, we do
> not use coroutines for user-mode emulation, so that is fine.
>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> util/coroutine-sigaltstack.c | 56 +++++++++++++++++++-----------------
> 1 file changed, 29 insertions(+), 27 deletions(-)
>
> @@ -121,7 +138,17 @@ static void coroutine_trampoline(int signal)
> /* Get the thread specific information */
> coTS = coroutine_get_thread_state();
> self = coTS->tr_handler;
> +
> + if (!self) {
> + /*
> + * This SIGUSR2 came from an external source, not from
> + * qemu_coroutine_new(), so perform the default action.
> + */
> + exit(0);
> + }
...here. Silently exiting with status 0 is wrong; the correct response
is to use signal(SIGUSR2, SIG_DFL); raise(SIGUSR2) to terminate the
process in the same manner as if we had not installed our hander at all.
With that fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org