Resolve restored scanner findings

Author: ktsaou

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

@@ -4,8 +4,8 @@
 
 Status: completed
 
-Sub-state: Completed after restoring the approved hygiene checks and validating
-the scanner/test matrix.
+Sub-state: Completed after fixing the remaining restored-rule GitHub Code
+Scanning findings and validating the scanner/test matrix.
 
 ## Requirements
 
@@ -614,3 +614,98 @@ Artifact updates:
   because public integration guidance did not change.
 - SOW lifecycle: this reopened regression is completed and the SOW will be
   moved back to `done/` in the same commit as the restored scanner changes.
+
+## Regression - 2026-06-03 Remote Alerts After Hygiene Restoration
+
+What broke:
+
+- The restored CodeQL and OSV rules were useful and found remaining real
+  hygiene/security issues after commit
+  `1b7ce780b7c4c54902e1ff0e957aad1542fe3733`.
+
+Evidence:
+
+- Codacy Cloud and local Codacy are clean for the restored commit, so the
+  remaining backlog is GitHub Code Scanning, not Codacy local configuration.
+- GitHub Code Scanning reported 32 open alerts after the restored-rule commit:
+  one Go integer-conversion alert in `bench/drivers/go/main.go`, nine Go
+  standard-library OSV alerts across the three Go modules, two C unused-code
+  alerts, three C unused-local alerts, thirteen C constant-comparison alerts,
+  two C TOCTOU alerts, one Go unchecked writable-close test alert, and one Go
+  useless-assignment test alert.
+- Official Go release history states that `go1.25.11` and `go1.26.4`, both
+  released on 2026-06-02, include security fixes for `crypto/x509`, `mime`,
+  and `net/textproto`; the repository Go module directives still declare
+  `go 1.25.10`.
+
+Why previous validation missed it:
+
+- Local `govulncheck` used the workstation Go runtime, currently `go1.26.3-X`,
+  while GitHub OSV scans the module `go` directive and therefore still sees
+  `go 1.25.10` as vulnerable.
+- The restored CodeQL rules only re-ran remotely after the follow-up commit was
+  pushed.
+
+Repair plan:
+
+- Update all Go module directives from `go 1.25.10` to the patched supported
+  Go line.
+- Fix the benchmark sample-count conversion by keeping the arithmetic bounded
+  before converting to `int`.
+- Check writable file close errors in the SHM edge test and remove the useless
+  UDS test assignment.
+- Remove unused C helpers and local variables.
+- Rewrite redundant overflow checks so the code preserves real guards without
+  constant comparisons.
+- Resolve the stale socket/shared-memory cleanup TOCTOU alerts with a
+  code-level security decision that is narrow and documented in code.
+
+Validation:
+
+- `cd src/go && go test ./... && go vet ./... && "$(go env GOPATH)/bin/govulncheck" ./... && "$(go env GOPATH)/bin/staticcheck" ./... && "$(go env GOPATH)/bin/gosec" -quiet -fmt json -out /tmp/plugin-ipc-gosec-src-go-followup.json -exclude=G404 ./...`
+  passed with no Go vulnerabilities or gosec findings.
+- `cd tests/fixtures/go && go test ./... && go vet ./... && "$(go env GOPATH)/bin/govulncheck" ./... && "$(go env GOPATH)/bin/staticcheck" ./... && "$(go env GOPATH)/bin/gosec" -quiet -fmt json -out /tmp/plugin-ipc-gosec-fixtures-go-followup.json -exclude=G404 ./...`
+  passed with no Go vulnerabilities or gosec findings.
+- `cd bench/drivers/go && go test ./... && go vet ./... && "$(go env GOPATH)/bin/govulncheck" ./... && "$(go env GOPATH)/bin/staticcheck" ./... && "$(go env GOPATH)/bin/gosec" -quiet -fmt json -out /tmp/plugin-ipc-gosec-bench-go-followup.json -exclude=G404 ./...`
+  passed with no Go vulnerabilities or gosec findings.
+- `cargo test --manifest-path src/crates/netipc/Cargo.toml --all-targets --all-features --no-run && cargo clippy --manifest-path src/crates/netipc/Cargo.toml --all-targets --all-features -- -D clippy::correctness -D clippy::suspicious`
+  passed. Clippy emitted existing warning-only hygiene output outside the hard
+  correctness/suspicious gate.
+- `cargo audit && cargo deny check advisories bans sources` passed in
+  `src/crates/netipc`.
+- `make` passed and rebuilt C, Rust, Go fixtures, and benchmark drivers.
+- `/usr/bin/ctest --test-dir build --output-on-failure` first passed 45/46 and
+  had one `go_FuzzDecodeCgroupsLookupRequest` timeout flake. The exact target
+  passed on rerun, and a second full `/usr/bin/ctest --test-dir build --output-on-failure`
+  run passed all 46 tests.
+- `actionlint` passed.
+- The local C static workflow commands passed: configure `build-static`, build
+  `netipc_protocol`, `netipc_uds`, `netipc_shm`, and `netipc_service`, then run
+  `clang-tidy`, `cppcheck`, and `flawfinder --minlevel=5 --error-level=5`.
+- After the final SHM cleanup adjustment, `make` passed again and a focused
+  `clang-tidy` plus `cppcheck` pass on `netipc_shm.c` exited 0.
+- `osv-scanner scan --recursive --format sarif --output-file /tmp/plugin-ipc-osv-followup.sarif .`
+  exited 0, and the SARIF result count was 0.
+- `codacy-analysis analyze . --install-dependencies --output-format json --output /tmp/plugin-ipc-codacy-followup.json --parallel-tools 2 --tool-timeout 900000`
+  exited 0 with 0 issues and 0 tool errors.
+- `codacy-analysis analyze . --install-dependencies --output-format sarif --output /tmp/plugin-ipc-codacy-followup.sarif --parallel-tools 2 --tool-timeout 900000`
+  exited 0 and generated SARIF with 0 results.
+- `bash .agents/sow/audit.sh` passed.
+- `git diff --check && git diff --cached --check` passed.
+- `git check-ignore -v .env` confirmed `.env` is ignored by `.gitignore`.
+
+Artifact updates:
+
+- AGENTS.md: no update needed; existing project validation commands and SOW
+  rules remain accurate.
+- Runtime project skills: no update needed; the repository still has no runtime
+  `project-*` skill, and no reusable repo workflow was missing from AGENTS.md.
+- Specs: updated `docs/level1-transport.md` and `docs/level1-posix-shm.md` to
+  document the POSIX private-runtime-directory rule for automatic stale unlink.
+- End-user/operator docs: updated the public transport docs listed above.
+- End-user/operator skills: updated `docs/netipc-integrator-skill.md` so
+  downstream integrators keep provider runtime directories private enough for
+  stale cleanup.
+- SOW lifecycle: this regression was appended after the prior SOW content; the
+  SOW is marked `completed` and will be moved back to `done/` in the same
+  commit as the implementation and docs.

