Fix expanded C CodeQL findings

Author: ktsaou

.agents/sow/current/SOW-0014-20260603-maintainability-hotspots.md

@@ -920,6 +920,55 @@ Raw cache, Go typed-facade, apps lookup builder, cgroups lookup builder, apps lo
   - `bash .agents/sow/audit.sh` passed.
   - `codacy-analysis analyze --output-format json` passed with 0 issues and 0 errors across Checkov, Opengrep/Semgrep, Trivy, cppcheck, ShellCheck, and Spectral.
 
+### 2026-06-04 Expanded CodeQL Findings
+
+- After commit `97557fd`, GitHub CodeQL reported 56 open code-scanning alerts, all from category `/language:c-cpp-posix`.
+- Evidence from GitHub code scanning:
+  - `cpp/path-injection`: 12 high alerts across POSIX SHM production transport, C interop fixtures, fuzz helper, and POSIX benchmark helper.
+  - `cpp/world-writable-file-creation`: 2 high alerts in C test/interop helpers.
+  - `cpp/wrong-type-format-argument`: 1 high alert in C ping-pong test output.
+  - `cpp/stack-address-escape`: 4 warning alerts across C POSIX service, payload-limit test, and benchmark helper.
+  - `cpp/unused-local-variable`: 31 note alerts in C tests and benchmark helper.
+  - `cpp/unused-static-function`: 5 note alerts in C protocol/SHM tests.
+  - `cpp/long-switch`: 1 note alert in C UDS fault-response test.
+- Root-cause model:
+  - The stronger C/C++ CodeQL build now compiles test, interop, fuzz, cache, stress, hardening, and benchmark C targets that were not extracted by the earlier default setup.
+  - Most alerts are newly visible pre-existing code patterns, not regressions introduced by the Windows CI workflow itself.
+  - Some alerts are real hygiene issues in test and benchmark code; at least two path findings and one stack-address warning touch production POSIX C code and need source review rather than dismissal.
+- Decision:
+  - Do not weaken CodeQL coverage or disable the rules by default.
+  - First fix source patterns that are real or cheaply made explicit.
+  - Use narrow suppression or workflow configuration only for remaining findings proven to be intentional test scaffolding after source review.
+- Risk:
+  - Production POSIX SHM path handling changes can affect SHM lifecycle, stale-file recovery, and interop tests.
+  - Stack-lifetime fixes around server/session code can affect thread and signal lifetime if handled mechanically.
+  - Removing stale test locals and wiring previously unused static tests is low risk but still needs CTest validation.
+- Validation plan:
+  - Build C targets locally with CMake.
+  - Run focused C protocol, POSIX UDS/SHM/service, fuzz, and benchmark target validation.
+  - Run action/static checks available locally.
+  - Push and verify GitHub CodeQL alerts close or reduce to only documented intentional findings.
+- Implemented:
+  - Removed stale typed-call request/response locals from C service, chaos, and benchmark tests.
+  - Wired dormant C protocol coverage tests into `test_protocol` and relaxed one overly-specific synthetic-overflow assertion to require rejection rather than a single error code.
+  - Removed an unused SHM test helper.
+  - Fixed portable `uint32_t` formatting in the C ping-pong test.
+  - Changed the C payload-limit test overflow fixture to use static storage instead of sharing a stack buffer with server handler state.
+  - Changed POSIX benchmark timer threads to receive heap-owned duration arguments instead of stack addresses.
+  - Changed POSIX SHM stale recovery and attach/create paths to open session files with `openat()` relative to a validated directory file descriptor and to unlink with `unlinkat()` where the directory identity matters.
+  - Added a C interop/benchmark run-directory helper requiring existing absolute non-symlink directories owned by the current user and not group/world writable.
+  - Removed C interop/benchmark helper-side `mkdir(run_dir)` calls; repo scripts already create the temporary run directories.
+  - Changed the C codec interop fixture writer to create files with explicit `0600` mode.
+  - Changed the standalone C fuzz harness to read bytes from stdin only and updated the extended fuzz script accordingly.
+  - Changed the UDS stale-recovery regular-file test fixture to use explicit `0600` file creation.
+- Local validation:
+  - `cmake --build build --target netipc_shm netipc_service test_protocol test_uds test_shm test_service test_service_payload_limits test_chaos test_ping_pong fuzz_protocol interop_codec_c interop_uds_c interop_shm_c interop_service_c interop_cache_c bench_posix_c` passed.
+  - `/usr/bin/ctest --test-dir build --output-on-failure -R '^(test_protocol|test_uds|test_shm|test_service|test_service_payload_limits|test_chaos|test_ping_pong|fuzz_protocol_30s|interop_codec|test_uds_interop|test_shm_interop|test_service_interop|test_cache_interop)$'` passed: 13/13 focused tests.
+  - `codacy-analysis analyze --output-format json` passed with 0 issues and 0 errors across Checkov, Opengrep/Semgrep, Trivy, cppcheck, ShellCheck, and Spectral.
+  - `git diff --check` passed.
+  - `bash .agents/sow/audit.sh` passed.
+  - Sensitive-data scan across the touched durable artifacts returned no matches.
+
 ## Lessons Extracted
 
 Pending.

