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

[Henrik Gramner]

On Thu, Dec 21, 2023 at 9:16 PM Rémi Denis-Courmont <remi at remlab.net> wrote:
> > +        checkasm_fail_func("%s",
> > +                           s == SIGFPE ? "fatal arithmetic error" :
> > +                           s == SIGILL ? "illegal instruction" :
> > +                           s == SIGBUS ? "bus error" :
> > +                                         "segmentation fault");
>
> The current code for the error print-out is both simpler and more versatile,
> so I don't get this.

IMO "illegal instruction" is a far better error message than "fatal
signal 4" (with an implementation-defined number which nobody knows
the meaning of without having to look it up).

> > +        /* fall back to the default signal handler */
> > +        static const struct sigaction default_sa = { .sa_handler = SIG_DFL
> > }; +        sigaction(s, &default_sa, NULL);
> > +        raise(s);
>
> Why raise here? Returning from the handler will reevaluate the same code with
> the same thread state, and trigger the default signal handler anyway (since
> you don't modify the user context).

No it wont, it'll get stuck in an infinite loop invoking the signal
handler over and over. At least on my system.

> > +    const struct sigaction sa = {
> > +        .sa_handler = signal_handler,
> > +        .sa_flags = SA_NODEFER,
>
> That does not look very sane to me. If a recursive signal occurs, processing
> it recursively is NOT a good idea. This would cause an infinite loop,
> eventually a literal stack overflow after maxing out the processor for a while.
> I'd rather let the OS kernel deal with the problem, by killing the process or
> whatever the last resort is.
>
> > +#define checkasm_save_context() setjmp(checkasm_context_buf)
> > +#define checkasm_load_context() longjmp(checkasm_context_buf, 1)
> > +#endif
>
> Been there done that and it did not end well.
> sigsetjmp() & co are necessary here.

For all intents and purposes sigjmp()/longjmp() with SA_NODEFER does
the same thing as sigsetjmp()/siglongjmp() without SA_NODEFER for this
particular use case (no infinite recursion is possible the way the
code is written). The change isn't necessary per se but it seems
reasonable and I have no objections to it.