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.
This commit is contained in:
sanhex 2017-10-18 12:08:32 -07:00
parent 3c7a09f52b
commit d33bd8deff
4 changed files with 40 additions and 3 deletions

View File

@ -70,8 +70,12 @@ int guac_ssh_client_free_handler(guac_client* client) {
/* Free terminal (which may still be using term_channel) */ /* Free terminal (which may still be using term_channel) */
if (ssh_client->term != NULL) { 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); pthread_join(ssh_client->client_thread, NULL);
guac_terminal_free(ssh_client->term);
} }
/* Free terminal channel now that the terminal is finished */ /* Free terminal channel now that the terminal is finished */

View File

@ -162,8 +162,14 @@ void* ssh_input_thread(void* data) {
pthread_mutex_lock(&(ssh_client->term_channel_lock)); pthread_mutex_lock(&(ssh_client->term_channel_lock));
libssh2_channel_write(ssh_client->term_channel, buffer, bytes_read); libssh2_channel_write(ssh_client->term_channel, buffer, bytes_read);
pthread_mutex_unlock(&(ssh_client->term_channel_lock)); 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; return NULL;
} }
@ -340,6 +346,12 @@ void* ssh_client_thread(void* data) {
break; 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 */ /* Send keepalive at configured interval */
if (settings->server_alive_interval > 0) { if (settings->server_alive_interval > 0) {
timeout = 0; timeout = 0;

View File

@ -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) { void guac_terminal_free(guac_terminal* term) {
/* Close user input pipe */ /* Close user input pipe */
close(term->stdin_pipe_fd[1]); guac_terminal_stop(term);
close(term->stdin_pipe_fd[0]);
/* Wait for render thread to finish */ /* Wait for render thread to finish */
pthread_join(term->thread, NULL); pthread_join(term->thread, NULL);

View File

@ -491,6 +491,15 @@ int guac_terminal_render_frame(guac_terminal* terminal);
*/ */
int guac_terminal_read_stdin(guac_terminal* terminal, char* c, int size); 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 * Notifies the terminal that an event has occurred and the terminal should
* flush itself when reasonable. * flush itself when reasonable.