[PATCH] sandbox: Support signal handling only when requested

[Heinrich Schuchardt]

On 22.03.21 06:21, Simon Glass wrote:
> At present if sandbox crashes it prints a message and tries to exit. But
> with the recently introduced signal handler, it often seems to get stuck
> in a loop until the stack overflows:
>
> Segmentation violation
>
> Segmentation violation
>
> Segmentation violation
>
> Segmentation violation
>
> Segmentation violation
>
> Segmentation violation
>
> Segmentation violation
> ...

Hello Simon,

do you have a reproducible example? I never have seen this.

Corrupting gd could cause an endless recursive loop, as these lines
follow printing the observed string:

        printf("pc = 0x%lx, ", pc);
        printf("pc_reloc = 0x%lx\n\n", pc - gd->reloc_off);

If we remove SA_NODEFER from the signal mask in arch/sandbox/cpu/os.c,
recursion cannot occur anymore. If a segmentation violation occurs
inside the handler it will be delegated to the default handler.

Furthermore we could consider removing the signal handler at the start
of os_signal_action().

Best regards

Heinrich

>
> The signal handler is only useful for a few tests, as I understand it.
> Make it optional.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
>  arch/sandbox/cpu/start.c         | 18 +++++++++++++++---
>  arch/sandbox/include/asm/state.h |  1 +
>  2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
> index e87365e800d..72fe293e8bc 100644
> --- a/arch/sandbox/cpu/start.c
> +++ b/arch/sandbox/cpu/start.c
> @@ -389,6 +389,16 @@ static int sandbox_cmdline_cb_select_unittests(struct sandbox_state *state,
>  }
>  SANDBOX_CMDLINE_OPT_SHORT(select_unittests, 'k', 1, "Select unit tests to run");
>
> +static int sandbox_cmdline_cb_signals(struct sandbox_state *state,
> +				      const char *arg)
> +{
> +	state->handle_signals = true;
> +
> +	return 0;
> +}
> +SANDBOX_CMDLINE_OPT_SHORT(signals, 'S', 0,
> +			  "Handle signals (such as SIGSEGV) in sandbox");
> +
>  static void setup_ram_buf(struct sandbox_state *state)
>  {
>  	/* Zero the RAM buffer if we didn't read it, to keep valgrind happy */
> @@ -472,9 +482,11 @@ int main(int argc, char *argv[])
>  	if (ret)
>  		goto err;
>
> -	ret = os_setup_signal_handlers();
> -	if (ret)
> -		goto err;
> +	if (state->handle_signals) {
> +		ret = os_setup_signal_handlers();
> +		if (ret)
> +			goto err;
> +	}
>
>  #if CONFIG_VAL(SYS_MALLOC_F_LEN)
>  	gd->malloc_base = CONFIG_MALLOC_F_ADDR;
> diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h
> index bca13069824..1c4c571e28d 100644
> --- a/arch/sandbox/include/asm/state.h
> +++ b/arch/sandbox/include/asm/state.h
> @@ -93,6 +93,7 @@ struct sandbox_state {
>  	bool ram_buf_read;		/* true if we read the RAM buffer */
>  	bool run_unittests;		/* Run unit tests */
>  	const char *select_unittests;	/* Unit test to run */
> +	bool handle_signals;		/* Handle signals within sandbox */
>
>  	/* Pointer to information for each SPI bus/cs */
>  	struct sandbox_spi_info spi[CONFIG_SANDBOX_SPI_MAX_BUS]
>