Avoid NULL pointer dereference in terminal_screen_child_exited

[rbuj]

Fixes #314

[sc0w]

I see this warning in terminal when closing mate-terminal windows (running mate-terminal in xterm)

** (mate-terminal:45473): CRITICAL **: 01:32:55.425: terminal_window_remove_screen: assertion 'gtk_widget_get_toplevel (GTK_WIDGET (screen)) == GTK_WIDGET (window)' failed

@rbuj maybe you know how to fix it?

but it isn't related to this PR, I see the same warning with mate-terminal 1.22.1

This PR fixes the issue, and I see no new warnings in travis logs.

[rbuj]

@cwendling You couldn't either do it before.

#include <stdio.h>
#include <signal.h>

int main(void)
{
  char c;

  raise(9);
  c = getchar();
  return 0;
}

The breakpoint on terminal-screen.c:1941 is not reached.

Test:

$ gcc test.c -o test
$ /usr/bin/mate-terminal -- /home/robert/Devel/test

[cwendling]

@rbuj well, it works for me (OK, I'm using 1.22.1 just now, but I could test on master if need be):

Are you sure your breakpoint works? _terminal_debug_print() is a macro that expands to a test; I wouldn't be sure on what the breakpoint is actually set.

[cwendling]

@rbuj BTW adding the getchar() call in the test program should never be reached as the program received SIGKILL already.

[rbuj]

Ok, I was wrong. I didn't debug it correctly.

[rbuj]

Please, check if this new approach is valid. The command executed by the terminal, it's stored in priv->override_command, so let's check if it's NULL. in such case we can safely close the window.

[lukefromdc]

I can confirm the terminal_window_remove_screen warning also exists with master

[raveit65]

With updated PR the terminal or any other terminal windows don't crash if closing one. But executing exit in terminal closes the terminal now. I don't use TERMINAL_EXIT_HOLD normally, but is this the right behaviour now?

[rbuj]

I'm sorry, I think it's fixed now.

[sc0w]

I still vote to keep original change, it was working like a charm :)

[rbuj]

@sc0w There are segfaults with the initial PR:

$ mate-terminal -- exit
$ mate-terminal -- /usr/bin/sh -c 'kill -9 $PPID'

and test binary closes terminal window:

gcc -x c -o test - <<EOF
#include <stdio.h>
#include <signal.h>

int main(void)
{
  char c;

  raise(9);
  c = getchar();
  return 0;
}
EOF
mate-terminal --title "C source" -- ./test

Initial PR:

$ git diff master src/terminal-screen.c

