Fix 14 review findings from the audit-remediation cycle (C security/perf pass)

.github/workflows/ci.yml

@@ -148,23 +148,30 @@ jobs:
       # Delegate a cgroup v2 subtree to the runner user so the positive-path
       # cgroup tests (test/cgroup_test.exs) exercise the privileged branch.
       # The shepherd hardcodes /sys/fs/cgroup/<path> and the Elixir side only
-      # accepts paths under net_runner/, so no env var is needed — just a
-      # writable net_runner/ subtree. Root cgroup.procs must also be writable
-      # to migrate a process across the common ancestor (cgroup v2 delegation
-      # containment rule).
+      # accepts paths under net_runner/, so just a writable net_runner/
+      # subtree is needed. Root cgroup.procs must also be writable to migrate
+      # a process across the common ancestor (cgroup v2 delegation
+      # containment rule). NR_CGROUP_DELEGATED=1 turns the tests' degraded
+      # (fail-closed) branch into a hard failure on this leg, so a delegation
+      # regression cannot pass as a green build with zero cgroup coverage.
       - name: Delegate cgroup v2 subtree for cgroup tests
         if: runner.os == 'Linux'
         run: |
           sudo mkdir -p /sys/fs/cgroup/net_runner
           sudo chown -R "$(whoami)" /sys/fs/cgroup/net_runner
           sudo chown "$(whoami)" /sys/fs/cgroup/cgroup.procs
+          echo "NR_CGROUP_DELEGATED=1" >> "$GITHUB_ENV"
       - run: mix test
 
   test-alpine:
     name: Tests (Alpine / musl)
     runs-on: ubuntu-latest
     # Digest-pinned (alpine:3.22 at time of pinning); Dependabot does not scan
     # `container:` keys, so bump this digest manually when moving minors.
+    # No cgroup delegation here (the container has no writable cgroup v2
+    # tree): the cgroup tests run their fail-closed branch and log
+    # "[degraded]"; the Ubuntu leg above is the one that covers the positive
+    # path.
     container: alpine:3.22@sha256:14358309a308569c32bdc37e2e0e9694be33a9d99e68afb0f5ff33cc1f695dce
     env:
       WERROR: "1"

CHANGELOG.md

@@ -10,7 +10,8 @@ Performance follow-up cycle (from the 2026-08-31 `/phx:perf` reports; numbers
 are medians on an Apple M1 Max, `MIX_ENV=prod`, harness under `bench/`).
 
 Audit-remediation cycle (2026-08-31 project-health audit, second pass: all 39
-remaining findings across shepherd/NIF, lib, tests, docs, and CI).
+remaining findings across shepherd/NIF, lib, tests, docs, and CI), followed by
+the 2026-10-07 review of that cycle (14 findings, C security/perf audit).
 
 ### Security
 
@@ -20,18 +21,71 @@ remaining findings across shepherd/NIF, lib, tests, docs, and CI).
   `MSG_ERROR` instead of silently running the child unconfined. A
   pre-existing cgroup leaf directory is now fatal too (teardown would not be
   owned), and the directory is created `0700`.
+- **cgroup teardown requires `cgroup.kill`.** `cgroup_setup` probes the new
+  leaf's `cgroup.kill` (Linux ≥ 5.14) and fails the spawn closed when it is
+  not writable: without it the only kill vector is `kill(-pgid)`, which a
+  `setsid()` daemoniser escapes. The cleanup write is now checked and logged
+  instead of silently ignored.
 - **cgroup attach no longer races `execvp`.** The child blocks on a sync
   pipe after `fork` and only execs once the shepherd has migrated it into
   the cgroup — descendants can no longer escape limits or `cgroup.kill`
