- Make client width/height _Atomic to fix data race between window-change
callback thread and session thread
- Persist join/leave system messages via message_save()
- Fix ftell() error handling: check <= 0 instead of == 0
- Make room_add_message static to enforce lock-must-be-held contract
- Use local copy in execute_command to avoid mutating command_input
- Increase help_copy buffer from 4096 to 8192 for CJK text safety
- Add :q/:quit/:exit command for Vim-style disconnect
- Fix unit test Makefile to link common.c
Critical fixes:
- C-1: Use atomic_bool for client->connected and redraw_pending to prevent
data races between callback and main threads
- C-2: Add reference counting for channel callbacks to prevent use-after-free
when callbacks fire during client cleanup
- C-3/M-7: Use ssh_channel_read_timeout (5s) for UTF-8 continuation bytes
to prevent thread blocking and stream desynchronization
High-severity fixes:
- H-1: Replace non-thread-safe setenv/tzset with timegm() in parse_rfc3339_utc
- H-2: Change room_get_message to return by value copy instead of interior pointer
- H-3: Log warning when rate-limit table evicts active IP entry
- H-4: Replace strcmp with constant-time comparison for access token validation
- H-5: Check signature_state in auth_pubkey to reject unsigned key offers
Medium/low fixes:
- M-1: Replace all atoi() with strtol() for proper error detection
- M-3: Move calloc outside rwlock in tui_render_screen to avoid blocking writers
- M-8: Fix off-by-one in rate limit threshold (> to >=)
- M-9: Trim partial UTF-8 sequences after snprintf truncation in message_format
- L-1: Validate continuation byte mask (0xC0==0x80) in utf8_decode
- D-3: Remove vestigial client_t.fd field
- L-3: Remove unreachable pthread_attr_destroy after infinite loop
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.
- 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
Fixes three critical bugs that caused crashes after long-running:
1. Use-after-free race condition in room_broadcast()
- Added reference counting to client_t structure
- Increment ref_count before using client outside lock
- Decrement and free only when ref_count reaches 0
- Prevents accessing freed client memory during broadcast
2. strtok() data corruption in tui_render_command_output()
- strtok() modifies original string by replacing delimiters
- Now use a local copy before calling strtok()
- Prevents corruption of client->command_output
3. Improved handle_key() consistency
- Return bool to indicate if key was consumed
- Fixes issue where mode-switch keys were processed twice
Thread safety changes:
- Added client->ref_count and client->ref_lock
- Added client_release() for safe cleanup
- room_broadcast() now properly increments/decrements refs
This fixes the primary cause of crashes during extended operation.