CMakeLists.txt

@@ -968,6 +968,8 @@ endif()
 if(NOT NETIPC_WINDOWS_RUNTIME AND NOT APPLE)
     # C benchmark binary
     add_executable(bench_posix_c bench/drivers/c/bench_posix.c)
+    target_include_directories(bench_posix_c PRIVATE
+        "${CMAKE_SOURCE_DIR}/tests/fixtures/c")
     target_link_libraries(bench_posix_c PRIVATE
         netipc_service netipc_shm netipc_uds netipc_protocol Threads::Threads m
     )

bench/drivers/c/bench_posix.c

@@ -27,6 +27,7 @@
 #include "netipc/netipc_protocol.h"
 #include "netipc/netipc_uds.h"
 #include "netipc/netipc_shm.h"
+#include "interop_path.h"
 
 #include <errno.h>
 #include <math.h>
@@ -313,7 +314,9 @@ static void sighandler(int sig)
 /* Timer thread: stops server after duration_sec */
 static void *timer_thread(void *arg)
 {
-    int duration_sec = *(int *)arg;
+    int *duration_arg = (int *)arg;
+    int duration_sec = *duration_arg;
+    free(duration_arg);
     sleep(duration_sec + 3);
     if (g_server)
         nipc_server_stop(g_server);
@@ -368,11 +371,20 @@ static int run_server(const char *run_dir, const char *service,
     pthread_t timer_tid = 0;
     int timer_failed = 0;
     if (duration_sec > 0) {
-        int rc = pthread_create(&timer_tid, NULL, timer_thread, &duration_sec);
-        if (rc != 0) {
-            fprintf(stderr, "timer thread create failed: %s\n", strerror(rc));
+        int *duration_arg = malloc(sizeof(*duration_arg));
+        if (!duration_arg) {
+            fprintf(stderr, "timer duration allocation failed\n");
             timer_failed = 1;
             nipc_server_stop(&server);
+        } else {
+            *duration_arg = duration_sec;
+            int rc = pthread_create(&timer_tid, NULL, timer_thread, duration_arg);
+            if (rc != 0) {
+                free(duration_arg);
+                fprintf(stderr, "timer thread create failed: %s\n", strerror(rc));
+                timer_failed = 1;
+                nipc_server_stop(&server);
+            }
         }
     }
 
@@ -443,11 +455,20 @@ static int run_server_batch(const char *run_dir, const char *service,
     pthread_t timer_tid = 0;
     int timer_failed = 0;
     if (duration_sec > 0) {
-        int rc = pthread_create(&timer_tid, NULL, timer_thread, &duration_sec);
-        if (rc != 0) {
-            fprintf(stderr, "batch timer thread create failed: %s\n", strerror(rc));
+        int *duration_arg = malloc(sizeof(*duration_arg));
+        if (!duration_arg) {
+            fprintf(stderr, "batch timer duration allocation failed\n");
             timer_failed = 1;
             nipc_server_stop(&server);
+        } else {
+            *duration_arg = duration_sec;
+            int rc = pthread_create(&timer_tid, NULL, timer_thread, duration_arg);
+            if (rc != 0) {
+                free(duration_arg);
+                fprintf(stderr, "batch timer thread create failed: %s\n", strerror(rc));
+                timer_failed = 1;
+                nipc_server_stop(&server);
+            }
         }
     }
 
@@ -924,9 +945,6 @@ static int run_snapshot_client(const char *run_dir, const char *service,
 
     uint64_t requests = 0;
     uint64_t errors = 0;
-    uint8_t req_buf[64];
-    uint8_t resp_buf[RESPONSE_BUF_SIZE];
-
     uint64_t cpu_start = cpu_ns();
     uint64_t wall_start = now_ns();
     uint64_t wall_end = wall_start + (uint64_t)duration_sec * 1000000000ull;
@@ -1362,12 +1380,12 @@ int main(int argc, char **argv)
             return 1;
         }
 
-        const char *run_dir = argv[2];
+        char run_dir[PATH_MAX];
+        if (nipc_test_resolve_run_dir(argv[2], run_dir, sizeof(run_dir)) != 0)
+            return 1;
         const char *service = argv[3];
         int duration = (argc >= 5) ? atoi(argv[4]) : DEFAULT_DURATION;
 
-        mkdir(run_dir, 0700);
-
         uint32_t profiles;
         uint16_t expected_method_code;
         nipc_server_handler_fn handler;
@@ -1403,7 +1421,9 @@ int main(int argc, char **argv)
             return 1;
         }
 
-        const char *run_dir = argv[2];
+        char run_dir[PATH_MAX];
+        if (nipc_test_resolve_run_dir(argv[2], run_dir, sizeof(run_dir)) != 0)
+            return 1;
         const char *service = argv[3];
         int duration = atoi(argv[4]);
         uint64_t target_rps = (uint64_t)strtoull(argv[5], NULL, 10);
@@ -1434,7 +1454,9 @@ int main(int argc, char **argv)
             return 1;
         }
 
-        const char *run_dir = argv[2];
+        char run_dir[PATH_MAX];
+        if (nipc_test_resolve_run_dir(argv[2], run_dir, sizeof(run_dir)) != 0)
+            return 1;
         const char *service = argv[3];
         int duration = atoi(argv[4]);
         uint64_t target_rps = (uint64_t)strtoull(argv[5], NULL, 10);
@@ -1464,12 +1486,12 @@ int main(int argc, char **argv)
             return 1;
         }
 
-        const char *run_dir = argv[2];
+        char run_dir[PATH_MAX];
+        if (nipc_test_resolve_run_dir(argv[2], run_dir, sizeof(run_dir)) != 0)
+            return 1;
         const char *service = argv[3];
         int duration = (argc >= 5) ? atoi(argv[4]) : DEFAULT_DURATION;
 
-        mkdir(run_dir, 0700);
-
         uint32_t profiles;
         if (strcmp(cmd, "uds-batch-ping-pong-server") == 0)
             profiles = BENCH_PROFILE_UDS;
@@ -1490,7 +1512,9 @@ int main(int argc, char **argv)
             return 1;
         }
 
-        const char *run_dir = argv[2];
+        char run_dir[PATH_MAX];
+        if (nipc_test_resolve_run_dir(argv[2], run_dir, sizeof(run_dir)) != 0)
+            return 1;
         const char *service = argv[3];
         int duration = atoi(argv[4]);
         uint64_t target_rps = (uint64_t)strtoull(argv[5], NULL, 10);
@@ -1518,7 +1542,9 @@ int main(int argc, char **argv)
             return 1;
         }
 
