Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/118#discussion_r146071398
--- Diff: src/protocols/ssh/client.c ---
@@ -70,8 +70,14 @@ 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);
+ /* Close user input pipe to stop reading in ssh_input_thread */
+ close(ssh_client->term->stdin_pipe_fd[1]);
+ close(ssh_client->term->stdin_pipe_fd[0]);
+ ssh_client->term->stdin_pipe_fd[1] = -1;
+ ssh_client->term->stdin_pipe_fd[0] = -1;
+
--- End diff --
No, that would still expose internals, this time within the name of the
function itself.
The new ordering with waiting on the thread prior to freeing the terminal
is correct, but it seems to me that it's the client thread's dependence on the
terminal that's the issue here. The input thread shouldn't be depending on
closure of the terminal at all. The various client-specific threads should take
into account the client's own stop signal, rather than relying on
`guac_terminal` to expand scope and shoulder that burden.
---
