From a5a62f057e3bf5a1c8b09ccef6986f5b0453b4df Mon Sep 17 00:00:00 2001 From: m1ngsama Date: Thu, 22 Jan 2026 14:06:15 +0800 Subject: [PATCH] fix(security): implement concurrency safety improvements - Enhance room_broadcast() reference counting: * Check client state (connected, show_help, command_output) before rendering * Perform state check while holding client ref_lock * Prevents rendering to disconnected/invalid clients * Ensures safe cleanup when ref count reaches zero - Fix tui_render_screen() message array TOCTOU: * Acquire all data (online count, message count, messages) in single lock * Create snapshot of messages to display * Calculate message range while holding lock * Render from snapshot without holding lock * Prevents inconsistencies from concurrent message additions * Eliminates race between two separate lock acquisitions - Fix handle_key() scroll position TOCTOU: * Get message count atomically when calculating scroll bounds * Calculate max_scroll properly accounting for message height * Apply consistent bounds checking for 'j' (down) and 'G' (bottom) * Prevents out-of-bounds access from concurrent message changes These changes address: - Race condition in broadcast rendering to disconnecting clients - TOCTOU between message count read and message access - Scroll position bounds check race conditions Prevents: - Use-after-free in client cleanup - Array out-of-bounds access - Inconsistent UI rendering - Crashes from concurrent message list modifications Improves thread safety without introducing deadlocks by: - Using snapshot approach to avoid long lock holds - Acquiring data in consistent lock order - Minimizing critical sections --- src/chat_room.c | 14 +++++++-- src/ssh_server.c | 18 ++++++++++-- src/tui.c | 75 +++++++++++++++++++++++++++++------------------- 3 files changed, 72 insertions(+), 35 deletions(-) diff --git a/src/chat_room.c b/src/chat_room.c index 65b4c5f..e2815f0 100644 --- a/src/chat_room.c +++ b/src/chat_room.c @@ -98,8 +98,18 @@ void room_broadcast(chat_room_t *room, const message_t *msg) { /* Render to each client (outside of lock) */ for (int i = 0; i < count; i++) { client_t *client = clients_copy[i]; - if (client->connected && !client->show_help && - client->command_output[0] == '\0') { + + /* Check client state before rendering (while holding ref) */ + bool should_render = false; + pthread_mutex_lock(&client->ref_lock); + if (client->ref_count > 0) { + should_render = client->connected && + !client->show_help && + client->command_output[0] == '\0'; + } + pthread_mutex_unlock(&client->ref_lock); + + if (should_render) { tui_render_screen(client); } diff --git a/src/ssh_server.c b/src/ssh_server.c index 4023611..3b7c6fc 100644 --- a/src/ssh_server.c +++ b/src/ssh_server.c @@ -342,7 +342,13 @@ static bool handle_key(client_t *client, unsigned char key, char *input) { tui_render_screen(client); return true; /* Key consumed - prevents double colon */ } else if (key == 'j') { - int max_scroll = room_get_message_count(g_room) - 1; + /* Get message count atomically to prevent TOCTOU */ + int max_scroll = room_get_message_count(g_room); + int msg_height = client->height - 3; + if (msg_height < 1) msg_height = 1; + max_scroll = max_scroll - msg_height; + if (max_scroll < 0) max_scroll = 0; + if (client->scroll_pos < max_scroll) { client->scroll_pos++; tui_render_screen(client); @@ -357,8 +363,14 @@ static bool handle_key(client_t *client, unsigned char key, char *input) { tui_render_screen(client); return true; /* Key consumed */ } else if (key == 'G') { - client->scroll_pos = room_get_message_count(g_room) - 1; - if (client->scroll_pos < 0) client->scroll_pos = 0; + /* Get message count atomically to prevent TOCTOU */ + int max_scroll = room_get_message_count(g_room); + int msg_height = client->height - 3; + if (msg_height < 1) msg_height = 1; + max_scroll = max_scroll - msg_height; + if (max_scroll < 0) max_scroll = 0; + + client->scroll_pos = max_scroll; tui_render_screen(client); return true; /* Key consumed */ } else if (key == '?') { diff --git a/src/tui.c b/src/tui.c index 7df9317..f5a7b77 100644 --- a/src/tui.c +++ b/src/tui.c @@ -19,11 +19,48 @@ void tui_render_screen(client_t *client) { char buffer[8192]; int pos = 0; + /* Acquire all data in one lock to prevent TOCTOU */ pthread_rwlock_rdlock(&g_room->lock); int online = g_room->client_count; int msg_count = g_room->message_count; + + /* Calculate which messages to show */ + int msg_height = client->height - 3; + if (msg_height < 1) msg_height = 1; + + int start = 0; + if (client->mode == MODE_NORMAL) { + start = client->scroll_pos; + if (start > msg_count - msg_height) { + start = msg_count - msg_height; + } + if (start < 0) start = 0; + } else { + /* INSERT mode: show latest */ + if (msg_count > msg_height) { + start = msg_count - msg_height; + } + } + + int end = start + msg_height; + if (end > msg_count) end = msg_count; + + /* Create snapshot of messages to display */ + message_t *msg_snapshot = NULL; + int snapshot_count = end - start; + + if (snapshot_count > 0) { + msg_snapshot = calloc(snapshot_count, sizeof(message_t)); + if (msg_snapshot) { + memcpy(msg_snapshot, &g_room->messages[start], + snapshot_count * sizeof(message_t)); + } + } + pthread_rwlock_unlock(&g_room->lock); + /* Now render using snapshot (no lock held) */ + /* Clear and move to top */ pos += snprintf(buffer + pos, sizeof(buffer) - pos, ANSI_CLEAR ANSI_HOME); @@ -47,40 +84,18 @@ void tui_render_screen(client_t *client) { } pos += snprintf(buffer + pos, sizeof(buffer) - pos, ANSI_RESET "\r\n"); - /* Messages area */ - int msg_height = client->height - 3; - if (msg_height < 1) msg_height = 1; - - /* Calculate which messages to show */ - int start = 0; - if (client->mode == MODE_NORMAL) { - start = client->scroll_pos; - if (start > msg_count - msg_height) { - start = msg_count - msg_height; - } - if (start < 0) start = 0; - } else { - /* INSERT mode: show latest */ - if (msg_count > msg_height) { - start = msg_count - msg_height; + /* Render messages from snapshot */ + if (msg_snapshot) { + for (int i = 0; i < snapshot_count; i++) { + char msg_line[1024]; + message_format(&msg_snapshot[i], msg_line, sizeof(msg_line), client->width); + pos += snprintf(buffer + pos, sizeof(buffer) - pos, "%s\r\n", msg_line); } + free(msg_snapshot); } - pthread_rwlock_rdlock(&g_room->lock); - - int end = start + msg_height; - if (end > msg_count) end = msg_count; - - for (int i = start; i < end; i++) { - char msg_line[1024]; - message_format(&g_room->messages[i], msg_line, sizeof(msg_line), client->width); - pos += snprintf(buffer + pos, sizeof(buffer) - pos, "%s\r\n", msg_line); - } - - pthread_rwlock_unlock(&g_room->lock); - /* Fill empty lines */ - for (int i = end - start; i < msg_height; i++) { + for (int i = snapshot_count; i < msg_height; i++) { buffer[pos++] = '\r'; buffer[pos++] = '\n'; }