-  teardown. Non-cgroup spawns are byte-for-byte unchanged.
-- **Pid-reuse guard on direct kills.** `Nif.nif_kill` fallbacks in
-  `NetRunner.Process` and the Watcher probe only fire when the shepherd port
-  is dead; while the shepherd lives it holds the child as a zombie, making
-  it the only safe signaller.
+  teardown. The gate is not created for non-cgroup spawns; those pay only
+  the two `fcntl` calls that make the UDS nonblocking (see Fixed).
+- **Shepherd argv is `--`-terminated.** The BEAM emits `--` after its flags
+  so a command whose name starts with `-` (`"--cgroup-path"`,
+  `"--kill-timeout"`, `"--pty"`) can no longer be parsed as a shepherd option
+  and re-target the cgroup or stretch the kill ladder behind the API.
+- **Signal routing is probe-then-signal.** While the shepherd Port lives,
+  `CMD_KILL` over the UDS is the signaller (the shepherd holds the child as
+  a zombie, so the pid cannot be recycled). Once the Port is dead — or the
+  UDS send fails — `NetRunner.Process` probes `kill(pid, 0)` before
+  `nif_kill`. This narrows, but does not close, the window in which a
+  dropped `MSG_CHILD_EXITED` leaves a reaped pid looking live; the comments
+  now say so. The Watcher's port-liveness gate was removed: the Port is owned
+  by the GenServer whose death the Watcher handles, so it was always already
+  closed and the gate was a no-op.
 - **`nif_read` can no longer expose uninitialized heap.** The shrink of a
   >64 KiB read buffer now handles `enif_realloc_binary` failure with an
   alloc+copy fallback.
 
+### Fixed (review of the audit cycle)
+
+- **`kill/2` reports lost signals.** `send_shepherd_command` now returns
+  `:ok | {:error, reason}`; a `CMD_KILL` that cannot reach a live shepherd
+  falls back to the direct probe-and-kill, and `kill/2` returns
+  `{:error, :not_running}` when the probe finds no process instead of a
+  misleading `:ok`. `set_window_size/3` likewise surfaces a failed send.
+- **Bounded UDS waits everywhere in the shepherd.** `send_fds` shared the
+  `write_fully` design but restarted its 5 s window on every `EAGAIN`
+  wakeup; both now use one `wait_pollout` helper with a single absolute
+  deadline (lazily computed, so a first-try write never reads the clock) and
+  a hard poll cap when the monotonic clock is unavailable.
+- **Option validation really is uniform.** `NetRunner.Daemon.start_link/1`
+  validates `:process_opts` in the caller (via the new
+  `NetRunner.Process.validate_opts!/1`) instead of raising inside `init/1`
+  and exiting the linked caller; `:kill_timeout` is range-checked
+  (`1..60_000`) and `:cgroup_path` rejects NUL bytes and non-binaries, so a
+  bad value raises `ArgumentError` instead of a silent 10 s
+  `:shepherd_connect_timeout`.
+- **`NetRunner.Process.Protocol.kill/1`** rejects signal numbers outside
+  `1..255` instead of truncating them to one byte.
+- **macOS test flake.** Timeout tests built a `sleep` duration from
+  `System.unique_integer/1`, which crossed `INT32_MAX` late in a full run;
+  macOS `/bin/sleep` then exited 1 with a usage error. `sleep_marker/0` keeps
+  it in range.
+
+### Added (review of the audit cycle)
+
+- **`Stats.shepherd_error`** — a post-spawn `MSG_ERROR` from the shepherd is
+  now exposed via `NetRunner.Process.stats/1` rather than buried in server
+  state.
+- Tests: Watcher probe on an orphaned child, direct `kill/2` after shepherd
+  death, `Protocol` encoder byte layouts, post-spawn `MSG_ERROR` recording,
+  Daemon client-side validation, `:kill_timeout` validation, `--` terminator;
+  the PTY-resize test's sentinel no longer matches its own failure message.
+  On the delegated Linux CI leg `NR_CGROUP_DELEGATED=1` makes the cgroup
+  tests' fail-closed branch a hard failure.
+
 ### Fixed
 
 - **EINTR is retried** in the NIF `read(2)`/`write(2)` loops and shepherd

CLAUDE.md