bench/drivers/go/go.mod

@@ -1,6 +1,6 @@
 module github.com/netdata/plugin-ipc/bench/drivers/go
 
-go 1.25.10
+go 1.25.11
 
 require github.com/netdata/plugin-ipc/go v0.0.0
 

bench/drivers/go/main.go

@@ -77,10 +77,6 @@ func cpuNS() uint64 {
 	return sec*1_000_000_000 + nsec
 }
 
-func maxIntValue() int {
-	return int(^uint(0) >> 1)
-}
-
 func u32FromNonNegativeInt(value int) (uint32, bool) {
 	if value < 0 || uint64(value) > uint64(^uint32(0)) {
 		return 0, false
@@ -110,15 +106,12 @@ func estimateSamples(defaultSamples int, targetRPS uint64, durationSec int) int
 	if targetRPS == 0 || durationSec <= 0 {
 		return defaultSamples
 	}
-	limit := uint64(maxIntValue() / durationSec) // #nosec G115 -- maxIntValue and durationSec are positive here.
-	if targetRPS > limit {
-		return maxLatencySamples
-	}
-	samples := int(targetRPS) * durationSec // #nosec G115 -- targetRPS is bounded by limit above.
-	if samples > maxLatencySamples {
+	duration := uint64(durationSec) // #nosec G115 -- durationSec is checked positive above.
+	if targetRPS > uint64(maxLatencySamples)/duration {
 		return maxLatencySamples
 	}
-	return samples
+	samples := targetRPS * duration
+	return int(samples) // #nosec G115 -- samples is bounded by maxLatencySamples above.
 }
 
 func randomBatchSize() uint32 {

docs/level1-posix-shm.md

@@ -246,15 +246,20 @@ When a session closes (graceful or broken):
 Both `server_create` (per-session) and `cleanup_stale` (startup scan)
 use the same stale detection logic:
 
-1. Open the file and mmap the header. If `open()` fails with a
+1. Verify `{run_dir}` is safe for automatic stale unlink: owned by the
+   effective process user and not group- or world-writable. If the directory is
+   unsafe, stale entries are left in place and treated as in-use.
+2. Open the file and mmap the header. If `open()` fails with a
    permission error (EACCES, EPERM), the file is left in place — it
    may belong to another user or process.
-2. Validate magic. If invalid or file is undersized: stale — unlink.
-3. Check `owner_pid` and `owner_generation`:
+3. Validate magic. If invalid or file is undersized: stale — unlink only when
+   `{run_dir}` passed the safety check.
+4. Check `owner_pid` and `owner_generation`:
    - If `owner_pid` is alive AND `owner_generation` is non-zero:
      the region is live — leave it.
    - Otherwise (PID dead, or generation is zero indicating an
-     uninitialized/legacy region): stale — unlink.
+     uninitialized/legacy region): stale — unlink only when `{run_dir}` passed
+     the safety check.
 
 The `owner_generation` check catches PID reuse: a new process that
 reuses an old PID will not have the same generation value. A zero
@@ -268,7 +273,8 @@ behind by a previous server instance that crashed or was killed:
 
 1. Scan `{run_dir}` for files matching `{service_name}-*.ipcshm`.
 2. For each file: apply the stale detection logic above.
-3. Stale files are unlinked. Live files are left in place.
+3. Stale files are unlinked only in a safe `{run_dir}`. Live files and files in
+   unsafe shared directories are left in place.
 
 This cleanup runs once at server startup, before the listener begins
 accepting connections. It is the safety net for crashes and hard
@@ -278,5 +284,6 @@ reboots.
 
 When `server_create` is called for a new session, it applies the same
 stale detection logic to the target path before attempting `O_EXCL`
-create. If a stale file exists, it is unlinked first. If a live file
-exists, the create fails with address-in-use.
+create. If a stale file exists and `{run_dir}` is safe, it is unlinked first.
+If a live file exists, or `{run_dir}` is unsafe for automatic stale unlink, the
+create fails with address-in-use.

docs/level1-transport.md

@@ -531,6 +531,10 @@ stale if a server process crashes without cleanup. Level 1 handles this:
   it (unlinks and recreates).
 - On listen: if an active endpoint exists from a live process, Level 1 fails
   with an appropriate error (address already in use).
+- On POSIX: automatic stale unlink is allowed only when `run_dir` is owned by
+  the effective process user and is not group- or world-writable. In unsafe
+  shared directories, stale entries are treated as in-use so Level 1 does not
+  unlink paths that another local user could race or replace.
 - Stale detection uses process ownership metadata (PID and generation) to
   avoid false reclamation due to PID reuse.
 

docs/netipc-integrator-skill.md

@@ -273,6 +273,9 @@ Netdata-specific integration rules:
 - use `os_run_dir(true)` on the provider side
 - use `os_run_dir(false)` on the consumer side
 - treat these as a matched pair
+- keep the provider runtime directory service-owned and not group- or
+  world-writable; POSIX stale cleanup will not unlink old socket/SHM paths from
+  unsafe shared directories
 - if provider and consumer do not agree on auth token or run dir, the client
   will stay in `AUTH_FAILED` or `NOT_FOUND`
 

src/crates/netipc/src/transport/posix.rs

@@ -12,6 +12,7 @@ use crate::protocol::{
 use std::collections::HashSet;
 use std::ffi::CString;
 use std::io;
+use std::os::unix::fs::MetadataExt;
 use std::os::unix::io::RawFd;
 use std::path::{Path, PathBuf};
 use std::sync::atomic::{AtomicU64, Ordering};
@@ -593,7 +594,7 @@ impl UdsListener {
         let path = build_socket_path(run_dir, service_name)?;
 
         // Stale recovery
-        match check_and_recover_stale(&path) {
+        match check_and_recover_stale(&path, run_dir_allows_stale_unlink(run_dir)) {
             StaleResult::LiveServer => return Err(UdsError::AddrInUse),
             StaleResult::Stale | StaleResult::NotExist => { /* proceed */ }
         }
@@ -910,7 +911,17 @@ enum StaleResult {
     LiveServer,
 }
 
-fn check_and_recover_stale(path: &str) -> StaleResult {
+fn run_dir_allows_stale_unlink(run_dir: &str) -> bool {
+    let metadata = match std::fs::metadata(run_dir) {
+        Ok(m) => m,
+        Err(_) => return false,
+    };
+    metadata.is_dir()
+        && metadata.uid() == unsafe { libc::geteuid() }
+        && metadata.mode() & 0o022 == 0
+}
+
+fn check_and_recover_stale(path: &str, allow_stale_unlink: bool) -> StaleResult {
     if !Path::new(path).exists() {
         return StaleResult::NotExist;
     }
@@ -926,10 +937,17 @@ fn check_and_recover_stale(path: &str) -> StaleResult {
             // Connected => live server
             StaleResult::LiveServer
         }
-        Err(UdsError::Connect(e)) if e == libc::ECONNREFUSED || e == libc::ENOENT => {
-            // Connection refused or no such socket => stale, unlink
-            let _ = std::fs::remove_file(path);
-            StaleResult::Stale
+        Err(UdsError::Connect(e)) if e == libc::ENOENT => StaleResult::NotExist,
+        Err(UdsError::Connect(e)) if e == libc::ECONNREFUSED => {
+            if !allow_stale_unlink {
+                return StaleResult::LiveServer;
+            }
+            // Connection refused means stale; unlink only in a private run dir.
+            match std::fs::remove_file(path) {
+                Ok(()) => StaleResult::Stale,
+                Err(err) if err.kind() == io::ErrorKind::NotFound => StaleResult::NotExist,
+                Err(_) => StaleResult::LiveServer,
+            }
         }
         Err(_) => {
             // Other errors (EACCES, etc.) — can't determine ownership,