Alawn/shadowsocks-libev
1
0
Fork 0
mirror of https://github.com/shadowsocks/shadowsocks-libev.git synced 2026-11-01 15:33:50 +00:00
Code Releases Activity

Fix bugs found by clang-tidy triage; ratchet warning budget to zero (#3047)

Browse Source 
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 commit is contained in:
Max Lv
2026-07-15 13:02:58 +08:00
committed by GitHub
co-authored by Claude Fable 5
parent ba7b9ad4cc
commit dd2704c857
12 changed files with 53 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.github/workflows/tests.yml
+5 -4
View File
@@ -83,11 +83,12 @@ jobs:
clang-tidy:
runs-on: ubuntu-latest
env:
# Ratchet: number of pre-existing clang-tidy warnings (see .clang-tidy),
# Ratchet: number of known clang-tidy warnings (see .clang-tidy),
# counted with clang-tidy-18 (pinned below so runner upgrades don't move
# the number). CI fails if the count goes UP; lower this value as
# findings get fixed.
MAX_WARNINGS: 29
# the number). All baseline findings have been fixed or triaged
# (false positives carry NOLINT comments with rationale), so any new
# warning fails CI.
MAX_WARNINGS: 0
steps:
- uses: actions/checkout@v6
with:
src/cache.c
+5
View File
@@ -89,6 +89,7 @@ cache_delete(struct cache *cache, int keep_data)
HASH_CLEAR(hh, cache->entries);
} else {
HASH_ITER(hh, cache->entries, entry, tmp){
// NOLINTNEXTLINE(clang-analyzer-unix.Malloc): uthash unlinks here; entry is freed afterwards
HASH_DEL(cache->entries, entry);
if (entry->data != NULL) {
if (cache->free_cb) {
@@ -129,6 +130,7 @@ cache_clear(struct cache *cache, ev_tstamp age)
HASH_ITER(hh, cache->entries, entry, tmp){
if (now - entry->ts > age) {
// NOLINTNEXTLINE(clang-analyzer-unix.Malloc): uthash unlinks here; entry is freed afterwards
HASH_DEL(cache->entries, entry);
if (entry->data != NULL) {
if (cache->free_cb) {
@@ -219,6 +221,7 @@ cache_lookup(struct cache *cache, char *key, size_t key_len, void *result)
if (tmp) {
HASH_DELETE(hh, cache->entries, tmp);
tmp->ts = ev_time();
// NOLINTNEXTLINE(clang-analyzer-core.DivideZero): uthash bucket count is never zero
HASH_ADD_KEYPTR(hh, cache->entries, tmp->key, key_len, tmp);
*dirty_hack = tmp->data;
} else {
@@ -241,6 +244,7 @@ cache_key_exist(struct cache *cache, char *key, size_t key_len)
if (tmp) {
HASH_DELETE(hh, cache->entries, tmp);
tmp->ts = ev_time();
// NOLINTNEXTLINE(clang-analyzer-core.DivideZero): uthash bucket count is never zero
HASH_ADD_KEYPTR(hh, cache->entries, tmp->key, key_len, tmp);
return 1;
}
@@ -284,6 +288,7 @@ cache_insert(struct cache *cache, char *key, size_t key_len, void *data)
entry->data = data;
entry->ts = ev_time();
// NOLINTNEXTLINE(clang-analyzer-core.DivideZero): uthash bucket count is never zero
HASH_ADD_KEYPTR(hh, cache->entries, entry->key, key_len, entry);
if (HASH_COUNT(cache->entries) >= cache->max_entries) {
src/jconf.c
+1
View File
@@ -278,6 +278,7 @@ read_jconf(const char *file)
conf.user = to_string(value);
} else if (strcmp(name, "plugin") == 0) {
conf.plugin = to_string(value);
// NOLINTNEXTLINE(clang-analyzer-unix.Malloc): config strings live for the process lifetime
if (conf.plugin && strlen(conf.plugin) == 0) {
ss_free(conf.plugin);
conf.plugin = NULL;
src/json.c
+1 -1
View File
@@ -666,7 +666,7 @@ json_value * json_parse_ex (json_settings * settings,
{
if ( (++ state.ptr) == end)
{
b = 0;
b = 0; /* NOLINT(clang-analyzer-deadcode.DeadStores) */
break;
}
src/local.c
+4 -4
View File
@@ -844,6 +844,7 @@ server_recv_cb(EV_P_ ev_io *w, int revents)
struct sockaddr_in peer_addr;
socklen_t peer_addr_len = sizeof peer_addr;
if (getpeername(server->fd, (struct sockaddr *)&peer_addr, &peer_addr_len) == 0) {
// NOLINTNEXTLINE(clang-analyzer-core.CallAndMessage): filled by getpeername() on success
LOGI("connection from %s:%hu", inet_ntoa(peer_addr.sin_addr), ntohs(peer_addr.sin_port));
}
}
@@ -1276,7 +1277,7 @@ create_remote(listen_ctx_t *listener,
{
struct sockaddr *remote_addr;
int index = rand() % listener->remote_num;
int index = (int)randombytes_uniform((uint32_t)listener->remote_num);
if (addr == NULL) {
remote_addr = listener->remote_addr[index];
} else {
@@ -1454,7 +1455,6 @@ main(int argc, char **argv)
char *remote_port = NULL;
memset(remote_addr, 0, sizeof(ss_addr_t) * MAX_REMOTE_NUM);
srand(time(NULL));
static struct option long_options[] = {
{ "reuse-port", no_argument, NULL, GETOPT_VAL_REUSE_PORT },
@@ -2059,8 +2059,6 @@ main(int argc, char **argv)
int
_start_ss_local_server(profile_t profile, ss_local_callback callback, void *udata)
{
srand(time(NULL));
char *remote_host = profile.remote_host;
char *local_addr = profile.local_addr;
char *method = profile.method;
@@ -2130,6 +2128,8 @@ _start_ss_local_server(profile_t profile, ss_local_callback callback, void *udat
struct sockaddr *remote_addr_tmp[MAX_REMOTE_NUM];
listen_ctx_t listen_ctx;
// fd stays -1 in UDP_ONLY mode but is still passed to the callback below
listen_ctx.fd = -1;
listen_ctx.remote_num = 1;
listen_ctx.remote_addr = remote_addr_tmp;
listen_ctx.remote_addr[0] = (struct sockaddr *)(&storage);
src/manager.c
+20 -9
View File
@@ -526,7 +526,6 @@ start_server_process(struct manager_ctx *manager, struct server *server)
}
restore_sigchld_after_wait(restore_sigchld, &old_sigchld);
restore_sigchld = 0;
ss_free(pid_path);
ss_free(conf_path);
@@ -852,11 +851,27 @@ add_server(struct manager_ctx *manager, struct server *server)
return 0;
}
static void
kill_pid_from_file(FILE *f)
{
char buf[16];
int pid;
if (fgets(buf, sizeof(buf), f) == NULL) {
return;
}
buf[strcspn(buf, "\r\n")] = '\0';
// Reject malformed pid file content instead of signaling a garbage pid
if (ss_parse_int(buf, 1, INT_MAX, &pid) == 0) {
kill(pid, SIGTERM);
}
}
static void
kill_server(char *prefix, char *pid_file)
{
char *path = NULL;
int pid, path_size = strlen(prefix) + strlen(pid_file) + 2;
int path_size = strlen(prefix) + strlen(pid_file) + 2;
path = ss_malloc(path_size);
snprintf(path, path_size, "%s/%s", prefix, pid_file);
FILE *f = fopen(path, "r");
@@ -867,9 +882,7 @@ kill_server(char *prefix, char *pid_file)
ss_free(path);
return;
}
if (fscanf(f, "%d", &pid) != EOF) {
kill(pid, SIGTERM);
}
kill_pid_from_file(f);
fclose(f);
remove(path);
ss_free(path);
@@ -879,7 +892,7 @@ static void
stop_server(char *prefix, char *port)
{
char *path = NULL;
int pid, path_size = strlen(prefix) + strlen(port) + 20;
int path_size = strlen(prefix) + strlen(port) + 20;
path = ss_malloc(path_size);
snprintf(path, path_size, "%s/.shadowsocks_%s.pid", prefix, port);
FILE *f = fopen(path, "r");
@@ -890,9 +903,7 @@ stop_server(char *prefix, char *port)
ss_free(path);
return;
}
if (fscanf(f, "%d", &pid) != EOF) {
kill(pid, SIGTERM);
}
kill_pid_from_file(f);
fclose(f);
ss_free(path);
}
src/redir.c
+1 -3
View File
@@ -767,7 +767,7 @@ accept_cb(EV_P_ ev_io *w, int revents)
setsockopt(serverfd, SOL_SOCKET, SO_NOSIGPIPE, &opt, sizeof(opt));
#endif
int index = rand() % listener->remote_num;
int index = (int)randombytes_uniform((uint32_t)listener->remote_num);
struct sockaddr *remote_addr = listener->remote_addr[index];
int protocol = IPPROTO_TCP;
@@ -891,8 +891,6 @@ signal_cb(EV_P_ ev_signal *w, int revents)
int
main(int argc, char **argv)
{
srand(time(NULL));
int i, c;
int pid_flags = 0;
int mptcp = 0;
src/server.c
+1
View File
@@ -680,6 +680,7 @@ connect_to_remote(EV_P_ struct addrinfo *res,
char ipstr[INET6_ADDRSTRLEN];
memset(ipstr, 0, INET6_ADDRSTRLEN);
// NOLINTNEXTLINE(clang-analyzer-core.NullDereference): callers always set res->ai_addr
if (res->ai_addr->sa_family == AF_INET) {
struct sockaddr_in s;
memcpy(&s, res->ai_addr, sizeof(struct sockaddr_in));
src/tunnel.c
+2 -3
View File
@@ -728,6 +728,7 @@ static void
free_server(server_t *server)
{
if (server->remote != NULL) {
// NOLINTNEXTLINE(clang-analyzer-unix.Malloc): back-pointers are cleared symmetrically on free
server->remote->server = NULL;
}
if (server->e_ctx != NULL) {
@@ -774,7 +775,7 @@ accept_cb(EV_P_ ev_io *w, int revents)
setsockopt(serverfd, SOL_SOCKET, SO_NOSIGPIPE, &opt, sizeof(opt));
#endif
int index = rand() % listener->remote_num;
int index = (int)randombytes_uniform((uint32_t)listener->remote_num);
struct sockaddr *remote_addr = listener->remote_addr[index];
int protocol = IPPROTO_TCP;
@@ -924,8 +925,6 @@ plugin_watcher_cb(EV_P_ ev_io *w, int revents)
int
main(int argc, char **argv)
{
srand(time(NULL));
int i, c;
int pid_flags = 0;
int mptcp = 0;
src/udprelay.c
+5 -5
View File
@@ -367,10 +367,11 @@ get_addr_str(const struct sockaddr *sa, bool has_port)
int addr_len = strlen(addr);
int port_len = strlen(port);
memcpy(s, addr, addr_len);
// s is a zeroed static buffer large enough for addr + ':' + port
memcpy(s, addr, addr_len); // NOLINT(bugprone-not-null-terminated-result)
if (has_port) {
memcpy(s + addr_len + 1, port, port_len);
memcpy(s + addr_len + 1, port, port_len); // NOLINT(bugprone-not-null-terminated-result)
s[addr_len] = ':';
}
@@ -1023,8 +1024,6 @@ server_recv_cb(EV_P_ ev_io *w, int revents)
LOGE("[udp] unable to get dest addr");
goto CLEAN_UP;
}
src_addr_len = msg.msg_namelen;
#else
ssize_t r;
r = recvfrom(server_ctx->fd, buf->data, buf_size,
@@ -1180,7 +1179,8 @@ server_recv_cb(EV_P_ ev_io *w, int revents)
}
addr_header[addr_header_len++] = 3;
addr_header[addr_header_len++] = host_len;
memcpy(addr_header + addr_header_len, host, host_len);
// addr_header is a binary protocol buffer, not a C string
memcpy(addr_header + addr_header_len, host, host_len); // NOLINT(bugprone-not-null-terminated-result)
addr_header_len += host_len;
}
memcpy(addr_header + addr_header_len, &port_net_num, 2);
src/utils.c
+7 -14
View File
@@ -570,27 +570,21 @@ get_default_conf(void)
{
#ifndef __MINGW32__
static char sysconf[] = "/etc/shadowsocks-libev/config.json";
static char *userconf = NULL;
static int buf_size = 0;
static char userconf[PATH_MAX];
char *conf_home;
conf_home = getenv("XDG_CONFIG_HOME");
// Memory of userconf only gets allocated once, and will not be
// freed. It is used as static buffer.
if (!conf_home) {
if (buf_size == 0) {
buf_size = 50 + strlen(getenv("HOME"));
userconf = malloc(buf_size);
// HOME may be unset (e.g. when started by an init system)
const char *home = getenv("HOME");
if (home == NULL) {
return sysconf;
}
snprintf(userconf, buf_size, "%s%s", getenv("HOME"),
snprintf(userconf, sizeof(userconf), "%s%s", home,
"/.config/shadowsocks-libev/config.json");
} else {
if (buf_size == 0) {
buf_size = 50 + strlen(conf_home);
userconf = malloc(buf_size);
}
snprintf(userconf, buf_size, "%s%s", conf_home,
snprintf(userconf, sizeof(userconf), "%s%s", conf_home,
"/shadowsocks-libev/config.json");
}
@@ -599,7 +593,6 @@ get_default_conf(void)
return userconf;
// If not, fall back to the system-wide config.
free(userconf);
return sysconf;
#else
return "config.json";
src/utils.h
+1 -1
View File
@@ -222,7 +222,7 @@ int ss_isnumeric(const char *s);
int ss_parse_int(const char *s, int min_value, int max_value, int *out);
int ss_parse_uint16_port(const char *s, uint16_t *out);
int run_as(const char *user);
void FATAL(const char *msg);
void FATAL(const char *msg) __attribute__((noreturn));
void usage(void);
void daemonize(const char *path);
char *ss_strndup(const char *s, size_t n);