Conversation
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10
Summary
Five bugs confirmed by static analysis of the full source, all causing crashes or permanent hangs in production.
Changes
1. Signal handler deadlock —
main.csignal_handlercalledroom_destroy()(which acquirespthread_rwlock_wrlock) andprintf()— neither is async-signal-safe. If SIGTERM arrived while any thread heldg_room->lock, the process deadlocked.Also:
close(g_listen_fd)was closingstdin(fd 0) becausessh_server_initreturns0on success, not a real fd.Fix: handler writes via
write(2)(async-signal-safe) and calls_exit(0). Remove the bogusclose(0).2. NULL dereference in
room_broadcast—chat_room.cPOSIX allows
calloc(0, n)to return NULL. No NULL check for the OOM case either.Fix: early return when
count == 0; checkcallocreturn.3. Stack buffer overflow in
tui_render_screen—tui.cchar buffer[8192]is overflowed with tall terminals:197 lines × ~1031 bytes/msg ≈ 203 KiB. Title padding loop also had uncheckedbuffer[pos++].Fix:
malloc(65536)withbuf_sizeused consistently throughout. Bounds check on padding loop.4.
sleep(2)inside libssh auth callback —ssh_server.cauth_passwordis invoked fromssh_event_dopollin the main thread. Sleeping there blocked the entire accept loop — one attacker with repeated wrong passwords stalled all connections for 2s per attempt. IP blocking viarecord_auth_failurealready handles brute force.Fix: remove
sleep(2)fromauth_password.5. Spurious
sleep()in accept loop error paths —ssh_server.csleep(1)/sleep(2)after rejecting rate-limited connections blocked accepting the next legitimate connection with no benefit.Fix: remove all
sleep()from accept loop error paths.Test plan
make clean && makesystemctlkill -TERM $(pidof tnt)