fix: address 10 security and correctness issues from code audit

[Jayian1890]

Summary

This PR addresses 10 bugs and safety issues found during a full manual code audit of socks5.c. All changes are in a single commit, and the build produces zero warnings (clang -Wall -Wextra). The complete test suite passes (the pre-existing GSSAPI Advertised failure without HAVE_GSSAPI is unchanged).

---

Fixes

🔴 Critical

1. USER_PASS authentication always disconnected before relay (goto cleanup bug)

The if (selected_method == SOCKS5_AUTH_USER_PASS) block unconditionally executed goto cleanup, meaning the proxy was completely non-functional when authentication was enabled — the client never got to send a CONNECT/BIND/UDP request. Fixed so that only a failed authentication jumps to cleanup; successful auth falls through to step 3 (Request).

---

🟠 High

2. pthread_create failure did not decrement active_connections

active_connections was incremented before pthread_create. On failure the socket and session were freed, but the counter was never decremented. Over time this would cause the counter to permanently drift upward, eventually blocking all new connections. Added atomic_fetch_sub in the failure path.

3. max_connections TOCTOU race

The previous pattern (atomic_load → check → atomic_fetch_add) allowed multiple threads accepting simultaneously to each read a value just below the limit and all proceed, collectively exceeding max_connections. Fixed by doing the atomic_fetch_add first and checking the returned (pre-increment) value, decrementing back if over the limit.

4. server->running — volatile bool instead of _Atomic bool

volatile does not provide any memory-ordering guarantees in C11. Worker threads spinning on while (server->running) could observe a stale true after another thread called socks5_server_stop. Changed to _Atomic bool; existing = true/false assignments and direct reads are valid atomic operations in C11.

5. localtime() is not thread-safe

localtime() uses an internal static buffer and is not reentrant. Under concurrent connections this is a data race. Replaced with localtime_r() on POSIX and localtime_s() on Windows using an #ifdef _WIN32 guard.

---

🟡 Medium

6. realloc result stored directly — pointer lost on failure

// Before (leaks original pointer if realloc returns NULL):
config->allow_ips = (char **)realloc(config->allow_ips, ...);

Fixed by storing the result in a temporary pointer and only assigning on success.

7. UDP relay: silent sendto() to 0.0.0.0:0 on getaddrinfo overflow

When the resolved address was larger than target_addr, the memcpy was silently skipped but sendto() was still called with an uninitialized/zeroed target_addr. Added else { freeaddrinfo(res); continue; } to skip the packet cleanly.

8. GSSAPI token length — strict-aliasing UB

// Before: undefined behaviour (misaligned/aliased read):
uint16_t in_len = (uint16_t)ntohs(*(uint16_t *)lenbuf);

Fixed with memcpy into a properly-typed variable before ntohs().

---

🔵 Low

9. nmethods bounds check used sizeof(buf) instead of 255

The SOCKS5 spec limits nmethods to 255 (1-byte field). The check nmethods > (int)sizeof(buf) was checking against the unrelated buffer size (512). Fixed to > 255.

10. Dead code: plen > 255 always false for uint8_t

plen is declared uint8_t, so plen > 255 can never be true. Removed the dead check.

---

Remaining issues

Remaining audit findings (timing-safe auth, system() → execvp(), handle_signal async-safety, ssize_t portability on Windows, credential logging, etc.) are tracked in #2.