Harden NetIPC memory-safety edge cases

Author: ktsaou

.agents/sow/done/SOW-0026-20260628-netipc-memory-safety-scout-findings.md

@@ -0,0 +1,217 @@
+# SOW-0027 - Netdata Vendor Memory-Safety Update
+
+## Status
+
+Status: open
+
+Sub-state: not started; tracks propagation of SOW-0026 source fixes to the Netdata vendored copy.
+
+## Requirements
+
+### Purpose
+
+Keep NetIPC memory-safety fixes source-owned in `plugin-ipc` while ensuring the Netdata vendored copy receives those fixes through the normal vendor/update path.
+
+### User Request
+
+The user asked to fix NetIPC library issues in `plugin-ipc`, not directly in the Netdata PR, because this repository is the source of truth.
+
+### Assistant Understanding
+
+Facts:
+
+- SOW-0026 implements source fixes for NetIPC memory-safety scout findings in `plugin-ipc`.
+- Netdata consumes NetIPC through a vendored copy.
+- Directly patching Netdata's vendored NetIPC copy would create source-of-truth drift.
+
+Inferences:
+
+- After SOW-0026 lands, the next safe step is a focused vendor/update pass against the relevant Netdata checkout.
+
+Unknowns:
+
+- The exact Netdata checkout, branch, and PR target for propagation must be confirmed before implementation starts.
+
+### Acceptance Criteria
+
+- The selected Netdata checkout is confirmed before implementation.
+- The vendored NetIPC copy is updated from `plugin-ipc` rather than manually patched.
+- The project-local vendor diff/checker is run and its result is recorded.
+- Netdata build or targeted tests covering the touched NetIPC integration paths are run or a blocker is recorded with evidence.
+- No unrelated Netdata changes are included.
+
+## Analysis
+
+Sources checked:
+
+- SOW-0026 source-ownership decision.
+- `docs/netipc-integrator-skill.md` source-of-truth guidance.
+- Prior vendor synchronization SOWs in `.agents/sow/done/`.
+
+Current state:
+
+- Source fixes are expected to land in `plugin-ipc` through SOW-0026 first.
+- No Netdata checkout has been selected for this SOW yet.
+
+Risks:
+
+- Copying files manually can introduce import-path or layout mistakes.
+- Updating the wrong Netdata checkout can create unrelated branch drift.
+- Skipping the vendor diff can hide missing language-specific updates.
+
+## Pre-Implementation Gate
+
+Status: needs-user-decision
+
+Problem / root-cause model:
+
+- NetIPC fixes must be propagated to the downstream Netdata vendored copy, but that work belongs in a separate focused pass after the source repository is committed and pushed.
+
+Evidence reviewed:
+
+- SOW-0026 records that NetIPC source ownership belongs to `plugin-ipc`.
+- Historical vendor-sync SOWs use the project-local `diff-netdata-vendor.sh` checker.
+
+Affected contracts and surfaces:
+
+- Netdata vendored C, Rust, and Go NetIPC sources.
+- Netdata build/test paths that consume NetIPC.
+- Vendor synchronization evidence in this SOW.
+
+Existing patterns to reuse:
+
+- `diff-netdata-vendor.sh`
+- Prior SOW-0003 and SOW-0008 vendor synchronization flow.
+
+Risk and blast radius:
+
+- Medium: changes land in a consumer repository and may affect Netdata build/test behavior.
+- Keep scope limited to NetIPC vendor propagation and required validation.
+
+Sensitive data handling plan:
+
+- No secrets, customer data, credentials, production logs, or private endpoints are required.
+- Evidence will use source paths, commands, commit hashes, and sanitized summaries only.
+
+Implementation plan:
+
+1. Confirm the target Netdata checkout and branch.
+2. Propagate the committed `plugin-ipc` source changes through the normal vendor/update path.
+3. Run the vendor diff/checker and targeted Netdata validation.
+4. Commit only the vendor update and required tracking artifacts.
+
+Validation plan:
+
+- Run the vendor diff/checker against the selected Netdata checkout.
+- Run targeted Netdata build/tests for touched C/Rust/Go NetIPC integration paths.
+- Run same-failure searches for the SOW-0026 finding classes in the Netdata vendored copy.
+
+Artifact impact plan:
+
+- AGENTS.md: no expected update.
+- Runtime project skills: no expected update.
+- Specs: no expected update unless propagation exposes source/doc drift.
+- End-user/operator docs: no expected update.
+- End-user/operator skills: no expected update.
+- SOW lifecycle: this SOW remains open until the user selects the Netdata checkout.
+
+Open-source reference evidence:
+
+- None checked yet; this SOW is a local vendor propagation task.
+
+Open decisions:
+
+1. Select the target Netdata checkout, branch, and PR context before implementation.
+
+## Implications And Decisions
+
+- No implementation decisions have been made yet.
+
+## Plan
+
+1. Confirm target checkout and branch.
+2. Run vendor propagation.
+3. Validate vendor parity and targeted Netdata behavior.
+4. Commit and push when validated.
+
+## Execution Log
+
+### 2026-06-29
+
+- Created as the tracked follow-up for SOW-0026 vendor propagation.
+- No implementation started.
+
+## Validation
+
+Acceptance criteria evidence:
+
+- Not started.
+
+Tests or equivalent validation:
+
+- Not started.
+
+Real-use evidence:
+
+- Not started.
+
+Reviewer findings:
+
+- Not started.
+
+Same-failure scan:
+
+- Not started.
+
+Sensitive data gate:
+
+- The SOW contains only source paths and workflow descriptions. No secrets or customer data are included.
+
+Artifact maintenance gate:
+
+- AGENTS.md: no update needed for this tracking SOW.
+- Runtime project skills: none exist.
+- Specs: no update needed until implementation changes behavior.
+- End-user/operator docs: no update needed until implementation changes behavior.
+- End-user/operator skills: no update needed until implementation changes behavior.
+- SOW lifecycle: created as open in `.agents/sow/pending/`.
+
+Specs update:
+
+- Not started.
+
+Project skills update:
+
+- Not started.
+
+End-user/operator docs update:
+
+- Not started.
+
+End-user/operator skills update:
+
+- Not started.
+
+Lessons:
+
+- None yet.
+
+Follow-up mapping:
+
+- This SOW tracks the SOW-0026 Netdata vendor propagation item.
+
+## Outcome
+
+Pending.
+
+## Lessons Extracted
+
+Pending.
+
+## Followup
+
+None yet.
+
+## Regression Log
+
+None yet.

