[FFmpeg-devel] [PATCH] checkasm: Generalize crash handling

[Martin Storsjö]

On Thu, 21 Dec 2023, Rémi Denis-Courmont wrote:

>> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
>> index 6318d9296b..668034c67f 100644
>> --- a/tests/checkasm/checkasm.c
>> +++ b/tests/checkasm/checkasm.c
>> @@ -23,8 +23,10 @@
>> #include "config.h"
>> #include "config_components.h"
>> 
>> -#ifndef _GNU_SOURCE
>> -# define _GNU_SOURCE // for syscall (performance monitoring API), strsignal()
>> +#if CONFIG_LINUX_PERF
>> +# ifndef _GNU_SOURCE
>> +#  define _GNU_SOURCE // for syscall (performance monitoring API)
>> +# endif
>> #endif
>> 
>> #include <signal.h>
>> @@ -326,6 +328,7 @@ static struct {
>>     const char *cpu_flag_name;
>>     const char *test_name;
>>     int verbose;
>> +    int catch_signals;
>> } state;
>> 
>> /* PRNG state */
>> @@ -627,6 +630,64 @@ static CheckasmFunc *get_func(CheckasmFunc **root, const char *name)
>>     return f;
>> }
>> 
>> +checkasm_context checkasm_context_buf;
>> +
>> +/* Crash handling: attempt to catch crashes and handle them
>> + * gracefully instead of just aborting abruptly. */
>> +#ifdef _WIN32
>> +#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP)
>> +static LONG NTAPI signal_handler(EXCEPTION_POINTERS *const e) {
>> +    const char *err;
>> +
>> +    if (!state.catch_signals)
>> +        return EXCEPTION_CONTINUE_SEARCH;
>> +
>> +    switch (e->ExceptionRecord->ExceptionCode) {
>> +    case EXCEPTION_FLT_DIVIDE_BY_ZERO:
>> +    case EXCEPTION_INT_DIVIDE_BY_ZERO:
>> +        err = "fatal arithmetic error";
>> +        break;
>> +    case EXCEPTION_ILLEGAL_INSTRUCTION:
>> +    case EXCEPTION_PRIV_INSTRUCTION:
>> +        err = "illegal instruction";
>> +        break;
>> +    case EXCEPTION_ACCESS_VIOLATION:
>> +    case EXCEPTION_ARRAY_BOUNDS_EXCEEDED:
>> +    case EXCEPTION_DATATYPE_MISALIGNMENT:
>> +    case EXCEPTION_STACK_OVERFLOW:
>> +        err = "segmentation fault";
>> +        break;
>> +    case EXCEPTION_IN_PAGE_ERROR:
>> +        err = "bus error";
>> +        break;
>> +    default:
>> +        return EXCEPTION_CONTINUE_SEARCH;
>> +    }
>> +    state.catch_signals = 0;
>> +    checkasm_fail_func("%s", err);
>> +    checkasm_load_context();
>> +    return EXCEPTION_CONTINUE_EXECUTION; /* never reached, but shuts up gcc */
>> +}
>> +#endif
>> +#else
>> +static void signal_handler(const int s) {
>> +    if (state.catch_signals) {
>> +        state.catch_signals = 0;
>> +        checkasm_fail_func("%s",
>> +                           s == SIGFPE ? "fatal arithmetic error" :
>> +                           s == SIGILL ? "illegal instruction" :
>> +                           s == SIGBUS ? "bus error" :
>> +                                         "segmentation fault");
>> +        checkasm_load_context();
>
> Use of format string is probably not async-signal-safe.

I'll have a look if it's possible to restructure this into doing the 
checkasm_fail_func call afterwards.

> I would also be surprised if the load_context() function was safe in 
> signal context. That's why the current code does pretty much nothing 
> other than a long jump.

The load_context() function here is either longjmp or RtlRestoreContext.

// Martin