-        const char *run_dir = argv[2];
+        char run_dir[PATH_MAX];
+        if (nipc_test_resolve_run_dir(argv[2], run_dir, sizeof(run_dir)) != 0)
+            return 1;
         const char *service = argv[3];
         int duration = atoi(argv[4]);
         uint64_t target_rps = (uint64_t)strtoull(argv[5], NULL, 10);
@@ -1685,7 +1711,9 @@ int main(int argc, char **argv)
             return 1;
         }
 
-        const char *run_dir = argv[2];
+        char run_dir[PATH_MAX];
+        if (nipc_test_resolve_run_dir(argv[2], run_dir, sizeof(run_dir)) != 0)
+            return 1;
         const char *service = argv[3];
         int duration = atoi(argv[4]);
         uint64_t target_rps = (uint64_t)strtoull(argv[5], NULL, 10);

src/libnetdata/netipc/src/transport/posix/netipc_shm.c

@@ -86,10 +86,10 @@ static int build_shm_path(char *dst, size_t dst_len,
     return 0;
 }
 
-static bool run_dir_allows_stale_unlink(const char *run_dir)
+static bool dir_fd_allows_stale_unlink(int dir_fd)
 {
     struct stat st;
-    if (stat(run_dir, &st) != 0)
+    if (dir_fd < 0 || fstat(dir_fd, &st) != 0)
         return false;
     if (!S_ISDIR(st.st_mode))
         return false;
@@ -189,13 +189,13 @@ static int unlink_same_file(int dir_fd, const char *name, const struct stat *exp
     return -1;
 }
 
-static int check_shm_stale(const char *path, int dir_fd, const char *name,
+static int check_shm_stale(int dir_fd, const char *name,
                            bool allow_stale_unlink)
 {
 #ifdef O_NOFOLLOW
-    int fd = open(path, O_RDONLY | O_NOFOLLOW);
+    int fd = openat(dir_fd, name, O_RDONLY | O_NOFOLLOW | O_CLOEXEC);
 #else
-    int fd = open(path, O_RDONLY);
+    int fd = openat(dir_fd, name, O_RDONLY | O_CLOEXEC);
 #endif
     if (fd < 0) {
         if (errno == ENOENT)
@@ -282,53 +282,62 @@ nipc_shm_error_t nipc_shm_server_create(const char *run_dir,
     if (name_rc < 0)
         return NIPC_SHM_ERR_PATH_TOO_LONG;
 
+    int dir_fd = open(run_dir, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+    if (dir_fd < 0)
+        return NIPC_SHM_ERR_OPEN;
+
     /* Round capacities up to alignment. */
     req_capacity  = align64(req_capacity);
     resp_capacity = align64(resp_capacity);
 
     uint32_t req_off  = align64(NIPC_SHM_HEADER_LEN);
     /* Guard against uint32 overflow before computing resp_off */
-    if (req_capacity > UINT32_MAX - req_off)
+    if (req_capacity > UINT32_MAX - req_off) {
+        close(dir_fd);
         return NIPC_SHM_ERR_BAD_PARAM;
+    }
     uint32_t resp_off = align64(req_off + req_capacity);
-    if (resp_capacity > UINT32_MAX - resp_off)
+    if (resp_capacity > UINT32_MAX - resp_off) {
+        close(dir_fd);
         return NIPC_SHM_ERR_BAD_PARAM;
+    }
     size_t region_size = (size_t)resp_off + resp_capacity;
 
     /* Try O_EXCL create first (fast path, no stale check needed). */
-    int fd = open(path, O_RDWR | O_CREAT | O_EXCL, 0600);
+    int fd = openat(dir_fd, shm_name,
+                    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0600);
 
     /* If O_EXCL failed because file exists, do stale recovery and retry. */
     if (fd < 0 && errno == EEXIST) {
-        bool allow_stale_unlink = run_dir_allows_stale_unlink(run_dir);
-        int dir_fd = allow_stale_unlink
-                         ? open(run_dir, O_RDONLY | O_DIRECTORY | O_CLOEXEC)
-                         : -1;
-        if (dir_fd < 0)
-            allow_stale_unlink = false;
-
-        int stale = check_shm_stale(path, dir_fd, shm_name, allow_stale_unlink);
-        if (dir_fd >= 0)
+        bool allow_stale_unlink = dir_fd_allows_stale_unlink(dir_fd);
+
+        int stale = check_shm_stale(dir_fd, shm_name, allow_stale_unlink);
+        if (stale == 1) {
             close(dir_fd);
-        if (stale == 1)
             return NIPC_SHM_ERR_ADDR_IN_USE;
+        }
         /* Stale file was unlinked, retry create */
-        fd = open(path, O_RDWR | O_CREAT | O_EXCL, 0600);
+        fd = openat(dir_fd, shm_name,
+                    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0600);
     }
-    if (fd < 0)
+    if (fd < 0) {
+        close(dir_fd);
         return NIPC_SHM_ERR_OPEN;
+    }
 
     if (ftruncate(fd, (off_t)region_size) < 0) {
         close(fd);
-        unlink(path);
+        unlinkat(dir_fd, shm_name, 0);
+        close(dir_fd);
         return NIPC_SHM_ERR_TRUNCATE;
     }
 
     void *map = mmap(NULL, region_size, PROT_READ | PROT_WRITE,
                      MAP_SHARED, fd, 0);
     if (map == MAP_FAILED) {
         close(fd);
-        unlink(path);
+        unlinkat(dir_fd, shm_name, 0);
+        close(dir_fd);
         return NIPC_SHM_ERR_MMAP;
     }
 
@@ -372,6 +381,7 @@ nipc_shm_error_t nipc_shm_server_create(const char *run_dir,
     strncpy(out->path, path, sizeof(out->path) - 1);
     out->path[sizeof(out->path) - 1] = '\0';
 
+    close(dir_fd);
     return NIPC_SHM_OK;
 }
 
@@ -425,8 +435,20 @@ nipc_shm_error_t nipc_shm_client_attach(const char *run_dir,
     if (path_rc < 0)
         return NIPC_SHM_ERR_PATH_TOO_LONG;
 
+    char shm_name[256];
+    int name_rc = build_shm_name(shm_name, sizeof(shm_name), service_name,
+                                 session_id);
+    if (name_rc == -2)
+        return NIPC_SHM_ERR_BAD_PARAM;
+    if (name_rc < 0)
+        return NIPC_SHM_ERR_PATH_TOO_LONG;
+
     /* Open the file. */
-    int fd = open(path, O_RDWR);
+    int dir_fd = open(run_dir, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+    if (dir_fd < 0)
+        return NIPC_SHM_ERR_OPEN;
+    int fd = openat(dir_fd, shm_name, O_RDWR | O_CLOEXEC);
+    close(dir_fd);
     if (fd < 0)
         return NIPC_SHM_ERR_OPEN;
 
@@ -830,7 +852,7 @@ void nipc_shm_cleanup_stale(const char *run_dir, const char *service_name)
         return;
 
     int dir_fd = dirfd(dir);
-    bool allow_stale_unlink = dir_fd >= 0 && run_dir_allows_stale_unlink(run_dir);
+    bool allow_stale_unlink = dir_fd >= 0 && dir_fd_allows_stale_unlink(dir_fd);
 
     struct dirent *ent;
     while ((ent = readdir(dir)) != NULL) {
@@ -852,7 +874,7 @@ void nipc_shm_cleanup_stale(const char *run_dir, const char *service_name)
 
         /* check_shm_stale unlinks stale files and returns:
          *   0 = stale (unlinked), +1 = live, -1 = gone, -2 = invalid (unlinked) */
-        check_shm_stale(path, dir_fd, ent->d_name, allow_stale_unlink);
+        check_shm_stale(dir_fd, ent->d_name, allow_stale_unlink);
     }
 
     closedir(dir);