.agents/sow/pending/SOW-0027-20260629-netdata-vendor-memory-safety-update.md

@@ -35,6 +35,9 @@ fallback.
    If a stale socket file exists from a dead process, the server
    unlinks it and recreates. If the socket is actively held by a live
    process, the server fails with address-in-use.
+   If two servers start concurrently for the same service name, one may
+   lose the probe/bind race and fail with address-in-use. The contract is
+   one owner per service endpoint.
 
 2. **Client**: connects to the socket path. The OS delivers a connected
    SEQPACKET file descriptor.

docs/level1-posix-uds.md

@@ -527,6 +527,18 @@ mandatory capability, not an optional enhancement.
   Level 1. Level 2 managed server mode provides an opinionated threading
   model on top.
 
+## Raw session thread ownership
+
+Level 1 raw session objects are mutable transport state. A single session
+object is single-owner unless the caller serializes access externally.
+
+- Multiple clients may use independent sessions concurrently.
+- A listener may accept multiple independent sessions concurrently.
+- Concurrent send/receive/close calls on the same raw session object require
+  caller-provided synchronization.
+- Code that needs a library-owned concurrency model should use the Level 2
+  managed server or typed client APIs instead of sharing raw Level 1 sessions.
+
 ## Stale endpoint recovery
 
 Service endpoints (socket files, named pipe names, SHM regions) may become