@@ -7,7 +7,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co
 ```bash
 mix deps.get              # Fetch dependencies
 mix compile               # Compile Elixir + C code (via elixir_make)
-mix test                  # Run full test suite (47 tests)
+mix test                  # Run full test suite (~250 tests)
 mix test test/process_test.exs          # Run a single test file
 mix test test/process_test.exs:10       # Run a single test at line
 mix format                # Auto-format code
@@ -34,7 +34,7 @@ NetRunner is a safe OS process execution library for Elixir with NIF-based backp
 
 1. **Shepherd** — detects BEAM death (POLLHUP on UDS), kills child process group
 2. **Watcher GenServer** — monitors Process GenServer, kills OS process on DOWN
-3. **NIF destructor** — closes FDs on garbage collection
+3. **NIF owner monitor** — `io_resource_down` closes FDs when the owning process dies (the destructor only runs for resources with no live `enif_select`)
 
 ### Spawn Sequence
 
@@ -49,7 +49,8 @@ NetRunner is a safe OS process execution library for Elixir with NIF-based backp
 - `NetRunner` — top-level API (`run/2`, `stream!/2`, `stream/2`)
 - `NetRunner.Process` — GenServer owning the OS process lifecycle
 - `NetRunner.Process.Exec` — spawn logic (UDS, Port, SCM_RIGHTS, Pipe creation)
-- `NetRunner.Process.Nif` — NIF stubs (`nif_read`, `nif_write`, `nif_close`, `nif_create_fd`, `nif_kill`)
+- `NetRunner.Nif` — NIF stubs (`nif_read`, `nif_write`, `nif_close`, `nif_create_fd`, `nif_kill`)
+- `NetRunner.Process.Protocol` — shepherd wire protocol: frame constants, parser, command encoders
 - `NetRunner.Process.Pipe` — struct wrapping a NIF resource with owner/type metadata
 - `NetRunner.Process.Operations` — pending operation queue (park on EAGAIN, retry on select)
 - `NetRunner.Stream` — `Stream.resource` wrapper with concurrent input writer Task

README.md

@@ -338,7 +338,7 @@ Isolate child processes in a cgroup v2 hierarchy for resource control:
 )
 ```
 
-The shepherd creates the cgroup directory, moves the child into it, and cleans up on exit (kills all processes via `cgroup.kill`, then removes the directory). No-op on macOS.
+The shepherd creates the cgroup directory, moves the child into it *before* `execvp`, and cleans up on exit (kills all processes via `cgroup.kill`, then removes the directory). Spawns fail closed with `{:error, {:shepherd_error, msg}}` when the cgroup cannot be created, the leaf already exists, the migration is refused, or `cgroup.kill` (Linux ≥ 5.14) is unavailable. No-op on macOS.
 
 ## Command DSL
 

bench/LINUX_VERIFICATION.md

@@ -243,6 +243,8 @@ OTP 29 / erts 17.0.3, `MIX_ENV=prod`:
 | stdin round trip, 16 MiB via `cat` | `run/2` 1002–1163 MB/s vs `stream!/2` 1074–1130 | 1 MiB pipes mean fewer round trips; both should rise together |
 | stderr drain | 928–978 MB/s | 1 MiB stderr pipe → a drain pass is more likely to hit the 16-chunk budget and use the `:consume_stderr_more` resume path (see below) |
 | spawn | 4.4–7.2 ms | Linux `posix_spawn`/`vfork` is usually faster than macOS |
+| spawn with `cgroup_path:` | n/a (macOS no-op) | `cgroup_setup` (mkdir + `cgroup.procs` write on cgroupfs) now runs *before* the child's `execvp`, gated by a sync pipe. Run `spawn_breakdown.exs` with and without `cgroup_path:` on a delegated subtree and record the delta here |
+| ≥32 concurrent 64 MiB `stream!/2` reads | n/a (64 KiB pipes regardless) | `pipe-user-pages-soft` (default 64 MiB) is exhausted by ~21 concurrent 3×1 MiB spawns; later pipes fall back to 64 KiB. Compare the per-stream chunk count (`Proc.stats/1` `read_count`) at 1, 16 and 32 concurrent streams — a ≥10× jump between 16 and 32 confirms the cliff. Chunk counts are structural, so no median/variance discipline is needed |
 
 ### One Linux-specific behaviour to watch
 

c_src/shepherd.c

