Daemon: harden launchd plist writes and abortable service restarts#46493
Daemon: harden launchd plist writes and abortable service restarts#46493shichangs wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR hardens launchd plist handling and service restart paths by: (1) resolving the user home directory through
Confidence Score: 4/5
|
| await handle.close(); | ||
| await fs.rename(tempPath, plistPath); |
There was a problem hiding this comment.
Temp file orphaned on rename failure
If fs.rename(tempPath, plistPath) fails (e.g. due to a permissions error or an unexpected race), the temp file at tempPath will be permanently orphaned in the LaunchAgents directory. Even though the file has 0600 permissions and is in the same directory as the final target (making a cross-device failure unlikely), it's still best practice to clean it up on failure:
| await handle.close(); | |
| await fs.rename(tempPath, plistPath); | |
| await handle.close(); | |
| try { | |
| await fs.rename(tempPath, plistPath); | |
| } catch (error) { | |
| await fs.unlink(tempPath).catch(() => undefined); | |
| throw error; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/daemon/launchd.ts
Line: 248-249
Comment:
**Temp file orphaned on rename failure**
If `fs.rename(tempPath, plistPath)` fails (e.g. due to a permissions error or an unexpected race), the temp file at `tempPath` will be permanently orphaned in the `LaunchAgents` directory. Even though the file has `0600` permissions and is in the same directory as the final target (making a cross-device failure unlikely), it's still best practice to clean it up on failure:
```suggestion
await handle.close();
try {
await fs.rename(tempPath, plistPath);
} catch (error) {
await fs.unlink(tempPath).catch(() => undefined);
throw error;
}
```
How can I resolve this? If you propose a fix, please make it concise.|
Superseded by #46499. I split the original rescue-watchdog work again and folded the daemon hardening into the new core PR so reviewers can see the actual watchdog runtime value in one place instead of starting from a standalone low-level daemon PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c83aca214e
ℹ️ 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".
| const taskName = resolveTaskName(effectiveEnv); | ||
| await execSchtasks(["/End", "/TN", taskName]); | ||
| const sig = signal ? { signal } : undefined; | ||
| await execSchtasks(["/End", "/TN", taskName], sig); |
There was a problem hiding this comment.
Abort Windows restart immediately when /End is canceled
After adding signal support, execSchtasks(["/End", ...], sig) can now return an aborted/non-zero result, but that result is ignored and the restart flow continues into listener termination and port-wait logic before failing later. In watchdog cancellation scenarios this defeats prompt cancelation and can still tear down runtime state after the caller has already aborted. Handle the /End result the same way stopScheduledTask does (allow only "not running"), and abort the restart path immediately on other non-zero outcomes.
Useful? React with 👍 / 👎.
| fsConstants.O_WRONLY | fsConstants.O_CREAT | fsConstants.O_TRUNC | fsConstants.O_NOFOLLOW, | ||
| LAUNCH_AGENT_PLIST_MODE, |
There was a problem hiding this comment.
Guarantee 0600 when writing launchd plist atomically
The secure writer opens the temp file with O_CREAT | O_TRUNC and then renames it, but never chmods the final plist. If that temp path already exists, POSIX ignores the passed mode on open, so the file can keep broader preexisting permissions and the resulting plist may not actually be 0600. Using O_EXCL (or a random unique temp filename) and/or enforcing mode after rename avoids this permission regression.
Useful? React with 👍 / 👎.
Summary
0600, and threadsAbortSignalthrough launchd/systemd/schtasks restart helpers viarunCommandWithTimeout.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
0600via a temp-file + rename flow.Security Impact (required)
Yes/No): NoYes/No): NoYes/No): NoYes/No): YesYes/No): NoYes, explain risk + mitigation:This change only narrows existing service-control behavior. Child restart commands now honor
AbortSignal, and launchd plist writes reject symlink targets and use trusted homedir resolution plus atomic writes to reduce path-hijack risk.Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Human Verification (required)
What you personally verified (not just CI), and how:
HOMEoverride and uses trusted user home lookup.0600mode.AbortSignalnow propagates through launchd/systemd/schtasks restart helpers into the process exec layer.os.userInfo()failure falls back toos.homedir().Review Conversations
Compatibility / Migration
Yes/No): YesYes/No): NoYes/No): NoFailure Recovery (if this breaks)
src/daemon/launchd.ts,src/daemon/systemd.ts,src/daemon/schtasks.ts,src/process/exec.tsRisks and Mitigations