Harden Asuswrt-Merlin service install and lifecycle

[maghuro]

Important

Local staging/review draft in the fork only. Upstream contribution guidelines require an issue before any PR is submitted to Control-D-Inc/ctrld.

Summary

Harden the Asuswrt-Merlin service install/start/stop/uninstall and self-upgrade lifecycle independently from the dnsmasq/SDN/NVRAM integration in #1/#2.

Changes

• atomically render/publish and conservatively migrate the private ctrld.startup script;
• migrate exact v1.5.x startup scripts from code executed by the newly launched binary after self-upgrade, while preserving installed argv;
• preserve current/custom startup scripts and keep an exact legacy script usable when migration publication fails;
• serialize Merlin Start, Stop, Uninstall and the real CLI restart/upgrade stop-cleanup-start transactions with a cross-process flock;
• use atomic exchange plus inode/content validation for legacy startup migration, with fail-safe preservation when an external actor races publication or rollback;
• POSIX-shell-quote the executable and every argument;
• validate service PID ownership through /proc/<pid>/cmdline, wait for the child to exec, and kill the exact PID whose ownership was validated;
• make restart strictly stop-before-start and report an intentionally stopped service as StatusStopped;
• publish missing shared Merlin hook stubs from a fully written/fsynced private file with no-clobber semantics;
• keep shared services-start / service-event edits idempotent and preserve unrelated content/modes;
• roll back partial installs conservatively and remove both current quoted and legacy unquoted hook references on uninstall;
• validate the dnsmasq PID before the diskmon service-event sends it a signal;
• add regression coverage for shell quoting, publication/migration races, restrictive umask, stale/reused PIDs, child-exec timing, restart ordering, lifecycle locking and hook ownership.

Validation

Current head passes go test -race ./... and Staticcheck on Ubuntu and Windows. The macOS job on this main-based branch fails in the pre-existing WillAbides/setup-go-faster@v1.8.0 step before repository tests run.

The final Codex request was blocked by review-credit limits; all findings from completed Codex reviews were addressed, followed by a manual base-to-head adversarial review.

Scope

Service lifecycle only. dnsmasq hook / Guest Network Pro / SDN behavior remains in #1/#2.

[maghuro]

@codex review

Please review this Merlin service-lifecycle draft from scratch. Focus especially on:

• PID-file reuse/stale PID handling on BusyBox/Asuswrt-Merlin;
• whether /proc//cmdline matching is safe during in-place binary upgrades;
• install rollback if one shared hook edit succeeds and the next fails;
• uninstall ordering and preservation of shared services-start/service-event files;
• idempotence of pc_delete + pc_append;
• shell portability of the generated startup script and tests.

This is intentionally independent from dnsmasq/NVRAM work in #1/#2.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4dd7d053d0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[maghuro]

@codex review

Fixed the test-only P1 on current head c1463c2:

• the raw-string assertion now expects the same single backslash used by the generated BusyBox shell command, tr '\000' ' '.

Please review the current head again, including the service lifecycle logic itself.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1463c23ba

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[maghuro]

@codex review

Addressed the startup race on current head 5f24b68:

• after spawning the background child, the Merlin startup script now polls up to 5 seconds for the recorded PID's cmdline to become the expected ctrld executable before declaring failure;
• a failed startup removes the stale PID file;
• added a regression requiring the exec-wait loop.

Please review the current head again, including install rollback/uninstall/PID ownership behavior.

[maghuro]

@codex review

One additional shared-hook ownership hardening landed on current head 104fd0b:

• installing ctrld no longer chmods pre-existing services-start/service-event hooks;
• rollback of a hook file created by ctrld first removes only ctrld's exact line, and deletes the file only if it is still the untouched "#!/bin/sh" stub;
• concurrent addon/user content therefore survives a failed ctrld install.

Please review the current head from scratch.

[maghuro]

@codex review

Current head eff5a93 also fixes an existing Merlin service-state bug:

• the generated status script intentionally prints stopped and exits 1;
• merlinSvc.Status() now interprets that output as service.StatusStopped instead of returning StatusUnknown + error;
• unexpected output still returns an error;
• added regressions for stopped and unexpected states.

Please review the full current head again.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: eff5a93960

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[maghuro]

@codex review

Final service-wrapper hardening on current head a5758e1:

• removed the flattened cmd="..." + $cmd launch path, which could not preserve argument boundaries;
• the generated shell now invokes the executable with each path/argument POSIX single-quoted independently;
• added merlinShellQuote and regressions covering spaces and embedded single quotes;
• PID ownership still validates argv[0] against the executable path.