@@ -547,6 +559,10 @@ stale if a server process crashes without cleanup. Level 1 handles this:
 - `run_dir` is expected to be the embedding service's private runtime
   directory (e.g. netdata's run dir); its permissions do not gate stale
   recovery.
+- Concurrent startup of two servers for the same POSIX socket can still race
+  between stale-file probe and bind. Level 1 treats this as an address-in-use
+  startup conflict, not as a steady-state safety issue. Operators should run one
+  owner for a service endpoint.
 
 ## Testing requirements
 

docs/level1-transport.md

@@ -143,12 +143,14 @@ Uses two named kernel events (`req_event`, `resp_event`) created via
    once more (avoid race), then `WaitForSingleObject(req_event,
    timeout)`.
 3. Clear `req_server_waiting` after waking.
-4. Read `req_len`. If `req_len` is 0, report a protocol error — `send`
-   rejects zero-length messages, so this indicates SHM corruption.
-5. Validate `req_len` against `request_capacity`. If `req_len` exceeds
-   the capacity, discard the message and report an error. This prevents
+4. Once `req_seq` has advanced, read `req_len`, copy the message bytes if
+   they fit the caller buffer, then re-read both `req_seq` and `req_len`.
+   If either value changed during the copy, report a protocol error.
+5. If `req_len` is 0, report a protocol error — `send` rejects zero-length
+   messages, so this indicates SHM corruption.
+6. Validate `req_len` against `request_capacity`. If `req_len` exceeds the
+   capacity, discard the message and report an error. This prevents
    out-of-bounds reads from a malicious or buggy peer.
-6. Read the message bytes from the request area.
 
 #### Server sends a response
 
@@ -163,11 +165,12 @@ Uses two named kernel events (`req_event`, `resp_event`) created via
 2. If not advanced: set `resp_client_waiting = 1`, check `resp_seq`
    once more, then `WaitForSingleObject(resp_event, timeout)`.
 3. Clear `resp_client_waiting` after waking.
-4. Read `resp_len`. If `resp_len` is 0, report a protocol error
-   (SHM corruption).
-5. Validate `resp_len` against `response_capacity`. If `resp_len`
-   exceeds the capacity, discard the message and report an error.
-6. Read the message bytes from the response area.
+4. Once `resp_seq` has advanced, read `resp_len`, copy the message bytes if
+   they fit the caller buffer, then re-read both `resp_seq` and `resp_len`.
+   If either value changed during the copy, report a protocol error.
+5. If `resp_len` is 0, report a protocol error (SHM corruption).
+6. Validate `resp_len` against `response_capacity`. If `resp_len` exceeds
+   the capacity, discard the message and report an error.
 
 ### SHM_BUSYWAIT (pure spin, no kernel events)
 
@@ -177,12 +180,12 @@ sequence numbers.
 - The publication protocol is the same as SHM_HYBRID except:
   - No `SetEvent` calls.
   - No `WaitForSingleObject` calls.
-  - The waiter spins indefinitely (with deadline polling every
-    `BUSYWAIT_DEADLINE_POLL_MASK + 1` iterations to check for timeout
-    expiry).
+  - The waiter spins indefinitely, polling every
+    `BUSYWAIT_DEADLINE_POLL_MASK + 1` iterations to check timeout/peer-close
+    state and yield the current thread.
 
-This profile burns full CPU on both client and server but achieves the
-lowest possible latency.
+This profile is CPU-intensive on both client and server, but the implementation
+must periodically yield so an infinite wait does not monopolize a core.
 
 ## Close protocol
 

docs/level1-windows-shm.md

@@ -157,6 +157,7 @@ Non-negotiable rule:
   - retries / reconnects
   - raw payload encode/decode
   - batch and chunk rules
+  - single-owner or externally locked access to each raw session object
 
 Use L1 for:
 

docs/netipc-integrator-skill.md

@@ -44,7 +44,7 @@ pub fn string_reverse_decode(buf: &[u8]) -> Result<StringReverseView<'_>, NipcEr
         return Err(NipcError::Truncated);
     }
     let str_offset = u32::from_ne_bytes(buf[0..4].try_into().unwrap()) as usize;
-    if str_offset < STRING_REVERSE_HDR_SIZE {
+    if str_offset != STRING_REVERSE_HDR_SIZE {
         return Err(NipcError::BadLayout);
     }
     let str_length = u32::from_ne_bytes(buf[4..8].try_into().unwrap()) as usize;
@@ -145,6 +145,19 @@ mod tests {
         ));
     }
 
+    #[test]
+    fn decode_bad_offset() {
+        let mut buf = [0u8; 16];
+        buf[0..4].copy_from_slice(&9u32.to_ne_bytes());
+        buf[4..8].copy_from_slice(&1u32.to_ne_bytes());
+        buf[9] = b'x';
+        buf[10] = 0;
+        assert!(matches!(
+            string_reverse_decode(&buf),
+            Err(NipcError::BadLayout)
+        ));
+    }
+
     #[test]
     fn dispatch_resp_too_small() {
         let s = b"hello";