@@ -99,6 +99,52 @@ static int64_t monotonic_ms(void) {
  * buffer space before being dropped. */
 #define UDS_WRITE_TIMEOUT_MS 5000
 
+/*
+ * Absolute deadline for one bounded POLLOUT wait, initialised lazily so a
+ * write that succeeds on the first attempt never reads the clock.
+ *   at == -2  unset
+ *   at == -1  monotonic clock unavailable; `polls` caps the wait instead
+ *   at >=  0  monotonic_ms() value to stop at
+ */
+struct write_deadline {
+    int64_t at;
+    int polls;
+};
+#define WRITE_DEADLINE_INIT {-2, 0}
+
+/* Without a clock each poll() is a full timeout_ms, so this caps the total
+ * wait at NOCLOCK_MAX_POLLS * timeout_ms regardless of how often the peer
+ * frees a byte or a signal interrupts us. */
+#define NOCLOCK_MAX_POLLS 4
+
+/*
+ * Wait until fd is writable or the deadline passes. Returns 0 when
+ * writable, -1 on timeout or error. Every EAGAIN/EINTR wakeup re-derives
+ * the remaining budget from the same deadline, so a peer that frees one
+ * byte per timeout cannot extend the wait indefinitely.
+ */
+static int wait_pollout(int fd, struct write_deadline *dl, int timeout_ms) {
+    if (dl->at == -2) {
+        int64_t now = monotonic_ms();
+        dl->at = (now < 0) ? -1 : now + timeout_ms;
+    }
+    for (;;) {
+        int wait_ms = timeout_ms;
+        if (dl->at >= 0) {
+            int64_t now = monotonic_ms();
+            if (now < 0 || now >= dl->at) return -1;
+            wait_ms = (int)(dl->at - now);
+        } else if (dl->polls++ >= NOCLOCK_MAX_POLLS) {
+            return -1;
+        }
+        struct pollfd pfd = {.fd = fd, .events = POLLOUT, .revents = 0};
+        int pret = poll(&pfd, 1, wait_ms);
+        if (pret > 0) return 0;
+        if (pret < 0 && errno == EINTR) continue;
+        return -1; /* timeout, or poll error */
+    }
+}
+
 /*
  * Write all of buf to the non-blocking UDS, polling POLLOUT for up to
  * timeout_ms before giving up. Returns 0 on success, -1 on error/timeout.
@@ -114,8 +160,7 @@ static int64_t monotonic_ms(void) {
  */
 static int write_fully(int fd, const uint8_t *buf, size_t len,
                        int timeout_ms) {
-    int64_t now = monotonic_ms();
-    int64_t deadline = (now < 0) ? -1 : now + timeout_ms;
+    struct write_deadline dl = WRITE_DEADLINE_INIT;
     size_t written = 0;
 
     while (written < len) {
@@ -126,16 +171,8 @@ static int write_fully(int fd, const uint8_t *buf, size_t len,
         }
         if (n < 0 && errno == EINTR) continue;
         if (n < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) {
-            int wait_ms = timeout_ms;
-            if (deadline >= 0) {
-                now = monotonic_ms();
-                if (now < 0 || now >= deadline) return -1;
-                wait_ms = (int)(deadline - now);
-            }
-            struct pollfd pfd = {.fd = fd, .events = POLLOUT, .revents = 0};
-            int pret = poll(&pfd, 1, wait_ms);
-            if (pret < 0 && errno != EINTR) return -1;
-            if (pret == 0) return -1; /* peer not draining: drop the frame */
+            /* peer not draining within the deadline: drop the frame */
+            if (wait_pollout(fd, &dl, timeout_ms) < 0) return -1;
             continue;
         }
         return -1;
@@ -175,8 +212,10 @@ static int send_fds(int uds_fd, int *fds, int nfds) {
     cmsg->cmsg_len = CMSG_LEN((size_t)nfds * sizeof(int));
     memcpy(CMSG_DATA(cmsg), fds, (size_t)nfds * sizeof(int));
 
-    /* Retry on EINTR; wait (bounded) for buffer space on EAGAIN — the UDS
-     * is non-blocking. Anything else, or a partial send, is an error. */
+    /* Retry on EINTR; wait (bounded by one shared deadline) for buffer
+     * space on EAGAIN — the UDS is non-blocking. Anything else, or a
+     * partial send, is an error. */
+    struct write_deadline dl = WRITE_DEADLINE_INIT;
     for (;;) {
         ssize_t ret = sendmsg(uds_fd, &msg, 0);
         if (ret == 1) {
@@ -185,10 +224,7 @@ static int send_fds(int uds_fd, int *fds, int nfds) {
         }
         if (ret < 0 && errno == EINTR) continue;
         if (ret < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) {
-            struct pollfd pfd = {.fd = uds_fd, .events = POLLOUT,
-                                 .revents = 0};
-            int pret = poll(&pfd, 1, UDS_WRITE_TIMEOUT_MS);
-            if (pret > 0 || (pret < 0 && errno == EINTR)) continue;
+            if (wait_pollout(uds_fd, &dl, UDS_WRITE_TIMEOUT_MS) == 0) continue;
             ERROR_LOG("sendmsg: no buffer space after %d ms",
                       UDS_WRITE_TIMEOUT_MS);
             free(cmsg_buf);
@@ -303,6 +339,30 @@ static int cgroup_setup(pid_t child_pid) {
         return -1;
     }
 
+    /* Teardown relies on cgroup.kill (Linux >= 5.14) to reach descendants
+     * that left the child's process group (setsid/setpgid daemonisers).
+     * Without it the only kill vector is kill(-pgid), so the containment
+     * the caller asked for would be partial — fail closed instead. The
+     * directory we just made is removed so a retry does not hit EEXIST. */
+    char kill_path[576];
+    n = snprintf(kill_path, sizeof(kill_path), "%s/cgroup.kill", full_path);
+    if (n < 0 || (size_t)n >= sizeof(kill_path)) {
+        ERROR_LOG("cgroup kill path too long");
+        snprintf(cgroup_errmsg, sizeof(cgroup_errmsg),
+                 "cgroup_setup: kill path too long");
+        return -1;
+    }
+    if (access(kill_path, W_OK) != 0) {
+        int err = errno;
+        ERROR_LOG("%s unavailable: %s", kill_path, strerror(err));
+        snprintf(cgroup_errmsg, sizeof(cgroup_errmsg),
+                 "cgroup_setup: cgroup.kill unavailable (%s); teardown could "
+                 "not reach escaped descendants",
+                 strerror(err));
+        if (rmdir(full_path) == 0) cgroup_owned = 0;
+        return -1;
+    }
+
     n = snprintf(procs_path, sizeof(procs_path), "%s/cgroup.procs", full_path);
     if (n < 0 || (size_t)n >= sizeof(procs_path)) {
         ERROR_LOG("cgroup procs path too long");
@@ -352,13 +412,18 @@ static void cgroup_cleanup(void) {
                      cgroup_path);
     if (n < 0 || (size_t)n >= sizeof(full_path)) return;
 
-    /* Kill all processes in the cgroup via cgroup.kill (cgroup v2) */
+    /* Kill all processes in the cgroup via cgroup.kill (cgroup v2).
+     * cgroup_setup() verified the file was writable, so a failure here is
+     * worth a log line: it means descendants may outlive the teardown. */
     n = snprintf(kill_path, sizeof(kill_path), "%s/cgroup.kill", full_path);
     if (n < 0 || (size_t)n >= sizeof(kill_path)) return;
     FILE *f = fopen(kill_path, "w");
-    if (f) {
-        fprintf(f, "1\n");
-        fclose(f);
+    if (!f) {
+        ERROR_LOG("cgroup_cleanup: open %s failed: %s", kill_path,
+                  strerror(errno));
+    } else if (fprintf(f, "1\n") < 0 || fclose(f) != 0) {
+        ERROR_LOG("cgroup_cleanup: write to %s failed: %s", kill_path,
+                  strerror(errno));
     }
 
     /* Poll for rmdir success rather than a fixed sleep — the kernel needs
@@ -646,7 +711,7 @@ static int event_loop(int uds_fd, pid_t child_pid, int *stdin_w) {
 }
 
 /*
- * Usage: shepherd <uds_path> [--kill-timeout <ms>] [--token-fd] <cmd> [args...]
+ * Usage: shepherd <uds_path> [--kill-timeout <ms>] [--token-fd] [--pty] [--cgroup-path <rel>] [--] <cmd> [args...]
  *
  *   uds_path:       Path to the UDS listener socket created by the BEAM
  *   --kill-timeout:  SIGTERM->SIGKILL escalation timeout in ms (default 5000)
@@ -659,7 +724,7 @@ static int event_loop(int uds_fd, pid_t child_pid, int *stdin_w) {
 int main(int argc, char *argv[]) {
     if (argc < 3) {
         fprintf(stderr,
-                "usage: shepherd <uds_path> [--kill-timeout <ms>] [--token-fd] <cmd> [args...]\n");
+                "usage: shepherd <uds_path> [--kill-timeout <ms>] [--token-fd] [--pty] [--cgroup-path <rel>] [--] <cmd> [args...]\n");
         return 1;
     }
 
@@ -706,6 +771,13 @@ int main(int argc, char *argv[]) {
             }
             memcpy(cgroup_path, path, strlen(path) + 1);
             cmd_idx += 2;
+        } else if (strcmp(argv[cmd_idx], "--") == 0) {
+            /* End of shepherd options. The BEAM always emits this so a
+             * command whose name starts with '-' can never be parsed as a
+             * shepherd flag (which would let a caller re-target
+             * --cgroup-path or stretch --kill-timeout behind the API). */
+            cmd_idx += 1;
+            break;
         } else {
             break; /* Unknown flag — treat as command */
         }