mirror of
https://github.com/m1ngsama/TNT.git
synced 2026-02-08 08:54:05 +00:00
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
This commit is contained in:
parent
abe477f713
commit
a5a62f057e
3 changed files with 72 additions and 35 deletions
|
|
@ -98,8 +98,18 @@ void room_broadcast(chat_room_t *room, const message_t *msg) {
|
||||||
/* Render to each client (outside of lock) */
|
/* Render to each client (outside of lock) */
|
||||||
for (int i = 0; i < count; i++) {
|
for (int i = 0; i < count; i++) {
|
||||||
client_t *client = clients_copy[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);
|
tui_render_screen(client);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -342,7 +342,13 @@ static bool handle_key(client_t *client, unsigned char key, char *input) {
|
||||||
tui_render_screen(client);
|
tui_render_screen(client);
|
||||||
return true; /* Key consumed - prevents double colon */
|
return true; /* Key consumed - prevents double colon */
|
||||||
} else if (key == 'j') {
|
} 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) {
|
if (client->scroll_pos < max_scroll) {
|
||||||
client->scroll_pos++;
|
client->scroll_pos++;
|
||||||
tui_render_screen(client);
|
tui_render_screen(client);
|
||||||
|
|
@ -357,8 +363,14 @@ static bool handle_key(client_t *client, unsigned char key, char *input) {
|
||||||
tui_render_screen(client);
|
tui_render_screen(client);
|
||||||
return true; /* Key consumed */
|
return true; /* Key consumed */
|
||||||
} else if (key == 'G') {
|
} else if (key == 'G') {
|
||||||
client->scroll_pos = room_get_message_count(g_room) - 1;
|
/* Get message count atomically to prevent TOCTOU */
|
||||||
if (client->scroll_pos < 0) client->scroll_pos = 0;
|
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);
|
tui_render_screen(client);
|
||||||
return true; /* Key consumed */
|
return true; /* Key consumed */
|
||||||
} else if (key == '?') {
|
} else if (key == '?') {
|
||||||
|
|
|
||||||
73
src/tui.c
73
src/tui.c
|
|
@ -19,11 +19,48 @@ void tui_render_screen(client_t *client) {
|
||||||
char buffer[8192];
|
char buffer[8192];
|
||||||
int pos = 0;
|
int pos = 0;
|
||||||
|
|
||||||
|
/* Acquire all data in one lock to prevent TOCTOU */
|
||||||
pthread_rwlock_rdlock(&g_room->lock);
|
pthread_rwlock_rdlock(&g_room->lock);
|
||||||
int online = g_room->client_count;
|
int online = g_room->client_count;
|
||||||
int msg_count = g_room->message_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);
|
pthread_rwlock_unlock(&g_room->lock);
|
||||||
|
|
||||||
|
/* Now render using snapshot (no lock held) */
|
||||||
|
|
||||||
/* Clear and move to top */
|
/* Clear and move to top */
|
||||||
pos += snprintf(buffer + pos, sizeof(buffer) - pos, ANSI_CLEAR ANSI_HOME);
|
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");
|
pos += snprintf(buffer + pos, sizeof(buffer) - pos, ANSI_RESET "\r\n");
|
||||||
|
|
||||||
/* Messages area */
|
/* Render messages from snapshot */
|
||||||
int msg_height = client->height - 3;
|
if (msg_snapshot) {
|
||||||
if (msg_height < 1) msg_height = 1;
|
for (int i = 0; i < snapshot_count; i++) {
|
||||||
|
|
||||||
/* 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;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
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];
|
char msg_line[1024];
|
||||||
message_format(&g_room->messages[i], msg_line, sizeof(msg_line), client->width);
|
message_format(&msg_snapshot[i], msg_line, sizeof(msg_line), client->width);
|
||||||
pos += snprintf(buffer + pos, sizeof(buffer) - pos, "%s\r\n", msg_line);
|
pos += snprintf(buffer + pos, sizeof(buffer) - pos, "%s\r\n", msg_line);
|
||||||
}
|
}
|
||||||
|
free(msg_snapshot);
|
||||||
pthread_rwlock_unlock(&g_room->lock);
|
}
|
||||||
|
|
||||||
/* Fill empty lines */
|
/* 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++] = '\r';
|
||||||
buffer[pos++] = '\n';
|
buffer[pos++] = '\n';
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue