Use descriptor-relative stale cleanup

Author: ktsaou

.agents/sow/done/SOW-0010-20260602-static-analysis-finding-cleanup.md

@@ -730,26 +730,28 @@ Evidence:
   stale cleanup `unlink()` calls in
   `src/libnetdata/netipc/src/transport/posix/netipc_shm.c` and
   `src/libnetdata/netipc/src/transport/posix/netipc_uds.c`.
-- The stale cleanup race cannot be fully eliminated with POSIX path unlink
-  semantics while preserving automatic stale socket/shared-memory cleanup. The
-  implemented product constraint is that automatic stale unlink only runs in a
-  process-owned run directory that is not group/world writable; the remaining
-  CodeQL finding is intentionally narrow and source-documented.
-- The previous inline `// codeql[cpp/toctou-race-condition]` comments did not
-  affect GitHub SARIF because the CodeQL configuration did not include the
-  C/C++ `AlertSuppression.ql` query.
+- The previous full-path stale cleanup used `lstat()` before `unlink()`. Even
+  with the process-owned private run directory requirement, CodeQL correctly
+  treats that pattern as a path check/use race.
+- A local Unix socket probe showed that connecting to a regular file at the
+  socket path returns `ECONNREFUSED`, so blindly unlinking after a failed
+  connect would be unsafe; the socket/file type and same-inode checks still
+  have to exist.
 
 Why previous validation missed it:
 
 - The local environment does not have the CodeQL CLI installed, so the only
   authoritative CodeQL validation point is the GitHub run after push.
 - The prior local validation proved the code built and local tools were clean,
-  but it did not prove CodeQL SARIF suppression handling.
+  but the remote CodeQL query source showed it specifically models full-path
+  `lstat()` guarding full-path `unlink()`.
 
 Repair plan:
 
-- Keep the CodeQL rule enabled globally and add the C/C++ alert suppression
-  query so the two reviewed stale-unlink suppressions are represented in SARIF.
+- Keep the CodeQL rule enabled globally and remove the full-path
+  `lstat()`/`unlink()` stale cleanup pattern. POSIX stale cleanup now opens the
+  validated private run directory and performs descriptor-relative
+  `fstatat()`/`unlinkat()` on generated entry names.
 - Compile the remaining addition-overflow guards only on 32-bit `size_t`
   platforms, where they are real portability checks, so the 64-bit CodeQL build
   no longer sees constant-false comparisons.
@@ -763,8 +765,8 @@ Validation:
   passed.
 - `cmake --build build-static --parallel --target netipc_protocol netipc_uds netipc_shm netipc_service`
   passed.
-- `clang-tidy -p build-static` on the C library sources passed with the
-  repository's existing warning-only baseline.
+- `clang-tidy -p build-static` on the changed POSIX transport sources passed
+  with the repository's existing warning-only baseline.
 - The first attempted `cppcheck --project=build-static/compile_commands.json`
   validation failed because it scanned the whole compile database and surfaced
   existing test/benchmark findings unrelated to this SOW; that is not the
@@ -778,6 +780,11 @@ Validation:
   exited 0, and the SARIF result count was 0.
 - `codacy-analysis analyze . --install-dependencies --output-format json --output /tmp/plugin-ipc-codacy-final.json --parallel-tools 2 --tool-timeout 900000`
   exited 0 with 0 issues and 0 tool errors.
+- After the descriptor-relative stale cleanup change, `make`,
+  `cmake --build build-static --parallel --target netipc_protocol netipc_uds netipc_shm netipc_service`,
+  focused `clang-tidy`, workflow-equivalent `cppcheck`,
+  `flawfinder --minlevel=5 --error-level=5 src/libnetdata/netipc tests`, and
+  `/usr/bin/ctest --test-dir build --output-on-failure` all passed again.
 
 Artifact updates:
 
@@ -787,8 +794,8 @@ Artifact updates:
   `project-*` skill.
 - Specs: no update needed; the stale cleanup behavior was already documented in
   the prior regression update.
-- End-user/operator docs: no update needed; this follow-up only makes CodeQL
-  suppression handling explicit and preserves the already documented behavior.
+- End-user/operator docs: no update needed; this follow-up preserves the
+  already documented private runtime directory behavior.
 - End-user/operator skills: no update needed; the public integrator skill was
   already updated for the private runtime directory requirement.
 - SOW lifecycle: this residual remote CodeQL regression was appended after the

.github/codeql.yml

@@ -6,10 +6,6 @@ queries:
   - uses: security-extended
   - uses: security-and-quality
 
-packs:
-  c-cpp:
-    - codeql/cpp-queries:AlertSuppression.ql
-
 query-filters:
   # Current baseline rule: CodeQL queries are enabled only when this branch has
   # no current alerts for them, or when the query is fixed in the same change.

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

@@ -55,16 +55,32 @@ static int validate_service_name(const char *name)
     return 0;
 }
 
+static int build_shm_name(char *dst, size_t dst_len,
+                          const char *service_name,
+                          uint64_t session_id)
+{
+    if (validate_service_name(service_name) < 0)
+        return -2; /* invalid service name */
+
+    int n = snprintf(dst, dst_len, "%s-%016llx.ipcshm",
+                     service_name, (unsigned long long)session_id);
+    if (n < 0 || (size_t)n >= dst_len)
+        return -1;
+    return 0;
+}
+
 /* Build per-session SHM file path: {run_dir}/{service_name}-{session_id:016x}.ipcshm */
 static int build_shm_path(char *dst, size_t dst_len,
                            const char *run_dir, const char *service_name,
                            uint64_t session_id)
 {
-    if (validate_service_name(service_name) < 0)
-        return -2; /* invalid service name */
+    char shm_name[256];
+    int name_rc = build_shm_name(shm_name, sizeof(shm_name), service_name,
+                                 session_id);
+    if (name_rc < 0)
+        return name_rc;
 
-    int n = snprintf(dst, dst_len, "%s/%s-%016llx.ipcshm",
-                     run_dir, service_name, (unsigned long long)session_id);
+    int n = snprintf(dst, dst_len, "%s/%s", run_dir, shm_name);
     if (n < 0 || (size_t)n >= dst_len)
         return -1;
     return 0;
@@ -153,28 +169,28 @@ static inline uint32_t *shm_u32_ptr(void *base, int offset)
  *  -1  = doesn't exist
  *  -2  = exists but undersized / invalid (treated as stale, unlinked)
  */
-static int unlink_same_file(const char *path, const struct stat *expected,
+static int unlink_same_file(int dir_fd, const char *name, const struct stat *expected,
                             bool allow_stale_unlink)
 {
     if (!allow_stale_unlink)
         return -1;
 
     struct stat current;
-    if (lstat(path, &current) != 0)
+    if (fstatat(dir_fd, name, &current, AT_SYMLINK_NOFOLLOW) != 0)
         return (errno == ENOENT) ? 0 : -1;
 
     if (current.st_dev != expected->st_dev || current.st_ino != expected->st_ino)
         return -1;
 
-    /* Stale recovery only unlinks same-inode files in a private run directory. */
-    // codeql[cpp/toctou-race-condition]
-    if (unlink(path) == 0 || errno == ENOENT)
+    /* Stale recovery only unlinks same-inode entries in a private run directory. */
+    if (unlinkat(dir_fd, name, 0) == 0 || errno == ENOENT)
         return 0;
 
     return -1;
 }
 
-static int check_shm_stale(const char *path, bool allow_stale_unlink)
+static int check_shm_stale(const char *path, int dir_fd, const char *name,
+                           bool allow_stale_unlink)
 {
 #ifdef O_NOFOLLOW
     int fd = open(path, O_RDONLY | O_NOFOLLOW);
@@ -203,21 +219,21 @@ static int check_shm_stale(const char *path, bool allow_stale_unlink)
     /* Must be at least header-sized to inspect. */
     if (st.st_size < (off_t)NIPC_SHM_HEADER_LEN) {
         close(fd);
-        return (unlink_same_file(path, &st, allow_stale_unlink) == 0) ? -2 : 1;
+        return (unlink_same_file(dir_fd, name, &st, allow_stale_unlink) == 0) ? -2 : 1;
     }
 
     void *map = mmap(NULL, NIPC_SHM_HEADER_LEN, PROT_READ, MAP_SHARED, fd, 0);
     close(fd);
     if (map == MAP_FAILED) {
-        return (unlink_same_file(path, &st, allow_stale_unlink) == 0) ? -2 : 1;
+        return (unlink_same_file(dir_fd, name, &st, allow_stale_unlink) == 0) ? -2 : 1;
     }
 
     const nipc_shm_region_header_t *hdr = (const nipc_shm_region_header_t *)map;
 
     /* Validate magic first. */
     if (hdr->magic != NIPC_SHM_REGION_MAGIC) {
         munmap(map, NIPC_SHM_HEADER_LEN);
-        return (unlink_same_file(path, &st, allow_stale_unlink) == 0) ? -2 : 1;
+        return (unlink_same_file(dir_fd, name, &st, allow_stale_unlink) == 0) ? -2 : 1;
     }
 
     int32_t owner = hdr->owner_pid;
@@ -229,7 +245,7 @@ static int check_shm_stale(const char *path, bool allow_stale_unlink)
     }
 
     /* Dead owner or zero generation (uninitialized/legacy) — stale */
-    return (unlink_same_file(path, &st, allow_stale_unlink) == 0) ? 0 : 1;
+    return (unlink_same_file(dir_fd, name, &st, allow_stale_unlink) == 0) ? 0 : 1;
 }
 
 /* ------------------------------------------------------------------ */
@@ -258,6 +274,14 @@ nipc_shm_error_t nipc_shm_server_create(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;
+
     /* Round capacities up to alignment. */
     req_capacity  = align64(req_capacity);
     resp_capacity = align64(resp_capacity);
@@ -277,7 +301,15 @@ nipc_shm_error_t nipc_shm_server_create(const char *run_dir,
     /* 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 stale = check_shm_stale(path, allow_stale_unlink);
+        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)
+            close(dir_fd);
         if (stale == 1)
             return NIPC_SHM_ERR_ADDR_IN_USE;
         /* Stale file was unlinked, retry create */
@@ -797,7 +829,8 @@ void nipc_shm_cleanup_stale(const char *run_dir, const char *service_name)
     if (!dir)
         return;
 
-    bool allow_stale_unlink = run_dir_allows_stale_unlink(run_dir);
+    int dir_fd = dirfd(dir);
+    bool allow_stale_unlink = dir_fd >= 0 && run_dir_allows_stale_unlink(run_dir);
 
     struct dirent *ent;
     while ((ent = readdir(dir)) != NULL) {
@@ -819,7 +852,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, allow_stale_unlink);
+        check_shm_stale(path, dir_fd, ent->d_name, allow_stale_unlink);
     }
 
     closedir(dir);

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

@@ -62,14 +62,27 @@ static int validate_service_name(const char *name)
     return 0;
 }
 
+static int build_socket_name(char *dst, size_t dst_len, const char *service_name)
+{
+    if (validate_service_name(service_name) < 0)
+        return -2; /* invalid service name */
+
+    int n = snprintf(dst, dst_len, "%s.sock", service_name);
+    if (n < 0 || (size_t)n >= dst_len)
+        return -1;
+    return 0;
+}
+
 /* Build socket path into dst, return 0 on success, -1 if too long. */
 static int build_socket_path(char *dst, size_t dst_len,
                              const char *run_dir, const char *service_name)
 {
-    if (validate_service_name(service_name) < 0)
-        return -2; /* invalid service name */
+    char socket_name[sizeof(((struct sockaddr_un *)0)->sun_path)];
+    int name_rc = build_socket_name(socket_name, sizeof(socket_name), service_name);
+    if (name_rc < 0)
+        return name_rc;
 
-    int n = snprintf(dst, dst_len, "%s/%s.sock", run_dir, service_name);
+    int n = snprintf(dst, dst_len, "%s/%s", run_dir, socket_name);
     if (n < 0 || (size_t)n >= dst_len)
         return -1; /* path too long */
     return 0;
@@ -506,28 +519,42 @@ static nipc_uds_error_t server_handshake(int fd,
 /*  Stale endpoint recovery                                            */
 /* ------------------------------------------------------------------ */
 
-static int unlink_stale_socket_path(const char *path, bool allow_stale_unlink)
+static int unlink_stale_socket_path(const char *run_dir,
+                                    const char *socket_name,
+                                    bool allow_stale_unlink)
 {
     if (!allow_stale_unlink)
         return -1;
 
+    int dir_fd = open(run_dir, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+    if (dir_fd < 0)
+        return -1;
+
     struct stat st;
-    if (lstat(path, &st) != 0)
-        return (errno == ENOENT) ? 0 : -1;
+    if (fstatat(dir_fd, socket_name, &st, AT_SYMLINK_NOFOLLOW) != 0) {
+        int ret = (errno == ENOENT) ? 0 : -1;
+        close(dir_fd);
+        return ret;
+    }
 
-    if (!S_ISSOCK(st.st_mode))
+    if (!S_ISSOCK(st.st_mode)) {
+        close(dir_fd);
         return -1;
+    }
 
-    /* Stale recovery only unlinks entries in a private run directory. */
-    // codeql[cpp/toctou-race-condition]
-    if (unlink(path) == 0 || errno == ENOENT)
-        return 0;
+    int ret = -1;
+    if (unlinkat(dir_fd, socket_name, 0) == 0 || errno == ENOENT)
+        ret = 0;
 
-    return -1;
+    close(dir_fd);
+    return ret;
 }
 
 /* Returns: 0 = stale (unlinked), 1 = live server, -1 = doesn't exist */
-static int check_and_recover_stale(const char *path, bool allow_stale_unlink)
+static int check_and_recover_stale(const char *run_dir,
+                                   const char *socket_name,
+                                   const char *path,
+                                   bool allow_stale_unlink)
 {
     /* Try connecting to check if a live server is there */
     int probe = socket(AF_UNIX, SOCK_SEQPACKET, 0);
@@ -548,11 +575,12 @@ static int check_and_recover_stale(const char *path, bool allow_stale_unlink)
         int saved_errno = errno;
         close(probe);
         /* Only ECONNREFUSED proves a stale socket path. Other errors
-         * (including regular files at the path) must not remove anything. */
+         * must not remove anything. */
         if (saved_errno == ENOENT) {
             ret = -1;
         } else if (saved_errno == ECONNREFUSED) {
-            ret = (unlink_stale_socket_path(path, allow_stale_unlink) == 0) ? 0 : 1;
+            ret = (unlink_stale_socket_path(run_dir, socket_name,
+                                            allow_stale_unlink) == 0) ? 0 : 1;
         } else {
             /* Can't determine ownership — treat as live to prevent overwriting */
             ret = 1;
@@ -581,9 +609,17 @@ nipc_uds_error_t nipc_uds_listen(const char *run_dir,
     if (path_rc < 0)
         return NIPC_UDS_ERR_PATH_TOO_LONG;
 
+    char socket_name[sizeof(((struct sockaddr_un *)0)->sun_path)];
+    int name_rc = build_socket_name(socket_name, sizeof(socket_name), service_name);
+    if (name_rc == -2)
+        return NIPC_UDS_ERR_BAD_PARAM;
+    if (name_rc < 0)
+        return NIPC_UDS_ERR_PATH_TOO_LONG;
+
     /* Stale recovery */
     bool allow_stale_unlink = run_dir_allows_stale_unlink(run_dir);
-    int stale = check_and_recover_stale(path, allow_stale_unlink);
+    int stale = check_and_recover_stale(run_dir, socket_name, path,
+                                        allow_stale_unlink);
     if (stale == 1)
         return NIPC_UDS_ERR_ADDR_IN_USE;