Please review the complete current head again, with particular attention to POSIX/BusyBox shell quoting, template rendering, PID matching, and install/start lifecycle behavior.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a5758e1e86

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[maghuro]

@codex review

Addressed the partial-startup-script P2 on current head f067b46:

• startup script is now fully written+fsync'd to a same-directory temp file;
• publication uses hard-link no-replace semantics, so a concurrently created target is never clobbered;
• the helper reports whether ctrld actually published the target, so error cleanup never removes an unrelated file;
• failed installs remove only ctrld's published startup script and sync the directory;
• added success/mode and no-clobber regression tests.

Please review the complete current head again, including shell portability, startup-script publication crash windows, install rollback, and compile/test consistency.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[maghuro]

@codex review

Current head 1d66661 also makes interrupted installs resumable:

• Install renders the expected private startup script before deciding whether an existing file is usable;
• an existing byte-identical ctrld startup script is treated as a resumable prior publication and hook installation continues;
• a differing existing startup script is never overwritten;
• rollback removes the startup script only when this invocation actually published it, so retrying an older partial install cannot delete pre-existing state.

Please include this in the full lifecycle review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5f3349999c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[maghuro]

@codex review

Candidate-final holistic review request for exact head 1d5b7567ae.

Please DO NOT review incrementally and do not stop after finding the first issue. Inspect the entire PR base-to-head first, trace all interacting control-flow paths, and return ALL material findings you can identify in ONE review pass, grouped by severity. If several problems are related, report them together rather than drip-feeding one finding per review cycle.

The previous self-upgrade P1 is now addressed by migrating the legacy Merlin startup script from code executed by the newly launched/downloaded binary itself:

• ctrld run calls RefreshMerlinStartupScriptFromRunningBinary after daemonization, so the code definitely comes from the new on-disk binary rather than the old upgrader process;
• the migration is best-effort/non-fatal, atomically preserves the legacy script on failure, and retries on future daemon starts;
• a regression reproduces the first upgraded-binary process with only the executable path and verifies exact migration while preserving the installed argv;
• Start remains a secondary migration path;
• Stop and the complete Restart transaction are now covered by the same Merlin lifecycle lock.

Please perform one exhaustive review covering at least:

1. fresh install and failed/partial install rollback;
2. exact v1.5.7 upgrade path, including which binary/process executes each step;
3. self-update success and rollback;
4. normal start/stop/restart/status;
5. current-v2 script preservation and future restart behavior;
6. legacy argv recovery and shell quoting;
7. ENOSPC/EIO/power-loss/crash durability;
8. all ctrld-vs-ctrld concurrency and external user/addon races;
9. symlink, special-file, hardlink and pathname-replacement/TOCTOU cases;
10. services-start/service-event coexistence, modes, install rollback and uninstall;
11. PID reuse/stale PID/process identity and exec timing;
12. diskmon service_event PID validation;
13. BusyBox/Asuswrt-Merlin shell compatibility;
14. uninstall/reinstall/idempotence;
15. Windows/macOS/Linux compile and test portability;
16. the new cmd/cli call site and import/package layering;
17. interaction assumptions with the already-reviewed Merlin DNS/SDN PR Prefer Merlin dnsmasq hooks and fix Guest Network Pro integration #2.

Please inspect ALL changed files and adjacent callers before posting findings. The goal of this pass is convergence: one complete review, not another sequence of one-at-a-time discoveries.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1d5b7567ae

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[maghuro]

@codex review

FINAL CANDIDATE - exhaustive base-to-head review of exact head 14cf31728192c72c68fff1b1543ca123b3658ac3.

Please review the ENTIRE PR from main to this head as one coherent change. Do not review only the latest commits and do not stop after the first finding. Return ALL material correctness, lifecycle, race, portability, security, crash-recovery and test/compile findings you can identify in ONE pass, grouped by severity. We are deliberately trying to avoid one-finding-per-cycle review.

Please explicitly re-check the three findings from your previous holistic review:

1. migration failure must still allow Start through an intact exact legacy script;
2. the CLI's REAL restart and upgrade flows must serialize Stop -> router/DNS cleanup/wait -> Start, not merely expose an unused locked Restart method;
3. startup migration must not overwrite a target modified/replaced by another actor.

Current implementation for those:

