Fix bugs found by clang-tidy triage; ratchet warning budget to zero - #3047
Merged
Merged
Conversation
Real bugs fixed: - manager.c: kill_server/stop_server checked fscanf() != EOF, so malformed pid file content left pid uninitialized and passed it to kill(). Parse with fgets + ss_parse_int and require pid > 0. - utils.c: get_default_conf() crashed on strlen(NULL) when HOME is unset (e.g. daemons started by init) and left a dangling static pointer after free(), a latent use-after-free on repeated calls. Use a static buffer and fall back to the system config when HOME is missing. - local.c: in UDP_ONLY mode, start_ss_local_server() passed uninitialized listen_ctx.fd to the library callback. Initialize it to -1. Improvements: - Replace rand()/srand(time(NULL)) upstream-server selection in ss-local, ss-redir and ss-tunnel with libsodium randombytes_uniform(): unbiased, unpredictable, and no seeding required (cert-msc30/msc32). - Mark FATAL() noreturn so both the compiler and analyzers understand control flow (removes a family of analyzer false positives). - Drop two dead stores (manager.c restore_sigchld, udprelay.c src_addr_len). All remaining findings were verified as false positives and carry NOLINT comments with rationale (uthash macro internals, symmetric back-pointer cleanup, analyzer-invisible postconditions). clang-tidy-18 on Linux now reports zero warnings, so MAX_WARNINGS drops from 29 to 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Summary
Full triage of the 29 baseline clang-tidy findings from #3046: every finding was either fixed or verified as a false positive and annotated. The CI ratchet (
MAX_WARNINGS) drops from 29 to 0, so any new clang-tidy finding now fails CI.Real bugs fixed
manager.c— uninitialized pid passed tokill():kill_server/stop_servercheckedfscanf(...) != EOF, which passes when the pid file contains garbage (fscanf returns 0), leavingpiduninitialized and signaling an arbitrary process — potentially catastrophic when ss-manager runs as root. Now parsed withfgets+ the hardenedss_parse_int, requiringpid > 0.utils.c— crash whenHOMEis unset:get_default_conf()calledstrlen(getenv("HOME"))without a NULL check (daemons started by init systems commonly have noHOME), and also left a dangling static pointer afterfree(), a latent use-after-free on repeated calls. Rewritten with a static buffer and a graceful fallback to the system config. Verified:env -u HOME ss-localnow errors cleanly instead of segfaulting.local.c— uninitialized fd handed to library callback: in UDP_ONLY modestart_ss_local_server()never setlisten_ctx.fdbut still passed it to the callback. Initialized to -1.Improvements
randombytes_uniform()instead ofrand() % nwithsrand(time(NULL))— unbiased and unpredictable, no seeding needed.FATAL()is nownoreturn, teaching the compiler and analyzers about control flow (eliminates a family of analyzer false positives).manager.c,udprelay.c).False positives (NOLINT with rationale)
uthash macro internals (use-after-free / div-by-zero models), symmetric server<->remote back-pointer cleanup,
getpeername/cork_ip_initpostconditions the analyzer can't see, binary protocol buffers flagged by C-string checks, and one intentional dead store in vendoredjson.c.Verification
ss-managertarget compiles under-Werror.🤖 Generated with Claude Code