[FFmpeg-devel] Fwd: [PATCH 1/2] ffmpeg: reset tty state correctly

Ganesh Ajjanagadde gajjanagadde at gmail.com
Mon Jul 27 15:01:54 CEST 2015


Apologies for this; I accidentally replied just to the responder and
not the mailing list.

---------- Forwarded message ----------
From: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
Date: Mon, Jul 27, 2015 at 8:27 AM
Subject: Re: [FFmpeg-devel] [PATCH 1/2] ffmpeg: reset tty state correctly
To: Nicolas George <george at nsup.org>


On Mon, Jul 27, 2015 at 4:39 AM, Nicolas George <george at nsup.org> wrote:
> L'octidi 8 thermidor, an CCXXIII, Ganesh Ajjanagadde a écrit :
>> tty state was not being reset upon "hard" signals (SIGSEGV etc)
>
> A good shell can do that for you.

Apparently zsh was not doing it. I tested this stuff using stty before
and after a SIGSEGV.
The problem before is that upon a hard signal, since no signal handler
was registered,
the terminal state which was changed by ffmpeg was not being restored by ffmpeg.
Of course, a shell could save the terminal state before invoking ffmpeg,
and restore it afterwards, but apparently zsh at least does not do it.
Have not tested bash.

>
>> This resets tty state in such situations, fixes Ticket2964
>
> This ticket is only about tcsetattr() not putting the tty in raw mode when
> stderr is redirected. Removing the isatty(2) test should be enough to fix
> it, but reviewing the discussions that lead to the test is necessary to know
> why it was deemed useful in the first place.

The particular commit c8a1101 does not have much about this.
Regardless, I think one should restore tty state on "hard" signals.

>
>>
>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> ---
>>  ffmpeg.c | 38 ++++++++++++++++++++++++++++----------
>>  1 file changed, 28 insertions(+), 10 deletions(-)
>>
>> diff --git a/ffmpeg.c b/ffmpeg.c
>> index 751c7d3..8b5a705 100644
>> --- a/ffmpeg.c
>> +++ b/ffmpeg.c
>> @@ -325,10 +325,29 @@ sigterm_handler(int sig)
>>      received_sigterm = sig;
>>      received_nb_signals++;
>>      term_exit_sigsafe();
>> -    if(received_nb_signals > 3) {
>> -        write(2/*STDERR_FILENO*/, "Received > 3 system signals, hard exiting\n",
>> -                           strlen("Received > 3 system signals, hard exiting\n"));
>>
>
>> +    switch (sig) {
>> +        /* 2 = STDERR_FILENO */
>> +        case SIGSEGV:
>> +            write(2, "Segmentation fault, hard exiting\n",
>> +              strlen("Segmentation fault, hard exiting\n"));
>> +            abort();
>> +        case SIGILL:
>> +            write(2, "Invalid instruction, hard exiting\n",
>> +              strlen("Invalid instruction, hard exiting\n"));
>> +            abort();
>> +        case SIGFPE:
>> +            write(2, "Arithmetic exception, hard exiting\n",
>> +              strlen("Arithmetic exception, hard exiting\n"));
>> +            abort();
>> +            break;
>> +        default:
>> +            break;
>> +    }
>
> That lakes a lot of code duplication.
>
>     char *msg = NULL;
>
>     switch (sig) {
>         case X: msg = "signal X"; break;
>         ...
>     }
>     if (msg)
>         write(2, msg, strlen(msg));
>

Changed retaining old error messages, attached updated patch.
Note abort() needs to still be called in all three cases immediately
after the write,
so I could not do exactly what you wanted.
I think "hard exiting" addition is perhaps useful, though if it is too verbose,
I will change it.

> But this change is not specified in the commit message.
>
>> +
>> +    if(received_nb_signals > 3) {
>> +        write(2, "Received > 3 system signals, hard exiting\n",
>> +          strlen("Received > 3 system signals, hard exiting\n"));
>>          exit(123);
>>      }

See new attached patch, where it is changed simply to make write calls
consistent,
i.e all of form write(2, msg, strlen(msg));

>>  }
>> @@ -370,11 +389,7 @@ void term_init(void)
>>  #if HAVE_TERMIOS_H
>>      if(!run_as_daemon){
>>          struct termios tty;
>
>> -        int istty = 1;
>> -#if HAVE_ISATTY
>> -        istty = isatty(0) && isatty(2);
>> -#endif
>> -        if (istty && tcgetattr (0, &tty) == 0) {
>> +        if (tcgetattr (0, &tty) == 0) {
>
> Why?

The tty mode needs to be changed in all cases when ffmpeg is invoked:
By the way, isatty(0) is not enough, simply because stdin could be
redirected as well.
Here is (albeit deliberate and silly) example:

ffmpeg -f lavfi -i testsrc -f null - 2>/dev/null (from ticket)

could be made into

</dev/null ffmpeg -f lavfi -i testsrc -f null - 2>/dev/null

New patch still changes tty mode in all modes.

>
>>              oldtty = tty;
>>              restore_tty = 1;
>>
>> @@ -393,8 +408,11 @@ void term_init(void)
>>      }
>>  #endif
>>
>> -    signal(SIGINT , sigterm_handler); /* Interrupt (ANSI).    */
>> -    signal(SIGTERM, sigterm_handler); /* Termination (ANSI).  */
>> +    signal(SIGINT , sigterm_handler); /* Interrupt (ANSI).              */
>> +    signal(SIGTERM, sigterm_handler); /* Termination (ANSI).            */
>
>> +    signal(SIGSEGV, sigterm_handler); /* Segmentation fault (ANSI).     */
>> +    signal(SIGILL , sigterm_handler); /* Invalid instruction (ANSI).    */
>> +    signal(SIGFPE , sigterm_handler); /* Arithmetic error (ANSI).       */
>
> NO!!!
>
> When a crash happens, we want it to happen right there, possibly leave a
> core. We do not want a signal handler to mess up the remains.

Core is still being dumped due to the abort call.
The handler is pretty minimal, just restoring tty state (necessary due
to above),
writing a message, and then calling abort.

>
>>  #ifdef SIGXCPU
>>      signal(SIGXCPU, sigterm_handler);
>>  #endif
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ffmpeg-reset-tty-state-correctly.patch
Type: text/x-patch
Size: 2652 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150727/63018eed/attachment.bin>


More information about the ffmpeg-devel mailing list