diff --git a/src/terminal-screen.c b/src/terminal-screen.c
index aba6cb1..936c81a 100644
--- a/src/terminal-screen.c
+++ b/src/terminal-screen.c
@@ -1956,6 +1956,12 @@ terminal_screen_child_exited (VteTerminal *terminal, int status)
                break;
        case TERMINAL_EXIT_HOLD:
        {
+               if (status == 9)
+               {
+                       g_signal_emit (screen, signals[CLOSE_SCREEN], 0);
+                       break;
+               }
+
                GtkWidget *info_bar;

                info_bar = terminal_info_bar_new (GTK_MESSAGE_INFO,

Test script:

#!/bin/bash
mate-terminal --title "Test 1" -- /usr/bin/bash -c 'exit'
mate-terminal --title "Test 2" -- /usr/bin/bash
mate-terminal --title "Test 3" -- /usr/bin/ls
mate-terminal --title "Test 4" -- /usr/bin/bash -c 'sleep 100 & killall -9 sleep'
mate-terminal --title "Test 5"
# SEGFAULT mate-terminal --title "Test 6" -- exit
# SEGFAULT mate-terminal --title "Test 7" -- /usr/bin/sh -c 'kill -9 $PPID'
mate-terminal --title "Test 8" & kill -9 $!
mate-terminal --title "Test 9" -- /usr/bin/bash & kill -9 $!
gcc -x c -o test - <<EOF
#include <stdio.h>
#include <signal.h>

int main(void)
{
  char c;

  raise(9);
  c = getchar();
  return 0;
}
EOF
# CLOSE ONLY LAUNCHED TERM  mate-terminal --title "Test 10" -- ./test
./test
exit
echo "hello world!"

I just created the unit tests with your comments.

[rbuj]

The 2nd approach:

[robert@localhost mate-terminal]$ git diff master src/terminal-screen.c

diff --git a/src/terminal-screen.c b/src/terminal-screen.c
index aba6cb1..cdd38bc 100644
--- a/src/terminal-screen.c
+++ b/src/terminal-screen.c
@@ -1952,10 +1952,14 @@ terminal_screen_child_exited (VteTerminal *terminal, int status)
                g_signal_emit (screen, signals[CLOSE_SCREEN], 0);
                break;
        case TERMINAL_EXIT_RESTART:
-               terminal_screen_launch_child_on_idle (screen);
+               if (priv->override_command != NULL)
+                       terminal_screen_launch_child_on_idle (screen);
                break;
        case TERMINAL_EXIT_HOLD:
        {
+               if (priv->override_command == NULL)
+                       break;
+
                GtkWidget *info_bar;

                info_bar = terminal_info_bar_new (GTK_MESSAGE_INFO,

Test:

#!/bin/bash
mate-terminal --title "Test 1" -- /usr/bin/bash -c 'exit'
# SEGFAULT ON CLOSING TEST 2 WINDOW mate-terminal --title "Test 2" -- /usr/bin/bash
mate-terminal --title "Test 3" -- /usr/bin/ls
mate-terminal --title "Test 4" -- /usr/bin/bash -c 'sleep 100 & killall -9 sleep'
mate-terminal --title "Test 5"
# SEGFAULT mate-terminal --title "Test 6" -- exit
# SEGFAULT mate-terminal --title "Test 7" -- /usr/bin/sh -c 'kill -9 $PPID'
mate-terminal --title "Test 8" & kill -9 $!
mate-terminal --title "Test 9" -- /usr/bin/bash & kill -9 $!
gcc -x c -o test - <<EOF
#include <stdio.h>
#include <signal.h>

int main(void)
{
  char c;

  raise(9);
  c = getchar();
  return 0;
}
EOF
mate-terminal --title "Test 10" -- ./test
./test
exit
echo "hello world!"

[raveit65]

With first approach and mate-terminal -- /usr/bin/sh -c 'kill -9 \$PPID'

/usr/bin/sh: line 0: kill: $PPID: arguments must be process or job IDs

And I can confirm that mate-terminal -- exit close all terminal. Are those really errors or segfaults?

The problem with second approach is that it isn't the expected behaviour when someone chose Hold terminal open when command exit (finished). User expect that terminal keeps open, or not?

[rbuj]

Typing exit on the terminal shows also the relaunch button:

--- a/src/terminal-screen.c
+++ b/src/terminal-screen.c
@@ -1956,6 +1956,9 @@ terminal_screen_child_exited (VteTerminal *terminal, int status)
                break;
        case TERMINAL_EXIT_HOLD:
        {
+               if ((status == 9) && (priv->override_command == NULL))
+                       break;
+
                GtkWidget *info_bar;

                info_bar = terminal_info_bar_new (GTK_MESSAGE_INFO,

Test

#!/bin/bash
mate-terminal --title "Test 1" -- /usr/bin/bash -c 'exit'
# SEGFAULT ON CLOSING TEST 2 WINDOW mate-terminal --title "Test 2" -- /usr/bin/bash
mate-terminal --title "Test 3" -- /usr/bin/ls
mate-terminal --title "Test 4" -- /usr/bin/bash -c 'sleep 100 & killall -9 sleep'
mate-terminal --title "Test 5"
# SEGFAULT mate-terminal --title "Test 6" -- exit
# SEGFAULT mate-terminal --title "Test 7" -- /usr/bin/sh -c 'kill -9 $PPID'
mate-terminal --title "Test 8" & kill -9 $!
mate-terminal --title "Test 9" -- /usr/bin/bash & kill -9 $!
gcc -x c -o test - <<EOF
#include <stdio.h>
#include <signal.h>

int main(void)
{
  char c;

  raise(9);
  c = getchar();
  return 0;
}
EOF
mate-terminal --title "Test 10" -- ./test
./test
exit
echo "hello world!"

[rbuj]

master: (the issue we want to solve)

# SEGFAULT ON CLOSING TEST 5 WINDOW
mate-terminal --title "Test 5"

[raveit65]

@cwendling Do you like to approve?