fix: resolve crash and hang causes found in production audit

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.
This commit is contained in:
m1ngsama 2026-03-06 01:58:56 +08:00
parent 25a277ab27
commit da81e17187
4 changed files with 37 additions and 38 deletions

View file

@ -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 */

View file

@ -5,19 +5,14 @@
#include <signal.h>
#include <unistd.h>
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;
}

View file

@ -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;
}

View file

@ -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 */