Skip to content

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

Open
nyo16 wants to merge 1 commit into
masterfrom
review-fixes/audit-remediation
Open

nyo16 wants to merge 1 commit into
masterfrom
review-fixes/audit-remediation

Conversation

@nyo16

@nyo16 nyo16 commented Oct 8, 2026

Copy link
Copy Markdown
Owner

Second-pass fixes for the 2026-10-07 review of #12 (1116a77), which added a
whole-file security and performance audit of c_src/. 14 findings fixed,
3 triage items consciously narrowed (see bottom). Review artefacts live under
.claude/plans/review/ (not in this PR).

Security

  • Shepherd argv is ---terminated. A command whose name starts with -
    (--cgroup-path, --kill-timeout, --pty) could be parsed as a shepherd
    flag and re-target the cgroup or stretch the kill ladder behind the API.
  • cgroup teardown requires cgroup.kill. cgroup_setup probes the new
    leaf with access(W_OK) and fails the spawn closed (rolling back its own
    mkdir) when it is unavailable; without it a setsid() daemoniser escapes
    kill(-pgid). The cleanup write is now checked and logged.
  • Signal routing is probe-then-signal. CMD_KILL over the UDS while the
    shepherd Port lives; once the Port is dead or the send fails,
    NetRunner.Process probes kill(pid, 0) before nif_kill. 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 — a no-op that
    documented a guarantee the code did not have.

Fixed

  • send_fds restarted its 5 s POLLOUT window on every EAGAIN; it and
    write_fully now share wait_pollout() with one lazily-computed absolute
    deadline (a first-try write never reads the clock) and a hard poll cap when
    the monotonic clock is unavailable.
  • kill/2 reports lost signals: send_shepherd_command returns
    :ok | {:error, _}, failed sends fall back to the direct probe-and-kill,
    and {:error, :not_running} replaces a misleading :ok.
    set_window_size/3 surfaces a failed send.
  • Option validation is uniform: Daemon.start_link/1 validates
    :process_opts in the caller via the new public Process.validate_opts!/1
    (previously raised inside init/1 and exited the linked caller);
    :kill_timeout is range-checked (1..60_000) and :cgroup_path rejects
    NUL bytes and non-binaries — instead of a silent 10 s
    :shepherd_connect_timeout.
  • Protocol.kill/1 rejects signals outside 1..255 instead of truncating.
  • macOS test flake: sleep markers built from System.unique_integer/1
    crossed INT32_MAX late in a run and /bin/sleep exited 1;
    sleep_marker/0 keeps them in range.

Added

  • Stats.shepherd_error exposes a post-spawn shepherd MSG_ERROR (was buried
    in server state).
  • Tests (+11, 253 total): Watcher kills an orphan after shepherd death,
    direct kill/2 after shepherd death, Protocol encoder byte layouts
    pinned to protocol.h, post-spawn MSG_ERROR recording, Daemon
    client-side validation, :kill_timeout validation, -- terminator
    end-to-end. PTY-resize sentinel no longer matches its own failure message
    ("NEVER RESIZED" =~ "RESIZED"); teardown test excludes the stdin
    forwarder from its drain-task sample.
  • CI: NR_CGROUP_DELEGATED=1 on the delegated Ubuntu leg turns the cgroup
    tests' fail-closed branch into a hard failure, so a delegation regression
    can't ship as a green build with zero cgroup coverage. Alpine leg
    documented as fail-closed-only.

Docs

  • NetRunner.Process.Nif → NetRunner.Nif and Exec.parse_uds_message →
    Protocol.parse_uds_message references corrected (CLAUDE.md,
    docs/modules.md, docs/protocol.md); Layer 3 is the NIF owner monitor,
    not GC; a pre-existing cgroup dir is a spawn error.
  • docs/backpressure.md: pipe-user-pages-soft caveat — 3 × 1 MiB per
    spawn exhausts the default budget at ~21 concurrent commands, after which
    pipes fall back to 64 KiB. Derived from kernel defaults, unmeasured.
  • bench/LINUX_VERIFICATION.md: cgroup spawn-latency and ≥32-concurrent
    chunk-count rows for a Linux host to fill in.

