diff --git a/src/guacd/proc.c b/src/guacd/proc.c index 4040a3be2..b9eac8766 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,109 @@ 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; + + /* While users are still connected, the session is healthy + * even though no new user file descriptors arrive on + * fd_socket: ongoing session traffic of an already-connected + * session runs in separate user threads on other sockets. + * Reset the absolute idle counter so it measures true idle + * time since the last disconnect, not time since the last new + * FD. Without this, the safety net below would accumulate + * idle_cycles on a healthy single-user session and force-exit + * the active connection after GUACD_PROC_MAX_IDLE_MS. */ + if (client->connected_users > 0) + idle_cycles = 0; + } + + /* Absolute safety net: if we've been idle for too long after + * having a user AND no users remain connected, 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) after a real disconnect. + * + * The connected_users == 0 guard is required: a healthy active + * session receives no new user FDs and would otherwise accumulate + * idle_cycles and be force-killed even though it is fully alive. + * idle_cycles is now reset above while users are connected, but + * the explicit connected_users check is kept as a second line of + * defence. */ + if (had_user + && client->connected_users == 0 + && idle_cycles * GUACD_PROC_IDLE_TIMEOUT_MS + >= GUACD_PROC_MAX_IDLE_MS) { + guacd_log(GUAC_LOG_WARNING, + "Worker exceeded maximum idle time (%ds) with no " + "connected users. 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 4ee749f0a..41853f22a 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. */