From 7ca5fa3270fb7b008cc3d35ab0e16701c27437ab Mon Sep 17 00:00:00 2001 From: ESCRA GmbH Date: Wed, 15 Apr 2026 01:27:57 +0200 Subject: [PATCH] GUACAMOLE-2265: Fix worker process leak after client disconnect. Replace the blocking recvmsg loop in the worker with poll-based idle detection so the worker can self-terminate when the client has stopped. Add a 60-second reconnect grace period before exiting on zero connected users, allowing browser refreshes and tab switches to rejoin without losing the remote session. Add a 120-second absolute safety-net timeout for edge cases where the normal exit path is blocked. Also add error handling for pthread_create failure in guacd_proc_add_user and comprehensive resource cleanup in the RDP connection fail path. --- src/guacd/proc.c | 92 ++++++++++++++++++++++++++++++++++++++++- src/guacd/proc.h | 24 +++++++++++ src/protocols/rdp/rdp.c | 30 ++++++++++++++ 3 files changed, 144 insertions(+), 2 deletions(-) diff --git a/src/guacd/proc.c b/src/guacd/proc.c index 4040a3be23..059405b156 100644 --- a/src/guacd/proc.c +++ b/src/guacd/proc.c @@ -34,6 +34,7 @@ #include #include +#include #include #include #include @@ -138,7 +139,17 @@ static void guacd_proc_add_user(guacd_proc* proc, int fd, int owner) { /* Start user thread */ pthread_t user_thread; - pthread_create(&user_thread, NULL, guacd_user_thread, params); + int err = pthread_create(&user_thread, NULL, guacd_user_thread, params); + if (err) { + guacd_log(GUAC_LOG_ERROR, + "Unable to create user thread (error %d: %s). " + "Closing connection and stopping worker to prevent leak.", + err, strerror(err)); + close(fd); + guac_mem_free(params); + guacd_proc_stop(proc); + return; + } pthread_detach(user_thread); } @@ -365,11 +376,88 @@ static void guacd_exec_proc(guacd_proc* proc, const char* protocol) { sigaction(SIGINT, &signal_stop_action, NULL); sigaction(SIGTERM, &signal_stop_action, NULL); + /* Track whether we have ever had a user, so we can detect the case + * where all users have disconnected and no new ones will arrive. + * Also track idle time so we can forcibly exit after an absolute max. */ + int had_user = 0; + int idle_cycles = 0; + int no_user_cycles = 0; + /* Add each received file descriptor as a new user */ int received_fd; - while ((received_fd = guacd_recv_fd(proc->fd_socket)) != -1) { + for (;;) { + + /* Use poll() with a timeout instead of blocking indefinitely in + * recvmsg(). This allows the worker to self-terminate if the client + * has stopped and no user thread triggered guacd_proc_stop(). */ + struct pollfd pfd = { + .fd = proc->fd_socket, + .events = POLLIN + }; + + int poll_result = poll(&pfd, 1, GUACD_PROC_IDLE_TIMEOUT_MS); + + if (poll_result < 0) { + if (errno == EINTR) + continue; + break; + } + + if (poll_result == 0) { + idle_cycles++; + + /* If we had a user and client explicitly stopped, exit */ + if (had_user && client->state != GUAC_CLIENT_RUNNING) { + guacd_log(GUAC_LOG_INFO, + "Worker idle timeout: client no longer running " + "(state=%d), exiting.", client->state); + break; + } + + /* If no users remain, allow a grace period for reconnects + * (browser refresh, tab re-open) before giving up */ + if (had_user && client->connected_users == 0) { + no_user_cycles++; + if (no_user_cycles * GUACD_PROC_IDLE_TIMEOUT_MS + >= GUACD_PROC_RECONNECT_GRACE_MS) { + guacd_log(GUAC_LOG_INFO, + "Worker idle timeout: no connected users " + "for %ds, exiting.", + GUACD_PROC_RECONNECT_GRACE_MS / 1000); + break; + } + } + else { + no_user_cycles = 0; + } + + /* Absolute safety net: if we've been idle for too long after + * having a user, force exit regardless of client state. This + * catches cases where the client free handler is blocked + * (e.g. guac_argv_await in FreeRDP authenticate callback). */ + if (had_user + && idle_cycles * GUACD_PROC_IDLE_TIMEOUT_MS + >= GUACD_PROC_MAX_IDLE_MS) { + guacd_log(GUAC_LOG_WARNING, + "Worker exceeded maximum idle time (%ds) after user " + "disconnected. Force exiting.", + GUACD_PROC_MAX_IDLE_MS / 1000); + break; + } + + continue; + } + + if (pfd.revents & (POLLERR | POLLHUP | POLLNVAL)) + break; + + received_fd = guacd_recv_fd(proc->fd_socket); + if (received_fd == -1) + break; guacd_proc_add_user(proc, received_fd, owner); + had_user = 1; + idle_cycles = 0; /* Future file descriptors are not owners */ owner = 0; diff --git a/src/guacd/proc.h b/src/guacd/proc.h index 4ee749f0ab..41853f22a8 100644 --- a/src/guacd/proc.h +++ b/src/guacd/proc.h @@ -46,6 +46,30 @@ */ #define GUACD_CLIENT_FREE_TIMEOUT 5 +/** + * The number of milliseconds the worker process will wait in poll() + * before checking whether the client is still alive. If no new user FDs + * arrive within this interval, the worker checks client state and user + * count to decide whether to continue waiting or exit. + */ +#define GUACD_PROC_IDLE_TIMEOUT_MS 5000 + +/** + * Grace period (in milliseconds) after the last user disconnects before + * the worker process exits. This allows a brief window during which a + * reconnecting user (browser refresh, tab re-open) can rejoin without + * losing the underlying remote desktop session. + */ +#define GUACD_PROC_RECONNECT_GRACE_MS 60000 + +/** + * Absolute maximum idle time (in milliseconds) after which a worker + * that once had a user will forcibly exit, regardless of client state. This + * catches edge cases where the normal exit path is blocked (e.g. FreeRDP's + * authenticate callback stuck in guac_argv_await). + */ +#define GUACD_PROC_MAX_IDLE_MS 120000 + /** * Process information of the internal remote desktop client. */ diff --git a/src/protocols/rdp/rdp.c b/src/protocols/rdp/rdp.c index 3342079d04..87904aa3f4 100644 --- a/src/protocols/rdp/rdp.c +++ b/src/protocols/rdp/rdp.c @@ -725,6 +725,36 @@ static int guac_rdp_handle_connection(guac_client* client) { return 0; fail: + + /* Clean up resources allocated before the failure point to prevent leaking + * the display, FreeRDP instance, keyboard, and SVC list on every failed + * connection attempt */ + + if (rdp_client->keyboard != NULL) { + guac_rdp_keyboard_free(rdp_client->keyboard); + rdp_client->keyboard = NULL; + } + + if (rdp_client->available_svc != NULL) { + guac_common_list_free(rdp_client->available_svc, NULL); + rdp_client->available_svc = NULL; + } + + if (rdp_inst != NULL) { + + if (GUAC_RDP_CONTEXT(rdp_inst) != NULL) { + gdi_free(rdp_inst); + freerdp_context_free(rdp_inst); + } + + freerdp_free(rdp_inst); + } + + if (rdp_client->display != NULL) { + guac_display_free(rdp_client->display); + rdp_client->display = NULL; + } + guac_rwlock_release_lock(&(rdp_client->lock)); return 1;