Fix review findings: stderr API, UDS perms, signal range, write loop,…#7
Merged
Conversation
… Daemon Address findings from a parallel code review of recent changes. Blocker: - Remove documented-but-unimplemented `stderr: :redirect` (output was silently dropped and the child could block on a full stderr pipe). `:stderr` is now validated at spawn time (`:consume`/`:disabled`); docs, typespec, and README updated. Security/correctness: - nif_kill: reject signals outside 1..31 (mirrors shepherd CMD_KILL). - UDS socket now lives in a per-spawn 0700 dir, blocking cross-user SCM_RIGHTS hijack; also cleans up the dir on failure paths (was leaking empty dirs in tmp). - Map a zero-byte write on a non-empty buffer to EAGAIN+enif_select in the NIF and drop the GenServer-blocking Process.sleep(1) retry. - Daemon drains via Task.Supervisor.async_nolink (added NetRunner.TaskSupervisor) so a drain crash can't take the Daemon down. Cleanups: - Log shepherd errors in handle_uds_message instead of dropping them. - Operations: O(1) demonitor via an op_ref -> mref reverse index. - Command.new/3: guard clauses instead of unless/raise. Tests: - process_test: poll instead of sleep(500) for async cleanup. - net_runner_test: unique `pgrep -f` marker (no longer racy under async). - signal_test: assert_raise message patterns. - daemon_test: drain-task isolation test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses findings from a parallel code review of the recent changes (commits #4–#6). One blocker, four warnings, and several cleanups.
Verified:
mix format --check-formatted,mix compile --warnings-as-errors(clean C rebuild),mix credo --strict(no issues),mix test→ 140 passed / 2 excluded.Blocker
stderr: :redirect. Output was silently dropped, and a child emitting enough stderr could block on a full pipe.:stderris now validated at spawn time (:consume/:disabledonly); docs, typespec, and README updated.Security / correctness
nif_kill: reject signals outside1..31(mirrors the shepherd'sCMD_KILLvalidation).0700directory, blocking the cross-userSCM_RIGHTSFD-hijack vector. Also cleans up the dir on failure paths (previously leaked empty dirs intmp).Peer-credential verification (
SO_PEERCRED) deferred — platform-divergent.EAGAIN+enif_selectinside the NIF, dropping the GenServer-blockingProcess.sleep(1)retry.Task.Supervisor.async_nolink(addedNetRunner.TaskSupervisorto the app tree) so a drain-task crash can't take the Daemon down.Cleanups
handle_uds_messageinstead of dropping them.Operations: O(1) demonitor via anop_ref -> mrefreverse index (was O(n)Enum.find).Command.new/3: guard clauses instead ofunless/raise.Tests
process_test: poll instead ofsleep(500)for async cleanup (CI flake risk).net_runner_test: uniquepgrep -fmarker — no longer racy underasync: true.signal_test:assert_raisenow matches the error message.daemon_test: added a drain-task isolation test.Deferred (intentional)
write_all_inputerror propagation — broken-pipe on stdin is normal forhead-style commands; making it fatal would regress.