You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@guacamole.apache.org by mj...@apache.org on 2018/01/23 22:46:09 UTC
[1/2] guacamole-server git commit: GUACAMOLE-384: fixing segfault
during ssh disconnect
Repository: guacamole-server
Updated Branches:
refs/heads/master 8c844e6ea -> db85163e2
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.
Project: http://git-wip-us.apache.org/repos/asf/guacamole-server/repo
Commit: http://git-wip-us.apache.org/repos/asf/guacamole-server/commit/d33bd8de
Tree: http://git-wip-us.apache.org/repos/asf/guacamole-server/tree/d33bd8de
Diff: http://git-wip-us.apache.org/repos/asf/guacamole-server/diff/d33bd8de
Branch: refs/heads/master
Commit: d33bd8deff9c7964a3a9c997c5c882d09dfed87b
Parents: 3c7a09f
Author: sanhex <sa...@gmail.com>
Authored: Wed Oct 18 12:08:32 2017 -0700
Committer: sanhex <sa...@gmail.com>
Committed: Sun Oct 29 18:11:15 2017 -0700
----------------------------------------------------------------------
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(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/guacamole-server/blob/d33bd8de/src/protocols/ssh/client.c
----------------------------------------------------------------------
diff --git a/src/protocols/ssh/client.c b/src/protocols/ssh/client.c
index c0a26b9..12eb1b7 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 */
http://git-wip-us.apache.org/repos/asf/guacamole-server/blob/d33bd8de/src/protocols/ssh/ssh.c
----------------------------------------------------------------------
diff --git a/src/protocols/ssh/ssh.c b/src/protocols/ssh/ssh.c
index 54d13e5..6575d9f 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;
http://git-wip-us.apache.org/repos/asf/guacamole-server/blob/d33bd8de/src/terminal/terminal.c
----------------------------------------------------------------------
diff --git a/src/terminal/terminal.c b/src/terminal/terminal.c
index 18477e3..a253572 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);
http://git-wip-us.apache.org/repos/asf/guacamole-server/blob/d33bd8de/src/terminal/terminal/terminal.h
----------------------------------------------------------------------
diff --git a/src/terminal/terminal/terminal.h b/src/terminal/terminal/terminal.h
index 351cb11..37b0748 100644
--- a/src/terminal/terminal/terminal.h
+++ b/src/terminal/terminal/terminal.h
@@ -492,6 +492,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.
*
[2/2] guacamole-server git commit: GUACAMOLE-384: Merge fix for
potential segfault if SSH connection is closed while output is still being
written to the terminal.
Posted by mj...@apache.org.
GUACAMOLE-384: Merge fix for potential segfault if SSH connection is closed while output is still being written to the terminal.
Project: http://git-wip-us.apache.org/repos/asf/guacamole-server/repo
Commit: http://git-wip-us.apache.org/repos/asf/guacamole-server/commit/db85163e
Tree: http://git-wip-us.apache.org/repos/asf/guacamole-server/tree/db85163e
Diff: http://git-wip-us.apache.org/repos/asf/guacamole-server/diff/db85163e
Branch: refs/heads/master
Commit: db85163e20a253dae1daffd0dd6b7ab6beb1ed6d
Parents: 8c844e6 d33bd8d
Author: Michael Jumper <mj...@apache.org>
Authored: Tue Jan 23 14:44:47 2018 -0800
Committer: Michael Jumper <mj...@apache.org>
Committed: Tue Jan 23 14:44:47 2018 -0800
----------------------------------------------------------------------
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(-)
----------------------------------------------------------------------