From d33bd8deff9c7964a3a9c997c5c882d09dfed87b Mon Sep 17 00:00:00 2001
From: sanhex <sanhex@gmail.com>
Date: Wed, 18 Oct 2017 12:08:32 -0700
Subject: [PATCH] GUACAMOLE-384: fixing segfault during ssh disconnect

Root Cause:
See the core dump and Valgrind report posted on Jira. guacd was reading a ssh terminal which had been freed. When a ssh connection is terminated, guac_ssh_client_free_handler() will be called from guacd_exec_proc() -> guac_client_free() with pointer client->free_handler. In guac_ssh_client_free_handler(), when ssh_client->term is freed, ssh_client->client_thread may still be using the ssh_client->term. It causes the crash reported in this bug.

The stack trace exposing the problem can be found by running guacd under Valgrind with a ssh test script. The test script repeats doing ssh login and logout for 5000 times.

Solution:
In guac_ssh_client_free_handler(), before calling guac_terminal_free(ssh_client->term), close the stdin pipe of the terminal to stop reading the pipe with guac_terminal_read_stdin() in ssh_input_thread(). So that ssh_input_thread() can be terminated in this case. Call pthread_join() to wait for ssh_client_thread() terminating before freeing the terminal.

Add a new function guac_terminal_stop() to close the pipe and set the fds to invalid (-1). Call it in guac_ssh_client_free_handler() and guac_terminal_free().

Checking the client running state in ssh_input_thread() and ssh_client_thread() to make sure they can be terminated when the client is stopped in guacd_exec_proc() by another thread.

Test:
- Confirmed ssh connection works normally.
- Observed the child process of guacd exits when ssh connection is terminated.
- Reran the ssh test script. Observed no crash.
---
 src/protocols/ssh/client.c       |  6 +++++-
 src/protocols/ssh/ssh.c          | 12 ++++++++++++
 src/terminal/terminal.c          | 16 ++++++++++++++--
 src/terminal/terminal/terminal.h |  9 +++++++++
 4 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/src/protocols/ssh/client.c b/src/protocols/ssh/client.c
index c0a26b9d..12eb1b71 100644
--- a/src/protocols/ssh/client.c
+++ b/src/protocols/ssh/client.c
@@ -70,8 +70,12 @@ int guac_ssh_client_free_handler(guac_client* client) {
 
     /* Free terminal (which may still be using term_channel) */
     if (ssh_client->term != NULL) {
-        guac_terminal_free(ssh_client->term);
+        /* Stop the terminal to unblock any pending reads/writes */
+        guac_terminal_stop(ssh_client->term);
+
+        /* Wait ssh_client_thread to finish before freeing the terminal */
         pthread_join(ssh_client->client_thread, NULL);
+        guac_terminal_free(ssh_client->term);
     }
 
     /* Free terminal channel now that the terminal is finished */
diff --git a/src/protocols/ssh/ssh.c b/src/protocols/ssh/ssh.c
index 54d13e5a..6575d9f3 100644
--- a/src/protocols/ssh/ssh.c
+++ b/src/protocols/ssh/ssh.c
@@ -162,8 +162,14 @@ void* ssh_input_thread(void* data) {
         pthread_mutex_lock(&(ssh_client->term_channel_lock));
         libssh2_channel_write(ssh_client->term_channel, buffer, bytes_read);
         pthread_mutex_unlock(&(ssh_client->term_channel_lock));
+
+        /* Make sure ssh_input_thread can be terminated anyway */
+        if (client->state == GUAC_CLIENT_STOPPING)
+            break;
     }
 
+    /* Stop the client so that ssh_client_thread can be terminated */
+    guac_client_stop(client);
     return NULL;
 
 }
@@ -340,6 +346,12 @@ void* ssh_client_thread(void* data) {
             break;
         }
 
+        /* Client is stopping, break the loop */
+        if (client->state == GUAC_CLIENT_STOPPING) {
+            pthread_mutex_unlock(&(ssh_client->term_channel_lock));
+            break;
+        }
+
         /* Send keepalive at configured interval */
         if (settings->server_alive_interval > 0) {
             timeout = 0;
diff --git a/src/terminal/terminal.c b/src/terminal/terminal.c
index 18477e34..a253572b 100644
--- a/src/terminal/terminal.c
+++ b/src/terminal/terminal.c
@@ -410,11 +410,23 @@ guac_terminal* guac_terminal_create(guac_client* client,
 
 }
 
+void guac_terminal_stop(guac_terminal* term) {
+
+    /* Close input pipe and set fds to invalid */
+    if (term->stdin_pipe_fd[1] != -1) {
+        close(term->stdin_pipe_fd[1]);
+        term->stdin_pipe_fd[1] = -1;
+    }
+    if (term->stdin_pipe_fd[0] != -1) {
+        close(term->stdin_pipe_fd[0]);
+        term->stdin_pipe_fd[0] = -1;
+    }
+}
+
 void guac_terminal_free(guac_terminal* term) {
 
     /* Close user input pipe */
-    close(term->stdin_pipe_fd[1]);
-    close(term->stdin_pipe_fd[0]);
+    guac_terminal_stop(term);
 
     /* Wait for render thread to finish */
     pthread_join(term->thread, NULL);
diff --git a/src/terminal/terminal/terminal.h b/src/terminal/terminal/terminal.h
index 351cb11c..37b07484 100644
--- a/src/terminal/terminal/terminal.h
+++ b/src/terminal/terminal/terminal.h
@@ -491,6 +491,15 @@ int guac_terminal_render_frame(guac_terminal* terminal);
  */
 int guac_terminal_read_stdin(guac_terminal* terminal, char* c, int size);
 
+/**
+ * Manually stop the terminal to forcibly unblock any pending reads/writes,
+ * e.g. forcing guac_terminal_read_stdin() to return and cease all terminal I/O.
+ *
+ * @param term
+ *     The terminal to stop.
+ */
+void guac_terminal_stop(guac_terminal* term);
+
 /**
  * Notifies the terminal that an event has occurred and the terminal should
  * flush itself when reasonable.