Deliberate deviations from triage

  • Force-exit backstop kept at 5 s, not made immediate on shepherd exit:
    recv_uds can return :ealready while a genuine MSG_CHILD_EXITED is
    still pending as a $socket message (the Retry UDS drain on port exit to fix macOS CI race #5 macOS race); synthesising 137
    immediately would overwrite a real exit code. Probe-then-signal already
    removed the pid-reuse exposure from that window.
  • Leading-- commands are not rejected. The -- terminator alone closes
    the hole (BEAM-emitted flags can never equal --), and -foo is a legal
    command name.
  • No {:error, {:shepherd_unreachable, _}} reply shape. All send
    failures fall through to the direct probe-and-kill; the observable error
    is {:error, :not_running}.

Known follow-ups (second-pass review, not in this PR)

  • Sync pipe (shepherd.c:913,1097) and nif_dup_fd (net_runner_nif.c:515)
    are still non-CLOEXEC — now flagged by convention C001.
  • Daemon.start_link(args: []) with no :cmd still raises KeyError inside
    init/1.
  • kill -KILL <ppid> in two tests is unguarded on a reparented child;
    sleep_marker can prefix-collide under pgrep -f.
  • The live-Port + failed-send fallback branch has no test.

Verification

  • mix format --check-formatted · WERROR=1 mix compile --warnings-as-errors
    (clang) · mix credo --strict — clean
  • mix test: 253 passed, 2 excluded (:linux_only), green on default seed
    and seed 151793 (the one that previously failed)
  • Runtime smoke: run(["--pty", "sh", "-c", "echo pwned"]) →
    {"", 127, "execvp: --pty\n"}; kill via shepherd → :ok / 137; kill after
    exit → {:error, :not_running}
  • Not exercised on this host: :linux_only cgroup tests (incl. the new
    cgroup.kill probe), gcc -Werror build, dialyzer

…erf pass)

## Security

- **Shepherd argv is `--`-terminated.** A command whose name starts with `-`
  (`--cgroup-path`, `--kill-timeout`, `--pty`) could be parsed as a shepherd
  flag and re-target the cgroup or stretch the kill ladder behind the API.
- **cgroup teardown requires `cgroup.kill`.** `cgroup_setup` probes the new
  leaf with `access(W_OK)` and fails the spawn closed (rolling back its own
  mkdir) when it is unavailable; without it a `setsid()` daemoniser escapes
  `kill(-pgid)`. The cleanup write is now checked and logged.
- **Signal routing is probe-then-signal.** `CMD_KILL` over the UDS while the
  shepherd Port lives; once the Port is dead or the send fails,
  `NetRunner.Process` probes `kill(pid, 0)` before `nif_kill`. 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 (a no-op).

## Fixed

- `send_fds` restarted its 5 s `POLLOUT` window on every `EAGAIN`; it and
  `write_fully` now share `wait_pollout()` with one lazily-computed absolute
  deadline and a hard poll cap when the monotonic clock is unavailable.
- `kill/2` reports lost signals: `send_shepherd_command` returns
  `:ok | {:error, _}`, failed sends fall back to the direct probe-and-kill,
  and `{:error, :not_running}` replaces a misleading `:ok`.
  `set_window_size/3` surfaces a failed send.
- Option validation is uniform: `Daemon.start_link/1` validates
  `:process_opts` in the caller via the new public
  `Process.validate_opts!/1`; `:kill_timeout` is range-checked (1..60_000)
  and `:cgroup_path` rejects NUL bytes and non-binaries.
- `Protocol.kill/1` rejects signals outside 1..255 instead of truncating.
- macOS test flake: `sleep` markers built from `System.unique_integer/1`
  crossed `INT32_MAX` late in a run; `sleep_marker/0` keeps them in range.

## Added

- `Stats.shepherd_error` exposes a post-spawn shepherd `MSG_ERROR`.
- Tests: Watcher kills an orphan after shepherd death, direct `kill/2` after
  shepherd death, `Protocol` encoder byte layouts, post-spawn `MSG_ERROR`
  recording, Daemon client-side validation, `:kill_timeout` validation,
  `--` terminator. PTY-resize sentinel no longer matches its own failure
  message; teardown test excludes the stdin forwarder from its drain sample.
- CI: `NR_CGROUP_DELEGATED=1` on the delegated Ubuntu leg makes the cgroup
  tests' fail-closed branch a hard failure. Alpine leg documented as
  fail-closed-only.

## Docs

- `NetRunner.Process.Nif` -> `NetRunner.Nif` and `Exec.parse_uds_message`
  -> `Protocol.parse_uds_message` references corrected; Layer 3 is the NIF
  owner monitor, not GC; pre-existing cgroup dir is a spawn error.
- `docs/backpressure.md`: `pipe-user-pages-soft` caveat (3 x 1 MiB per spawn
  exhausts the default budget at ~21 concurrent commands; unmeasured).
- `bench/LINUX_VERIFICATION.md`: cgroup spawn-latency and >=32-concurrent
  chunk-count rows for a Linux host to fill in.

## Deliberate deviations from triage

- Force-exit backstop kept at 5 s (not immediate): `recv_uds` can hit
  `:ealready` with a real `MSG_CHILD_EXITED` still pending (PR #5 race).
- Leading-`-` commands are not rejected; the `--` terminator alone closes
  the hole and `-foo` is a legal command name.
- No `{:error, {:shepherd_unreachable, _}}` shape; all send failures fall
  through to the direct probe-and-kill.

Verified: format, `WERROR=1` compile (clang), credo --strict, 253 tests
green on two seeds. Not exercised: `:linux_only` cgroup tests, gcc build.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant