From ec507965b2dd590f5564f5d1d4dc3e47f6a528f8 Mon Sep 17 00:00:00 2001 From: m1ngsama Date: Wed, 27 May 2026 09:11:07 +0800 Subject: [PATCH] Centralize client session ownership release --- docs/CHANGELOG.md | 3 +++ docs/Development-Guide.md | 4 ++++ docs/ROADMAP.md | 6 +++--- include/client.h | 20 +++++++++----------- include/ssh_server.h | 1 + scripts/release_check.sh | 4 ++++ src/bootstrap.c | 7 +------ src/client.c | 26 ++++++++++++++++++++++++++ src/input.c | 12 +----------- 9 files changed, 52 insertions(+), 31 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index ddfa6eb..13a7045 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -48,6 +48,9 @@ - Interactive client writes now pass through a bounded per-client outbox and flush against the remote SSH window from that client's session loop. Exec sessions still write synchronously to preserve script output ordering. +- Session callback refs are now owned and released through `client.c`, so + bootstrap and interactive cleanup no longer need to manually mirror the + main-ref / callback-ref release sequence. - The two-user lifecycle test now covers opening `:inbox` before a private message arrives, matching the way users often leave an inbox page open. - Private-message inbox access now uses its own mutex instead of sharing the diff --git a/docs/Development-Guide.md b/docs/Development-Guide.md index 900531d..5541bfe 100644 --- a/docs/Development-Guide.md +++ b/docs/Development-Guide.md @@ -131,6 +131,7 @@ typedef struct client { int ref_count; // Reference counting pthread_mutex_t ref_lock; pthread_mutex_t io_lock; // Own SSH channel writes only + bool channel_callback_ref; // Ref held while callbacks are installed } client_t; ``` @@ -284,6 +285,9 @@ void room_broadcast(chat_room_t *room, const message_t *msg) { - Cross-client lookups, such as mentions and private messages, must call `client_addref()` before using a client pointer outside `g_room->lock`, then `client_release()` when done. Do not increment `ref_count` directly. +- Session callback lifetime is owned by `client.c`: `client_install_channel_callbacks()` + takes the callback ref, and `client_release_session()` removes callbacks and + releases both the callback ref and the session main ref. ### 3. Message Persistence (message.c) diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 086fb21..6bd64f9 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -38,7 +38,8 @@ Goal: make TNT predictable for operators, scripts, and package maintainers. Goal: make long-running operation boring and reliable. -- move client state to a clearer ownership model with one release path +- ✅ move session callback ownership into `client.c` and release sessions + through one `client_release_session()` path - ✅ remove cross-client SSH channel writes from mention and private-message notifications - continue replacing ad hoc cross-thread UI mutation with per-client event @@ -103,6 +104,5 @@ Goal: make regressions harder to introduce. These are the next changes that should happen before new feature work expands the surface area. -1. Finish untangling client-state ownership into a clearer release path. -2. Replace remaining release placeholders with real maintainer metadata and +1. Replace remaining release placeholders with real maintainer metadata and source-archive checksums when cutting a public package release. diff --git a/include/client.h b/include/client.h index af06833..16cc4a4 100644 --- a/include/client.h +++ b/include/client.h @@ -32,20 +32,18 @@ int client_printf(client_t *client, const char *fmt, ...); /* Reference counting for safe cross-thread cleanup. * * Lifecycle: bootstrap_run() creates the client_t with ref_count = 1 - * (the "main" ref), then adds a second ref before installing the channel - * callbacks (the "callback" ref) so the client outlives any in-flight - * eof / close / window-change callback invocation. The interactive - * session releases both refs in its cleanup path; the final release - * frees the SSH session, channel, callback struct, and the client_t. */ + * (the "main" ref). client_install_channel_callbacks() takes a second + * ref owned by client.c while channel callbacks are installed, so the + * client outlives in-flight eof / close / window-change callbacks. + * input_run_session() ends ownership with client_release_session(). */ void client_addref(client_t *client); void client_release(client_t *client); +void client_release_session(client_t *client); -/* Install the post-bootstrap channel callbacks (window-change, eof, close) - * that target this client_t. Caller MUST have already added one - * client_addref() to keep the client alive across in-flight callback - * invocations; the matching client_release() happens during cleanup in - * input_run_session(). Returns 0 on success, -1 on failure (in which - * case the caller still owns both refs and must release them). */ +/* Install the post-bootstrap channel callbacks (window-change, eof, close). + * On success this function takes the callback reference described above. + * On failure no callback reference remains and the caller still owns only + * its original main reference. */ int client_install_channel_callbacks(client_t *client); #endif /* CLIENT_H */ diff --git a/include/ssh_server.h b/include/ssh_server.h index f584567..2bcd0e7 100644 --- a/include/ssh_server.h +++ b/include/ssh_server.h @@ -74,6 +74,7 @@ typedef struct client { pthread_mutex_t ref_lock; /* Lock for ref_count */ pthread_mutex_t io_lock; /* Serialize SSH channel writes */ pthread_mutex_t whisper_lock; /* Serialize whisper inbox access */ + bool channel_callback_ref; /* client.c owns one ref while callbacks are installed */ struct ssh_channel_callbacks_struct *channel_cb; } client_t; diff --git a/scripts/release_check.sh b/scripts/release_check.sh index a80c1ad..27dedc6 100755 --- a/scripts/release_check.sh +++ b/scripts/release_check.sh @@ -109,6 +109,10 @@ step "checking client I/O ownership boundaries" fail "cross-client target-array writes must be queued through client_queue_bell" ! grep -n "pthread_mutex_lock(&.*->io_lock)" src/commands.c >/dev/null || fail "commands.c must not use SSH io_lock for in-memory command state" +! grep -n "client_addref(client)" src/bootstrap.c >/dev/null || + fail "bootstrap.c must let client_install_channel_callbacks own callback refs" +grep -q "client_release_session(client)" src/input.c || + fail "input.c must release session ownership through client_release_session" if grep -R "ssh_channel_write" src include | grep -v "^src/client.c:" >/dev/null; then fail "raw SSH channel writes must stay inside src/client.c" fi diff --git a/src/bootstrap.c b/src/bootstrap.c index b08d84c..5c91e42 100644 --- a/src/bootstrap.c +++ b/src/bootstrap.c @@ -476,17 +476,12 @@ void *bootstrap_run(void *arg) { } client->exec_command_too_long = ctx->exec_command_too_long; - /* Add a ref for the channel callbacks (eof/close/window_change) so the - * client_t outlives any in-flight callback invocation. */ - client_addref(client); - if (client_install_channel_callbacks(client) < 0) { /* Nullify session/channel ownership so client_release won't * double-free what cleanup_failed_session is about to free. */ client->session = NULL; client->channel = NULL; - client_release(client); /* drop the callback ref (2 → 1) */ - client_release(client); /* drop the main ref (1 → 0, frees client) */ + client_release(client); cleanup_failed_session(session, ctx); return NULL; } diff --git a/src/client.c b/src/client.c index d5a5c8d..aff4a9e 100644 --- a/src/client.c +++ b/src/client.c @@ -244,6 +244,25 @@ void client_release(client_t *client) { } } +void client_release_session(client_t *client) { + if (!client) return; + + if (client->channel && client->channel_cb) { + ssh_remove_channel_callbacks(client->channel, client->channel_cb); + } + if (client->channel_cb) { + free(client->channel_cb); + client->channel_cb = NULL; + } + + if (client->channel_callback_ref) { + client->channel_callback_ref = false; + client_release(client); + } + + client_release(client); +} + /* Send formatted string to client */ int client_printf(client_t *client, const char *fmt, ...) { char buffer[2048]; @@ -315,8 +334,13 @@ int client_install_channel_callbacks(client_t *client) { return -1; } + client_addref(client); + client->channel_callback_ref = true; + client->channel_cb = calloc(1, sizeof(struct ssh_channel_callbacks_struct)); if (!client->channel_cb) { + client->channel_callback_ref = false; + client_release(client); return -1; } @@ -330,6 +354,8 @@ int client_install_channel_callbacks(client_t *client) { if (ssh_set_channel_callbacks(client->channel, client->channel_cb) != SSH_OK) { free(client->channel_cb); client->channel_cb = NULL; + client->channel_callback_ref = false; + client_release(client); return -1; } diff --git a/src/input.c b/src/input.c index 8cd2573..cf54e22 100644 --- a/src/input.c +++ b/src/input.c @@ -995,17 +995,7 @@ cleanup: ratelimit_release_ip(client->client_ip); - /* Remove channel callbacks before releasing refs to prevent use-after-free - * if a callback fires between the two releases. */ - if (client->channel && client->channel_cb) { - ssh_remove_channel_callbacks(client->channel, client->channel_cb); - } - - /* Release the callback reference (paired with addref before client_install_channel_callbacks) */ - client_release(client); - - /* Release the main reference - client will be freed when all refs are gone */ - client_release(client); + client_release_session(client); /* Decrement connection count */ ratelimit_decrement_total();