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

["Rémi Denis-Courmont" <remi@remlab.net>]

Le 22 décembre 2023 00:03:59 GMT+02:00, Henrik Gramner via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> a écrit :
>On Thu, Dec 21, 2023 at 9:16 PM Rémi Denis-Courmont <remi@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).

The current code prints the number and the name.

>
>> > +        /* 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.

No, it won't since the default signal handler was restored. And it's much less confusing to debug if the signal comes from where it was actually triggered than from explicit raise call.

>
>> > +    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.
>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel@ffmpeg.org
>https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>To unsubscribe, visit link above, or email
>ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".