From da81e1718729b0bcfbe74b665a9157683df5b199 Mon Sep 17 00:00:00 2001 From: m1ngsama Date: Fri, 6 Mar 2026 01:58:56 +0800 Subject: [PATCH] fix: resolve crash and hang causes found in production audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #10. Five bugs that caused the server to crash or become unresponsive: 1. Signal handler deadlock (main.c) signal_handler called room_destroy (pthread_rwlock + free) and printf — neither is async-signal-safe. If SIGTERM arrived while any thread held g_room->lock, the process deadlocked permanently. Fix: handler now only writes a message via write(2) and calls _exit(0). Also remove close(g_listen_fd) which was closing stdin (fd 0), since ssh_server_init returns 0 on success, not a real file descriptor. 2. NULL dereference in room_broadcast when room is empty (chat_room.c) calloc(0, n) may return NULL per POSIX; memcpy on NULL is undefined. Also: no NULL check after calloc for the OOM case. Fix: early return if count == 0; check calloc return value. 3. Stack buffer overflow in tui_render_screen (tui.c) char buffer[8192] overflows with tall terminals: 197 visible lines * ~1031 bytes/message ≈ 203 KiB. Title padding loop also lacked a bounds check (buffer[pos++] = ' ' with no guard). Fix: switch to malloc(65536) with buf_size used consistently. Add bounds check to the title padding loop. 4. sleep() inside libssh auth callback (ssh_server.c) auth_password is called from ssh_event_dopoll in the main thread. sleep(2) there blocks the entire accept loop — one attacker with repeated wrong passwords stalls all incoming connections. IP blocking via record_auth_failure already handles brute force. Fix: remove sleep(2) from auth_password. 5. Spurious sleep() calls in the main accept loop (ssh_server.c) sleep(1/2) after rejecting rate-limited or over-limit connections delays accepting the next legitimate connection for no benefit. Fix: remove all sleep() from the accept loop error paths. --- src/chat_room.c | 10 +++++++++- src/main.c | 25 ++++++++----------------- src/ssh_server.c | 9 ++------- src/tui.c | 31 ++++++++++++++++++------------- 4 files changed, 37 insertions(+), 38 deletions(-) diff --git a/src/chat_room.c b/src/chat_room.c index e2815f0..b95153f 100644 --- a/src/chat_room.c +++ b/src/chat_room.c @@ -82,8 +82,16 @@ void room_broadcast(chat_room_t *room, const message_t *msg) { room_add_message(room, msg); /* Get copy of client list and increment ref counts */ - client_t **clients_copy = calloc(room->client_count, sizeof(client_t*)); int count = room->client_count; + if (count == 0) { + pthread_rwlock_unlock(&room->lock); + return; + } + client_t **clients_copy = calloc(count, sizeof(client_t*)); + if (!clients_copy) { + pthread_rwlock_unlock(&room->lock); + return; + } memcpy(clients_copy, room->clients, count * sizeof(client_t*)); /* Increment reference count for each client */ diff --git a/src/main.c b/src/main.c index b559abd..cf38110 100644 --- a/src/main.c +++ b/src/main.c @@ -5,19 +5,14 @@ #include #include -static int g_listen_fd = -1; - -/* Signal handler for graceful shutdown */ +/* Signal handler: must only call async-signal-safe functions. + * pthread, malloc, printf, exit() are NOT safe here. + * Just write a message and call _exit() — OS reclaims all resources. */ static void signal_handler(int sig) { (void)sig; - if (g_listen_fd >= 0) { - close(g_listen_fd); - } - if (g_room) { - room_destroy(g_room); - } - printf("\nShutting down...\n"); - exit(0); + static const char msg[] = "\nShutting down...\n"; + (void)write(STDERR_FILENO, msg, sizeof(msg) - 1); + _exit(0); } int main(int argc, char **argv) { @@ -60,19 +55,15 @@ int main(int argc, char **argv) { } /* Initialize server */ - g_listen_fd = ssh_server_init(port); - if (g_listen_fd < 0) { + if (ssh_server_init(port) < 0) { fprintf(stderr, "Failed to initialize server\n"); room_destroy(g_room); return 1; } /* Start server (blocking) */ - int ret = ssh_server_start(g_listen_fd); + int ret = ssh_server_start(0); - /* Cleanup */ - close(g_listen_fd); room_destroy(g_room); - return ret; } diff --git a/src/ssh_server.c b/src/ssh_server.c index 15c0c40..5fab76e 100644 --- a/src/ssh_server.c +++ b/src/ssh_server.c @@ -482,7 +482,6 @@ static int read_username(client_t *client) { if (!is_valid_username(client->username)) { client_printf(client, "Invalid username. Using 'anonymous' instead.\r\n"); strcpy(client->username, "anonymous"); - sleep(1); /* Slow down rapid retry attempts */ } else { /* Truncate to 20 characters */ if (utf8_strlen(client->username) > 20) { @@ -929,9 +928,9 @@ static int auth_password(ssh_session session, const char *user, ctx->auth_success = true; return SSH_AUTH_SUCCESS; } else { - /* Wrong token */ + /* Wrong token — IP blocking handles brute force, no sleep needed here + * (sleeping in a libssh callback blocks the entire accept loop). */ record_auth_failure(ctx->client_ip); - sleep(2); /* Slow down brute force */ return SSH_AUTH_DENIED; } } else { @@ -1149,7 +1148,6 @@ int ssh_server_start(int unused) { ssh_disconnect(session); ssh_free(session); free(ctx); - sleep(1); /* Slow down blocked clients */ continue; } @@ -1159,7 +1157,6 @@ int ssh_server_start(int unused) { ssh_disconnect(session); ssh_free(session); free(ctx); - sleep(1); continue; } @@ -1182,7 +1179,6 @@ int ssh_server_start(int unused) { ssh_disconnect(session); ssh_free(session); free(ctx); - sleep(1); continue; } @@ -1230,7 +1226,6 @@ int ssh_server_start(int unused) { ssh_free(session); if (ctx->channel_cb) free(ctx->channel_cb); free(ctx); - sleep(2); /* Longer delay for auth failures */ continue; } diff --git a/src/tui.c b/src/tui.c index 54dd9fb..b9d5198 100644 --- a/src/tui.c +++ b/src/tui.c @@ -16,7 +16,11 @@ void tui_clear_screen(client_t *client) { void tui_render_screen(client_t *client) { if (!client || !client->connected) return; - char buffer[8192]; + /* Heap-allocated: worst case is ~200 messages * ~1100 bytes + separator + status. + * 64 KiB covers all real terminal sizes without stack risk. */ + const size_t buf_size = 65536; + char *buffer = malloc(buf_size); + if (!buffer) return; int pos = 0; /* Acquire all data in one lock to prevent TOCTOU */ @@ -62,7 +66,7 @@ void tui_render_screen(client_t *client) { /* Now render using snapshot (no lock held) */ /* Move to top (Home) - Do NOT clear screen to prevent flicker */ - pos += snprintf(buffer + pos, sizeof(buffer) - pos, ANSI_HOME); + pos += snprintf(buffer + pos, buf_size - pos, ANSI_HOME); /* Title bar */ const char *mode_str = (client->mode == MODE_INSERT) ? "INSERT" : @@ -78,51 +82,52 @@ void tui_render_screen(client_t *client) { int padding = client->width - title_width; if (padding < 0) padding = 0; - pos += snprintf(buffer + pos, sizeof(buffer) - pos, ANSI_REVERSE "%s", title); - for (int i = 0; i < padding; i++) { + pos += snprintf(buffer + pos, buf_size - pos, ANSI_REVERSE "%s", title); + for (int i = 0; i < padding && pos < (int)buf_size - 4; i++) { buffer[pos++] = ' '; } - pos += snprintf(buffer + pos, sizeof(buffer) - pos, ANSI_RESET "\033[K\r\n"); + pos += snprintf(buffer + pos, buf_size - pos, ANSI_RESET "\033[K\r\n"); /* 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\033[K\r\n", msg_line); + pos += snprintf(buffer + pos, buf_size - pos, "%s\033[K\r\n", msg_line); } free(msg_snapshot); } /* Fill empty lines and clear them */ for (int i = snapshot_count; i < msg_height; i++) { - pos += snprintf(buffer + pos, sizeof(buffer) - pos, "\033[K\r\n"); + pos += snprintf(buffer + pos, buf_size - pos, "\033[K\r\n"); } /* Separator - use box drawing character */ - for (int i = 0; i < client->width && pos < (int)sizeof(buffer) - 10; i++) { - const char *line_char = "─"; /* U+2500 box drawing */ + for (int i = 0; i < client->width && pos < (int)buf_size - 10; i++) { + const char *line_char = "─"; /* U+2500 box drawing, 3 bytes */ int len = strlen(line_char); memcpy(buffer + pos, line_char, len); pos += len; } - pos += snprintf(buffer + pos, sizeof(buffer) - pos, "\033[K\r\n"); + pos += snprintf(buffer + pos, buf_size - pos, "\033[K\r\n"); /* Status/Input line */ if (client->mode == MODE_INSERT) { - pos += snprintf(buffer + pos, sizeof(buffer) - pos, "> \033[K"); + pos += snprintf(buffer + pos, buf_size - pos, "> \033[K"); } else if (client->mode == MODE_NORMAL) { int total = msg_count; int scroll_pos = client->scroll_pos + 1; if (total == 0) scroll_pos = 0; - pos += snprintf(buffer + pos, sizeof(buffer) - pos, + pos += snprintf(buffer + pos, buf_size - pos, "-- NORMAL -- (%d/%d)\033[K", scroll_pos, total); } else if (client->mode == MODE_COMMAND) { - pos += snprintf(buffer + pos, sizeof(buffer) - pos, + pos += snprintf(buffer + pos, buf_size - pos, ":%s\033[K", client->command_input); } client_send(client, buffer, pos); + free(buffer); } /* Render the input line */