Apologies for this; I accidentally replied just to the responder and
not the mailing list.
---------- Forwarded message ----------
From: Ganesh Ajjanagadde <gajjanaga...@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 <geo...@nsup.org>
On Mon, Jul 27, 2015 at 4:39 AM, Nicolas George <geo...@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 <gajjanaga...@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
From b8449553fa1c09128b08ab3b36a8c757436ccfcb Mon Sep 17 00:00:00 2001
From: Ganesh Ajjanagadde <gajjanaga...@gmail.com>
Date: Sun, 26 Jul 2015 23:28:21 -0400
Subject: [PATCH 1/2] ffmpeg: reset tty state correctly
tty state was not being reset upon "hard" signals (SIGSEGV etc)
This resets tty state in such situations, fixes Ticket2964
Signed-off-by: Ganesh Ajjanagadde <gajjanaga...@gmail.com>
---
ffmpeg.c | 38 ++++++++++++++++++++++++++++----------
1 file changed, 28 insertions(+), 10 deletions(-)
diff --git a/ffmpeg.c b/ffmpeg.c
index 751c7d3..333ec08 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"));
+ char *msg = NULL;
+
+ switch (sig) {
+ /* 2 = STDERR_FILENO */
+ case SIGSEGV:
+ msg = "Segmentation fault, hard exiting\n";
+ write(2, msg, strlen(msg));
+ abort();
+ case SIGILL:
+ msg = "Invalid instruction, hard exiting\n";
+ write(2, msg, strlen(msg));
+ abort();
+ case SIGFPE:
+ msg = "Arithmetic exception, hard exiting\n";
+ write(2, msg, strlen(msg));
+ abort();
+ default:
+ break;
+ }
+ if(received_nb_signals > 3) {
+ msg = "Received > 3 system signals, hard exiting\n";
+ write(2, msg, strlen(msg));
exit(123);
}
}
@@ -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) {
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). */
#ifdef SIGXCPU
signal(SIGXCPU, sigterm_handler);
#endif
--
2.4.6
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