• exact-legacy publication failures return a typed migration error; Start re-reads the path and proceeds only if it is still the exact captured legacy bytes or the current marked script;
• Stop is locked, Restart holds one lock, and both actual CLI restart + upgrade flows use restartServiceTransaction; Merlin implements RestartWith, holding one cross-process flock around stop, the cleanup/wait callback, and start;
• Linux migration uses renameat2(RENAME_EXCHANGE); after the atomic exchange it validates that the displaced target is the original inode AND exact legacy bytes. Unsafe rollback also verifies that the public path is still ctrld's prepared inode/bytes before exchange-back, otherwise it preserves the captured legacy inode in quarantine rather than clobbering an external change;
• Asuswrt-Merlin's kernel/JFFS2 source includes explicit RENAME_EXCHANGE support;
• successful publication of ctrld's private startup script is kept for resumable install instead of being deleted by pathname on a later install failure;
• first self-upgrade migration is invoked from the newly launched ctrld run process, not solely from the old upgrader's in-memory service object.

Please inspect all changed files AND adjacent callers, including:

• fresh install, partial install and retry;
• exact v1.5.7 install and first self-upgrade;
• self-update success/failure/rollback and old-vs-new process execution;
• normal service start/stop/restart/status and actual Cobra restart command;
• upgrade doRestart and second restart after binary rollback;
• lifecycle flock coverage and nested-lock/deadlock risk;
• legacy argv recovery and shell quoting;
• current-v2/custom script preservation;
• ENOSPC/EIO, fsync/rename/exchange and power-loss behavior;
• external in-place edits, inode/path replacements, symlink/special-file/hardlink attacks, TOCTOU before/after exchange and rollback;
• shared services-start/service-event editing, exact-line ownership, modes, rollback and uninstall;
• PID reuse, stale PID, child exec timing and diskmon dnsmasq PID validation;
• BusyBox shell compatibility and GT-AX6000/Merlin assumptions;
• uninstall/reinstall/idempotence;
• Linux/Windows/macOS build/test portability;
• new cmd/cli/cli.go migration call and package layering;
• interaction with already-reviewed PR Prefer Merlin dnsmasq hooks and fix Guest Network Pro integration #2 DNS/SDN changes.

Please also call out any semantic regression in non-Merlin service restart behavior caused by the new generic restartServiceTransaction fallback.

If there are no remaining material issues, please say so explicitly.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[maghuro]

Final local review note - no further Codex ping because code-review credits are exhausted.

All material findings from the last completed Codex reviews have been addressed in the current branch:

• first self-upgrade migration runs from the newly launched binary;
• exact legacy startup remains usable if migration publication fails;
• Stop and the real CLI restart/upgrade transactions are serialized on Merlin;
• Linux migration uses atomic exchange and validates displaced inode+bytes;
• unsafe exchange rollback refuses to clobber external changes;
• successful private startup publication is resumable rather than pathname-rolled-back;
• non-Merlin restart behavior has been kept on the historical path rather than opting into the Merlin transaction.

The final attempted holistic Codex review did not run: the bot returned only the usage-limit message, with no new finding.

Windows passes go test -race and staticcheck on the current head's workflow. The first Ubuntu attempt failed only in the unrelated root Test_osResolver_Singleflight; that job has been rerun to distinguish the known upstream flake from a reproducible branch failure. macOS continues to fail in setup-go-faster before tests.

[maghuro]

Final local readiness note:

• current head: ca315796d4;
• the last completed Codex review found the real CLI restart transaction was not fully serialized; subsequent commits implemented the Merlin-only RestartWith path and restored the historical non-Merlin restart flow;
• the final requested holistic Codex review did not run because code-review credits were exhausted;
• manual base-to-head review of the post-Codex commits found no remaining material lifecycle issue;
• current-head CI passes on Ubuntu and Windows;
• macOS fails in WillAbides/setup-go-faster@v1.8.0 before repository tests start.

All material findings from completed Codex reviews are addressed.

[maghuro]

Final manual-review follow-up on current head 6db0253d1b:

• replaced shared-hook stub publication with build-private + fsync + same-directory hard-link no-clobber publication, so a raced external hook is never truncated or removed by pathname rollback;
• stop now captures one PID, validates that exact PID as ctrld, and signals that same PID instead of re-reading the pidfile between validation and kill;
• added regressions for both cases;
• Ubuntu and Windows pass go test -race ./... and Staticcheck on this head;
• macOS still fails only in the pre-existing WillAbides/setup-go-faster@v1.8.0 bootstrap step before repository tests.

All 20 historical review threads are now resolved. No completed Codex finding remains open; the final holistic Codex pass could not run because review credits were exhausted.

[maghuro]

Superseded by the maintained v1.x port in #8, now merged into v1.0 after #6/#7. This main-based draft is retained only as review history/source-of-truth for the port.