pidfile: Import new pidfile from NetBSD#617
Conversation
While here, remove the hard coded limit on pidfile size.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors pidfile handling to use a heap-allocated path and an FD-tracked lock with conditional unlinking; adds pidfile accessors and unlock API; updates configure to enable compat pidfile; adjusts dhcpcd main, privsep flows, seccomp rules; and adds POSIX includes where needed. ChangesPidfile infrastructure and integration
Sequence DiagramssequenceDiagram
participant app as Application
participant pf as pidfile module
participant fs as Filesystem
app->>pf: pidfile_lock(path)
pf->>pf: allocate pf_path
pf->>fs: open with O_RDWR|O_CREAT|O_NONBLOCK
pf->>fs: write current PID via dprintf
pf->>pf: store pf_fd, register atexit
app->>pf: pidfile_path()
pf-->>app: return pf_path
app->>pf: pidfile_unlock()
pf->>pf: validate pf_fd ownership
pf->>fs: truncate and conditionally unlink
pf->>pf: close fd, free pf_path
flowchart TD
main["main()"]
asprintf_call["asprintf(ctx.pidfile)"]
cap_limit["cap_rights_limit(pidfile_fd())"]
shutdown["Shutdown"]
free_call["free(ctx.pidfile)"]
main-->asprintf_call
asprintf_call-->cap_limit
shutdown-->free_call
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
configure (1)
1042-1072:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix configure pidfile feature probe to test the full pidfile API used by the build
- The current probe only compiles/references
pidfile_unlock(); later code also needspidfile_lock(),pidfile_read(),pidfile_fd()(underPRIVSEP_RIGHTS), andpidfile_unremoveable().- On systems where
<util.h>/libutil provides only a partial pidfile API, configure may setHAVE_UTIL_Hand skipcompat/pidfile.c, leading to missing symbols or wrong declarations.Suggested fix
cat <<EOF >_pidfile.c `#include` <stdlib.h> `#include` <util.h> int main(void) { - return (int)pidfile_unlock(); + (void)pidfile_unlock(); + (void)pidfile_lock(NULL); + (void)pidfile_read(NULL); + (void)pidfile_fd(); + pidfile_unremoveable(); + return 0; } EOF🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@configure` around lines 1042 - 1072, The pidfile feature probe currently only tests pidfile_unlock(), which can let partial libutil implementations slip through; update the probe in configure to compile/link a small test program that references the full pidfile API used by the build (pidfile_lock, pidfile_unlock, pidfile_read, pidfile_fd, pidfile_unremoveable) so the check only succeeds if all those symbols are available (and keep using -lutil when needed); ensure the probe sets HAVE_UTIL_H and skips adding compat/pidfile.c only when the combined test program actually links and runs successfully, otherwise fall back to defining PIDFILE_LOCAL and adding compat/pidfile.c to CONFIG_MK.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@compat/pidfile.c`:
- Around line 111-123: The function pidfile_unlock currently sets error = EBADF
when pf_fd == -1 which returns a positive errno instead of the documented -1;
change the branch so that when pf_fd == -1 it sets error = -1 (preserving the
documented contract), keep the existing behavior for the close() path (capture
close(pf_fd) return into error, then set pf_fd = -1), and ensure pf_path is
still freed and cleared; update only pidfile_unlock and references to
pf_fd/pf_path as needed.
- Around line 193-214: pidfile_read() currently opens a temporary fd when
reading a pidfile that is not the tracked pf_path and never closes it, leaking
descriptors; after calling pidfile_readfd(fd) in pidfile_read(), if fd is not
the tracked pf_fd (i.e., pf_fd != -1 and strcmp(...) check failed) close the fd
(and only then) before jumping to out, making sure you don't close pf_fd or -1;
reference variables/functions: pidfile_read(), pidfile_readfd(), fd, pf_fd,
pf_path, dpath.
- Around line 262-263: The fallback branch that reads descriptor flags into opts
with fcntl(fd, F_GETFD) should set the close-on-exec flag using F_SETFD
(descriptor flags), not F_SETFL (file-status flags); change the failing call to
fcntl(fd, F_SETFD, opts | FD_CLOEXEC) so the FD_CLOEXEC bit is applied to fd
(the same opts from F_GETFD) and keep the existing -1 error checks in
pidfile_lock()/the surrounding block.
In `@compat/pidfile.h`:
- Around line 34-44: The header uses pid_t in declarations (e.g., pidfile_lock,
pidfile_read) but doesn't guarantee the typedef is available; add a
type-providing include such as `#include` <sys/types.h> (or another portable
header that defines pid_t) at the top of compat/pidfile.h before the
pid_t-containing declarations so pid_t is always defined even when config.h is
generated by configure.
---
Outside diff comments:
In `@configure`:
- Around line 1042-1072: The pidfile feature probe currently only tests
pidfile_unlock(), which can let partial libutil implementations slip through;
update the probe in configure to compile/link a small test program that
references the full pidfile API used by the build (pidfile_lock, pidfile_unlock,
pidfile_read, pidfile_fd, pidfile_unremoveable) so the check only succeeds if
all those symbols are available (and keep using -lutil when needed); ensure the
probe sets HAVE_UTIL_H and skips adding compat/pidfile.c only when the combined
test program actually links and runs successfully, otherwise fall back to
defining PIDFILE_LOCAL and adding compat/pidfile.c to CONFIG_MK.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02aff9f8-6fb6-4088-aa8a-9e236ca9a3a2
📒 Files selected for processing (10)
compat/pidfile.ccompat/pidfile.hconfiguresrc/bpf-bsd.csrc/common.csrc/dhcpcd.csrc/dhcpcd.hsrc/privsep-control.csrc/privsep-linux.csrc/privsep.c
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
compat/pidfile.c (3)
302-307:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing NULL check on
strdup()return.If
strdup()fails,pf_pathbecomes NULL whilepf_fdremains valid. Later,pidfile_clean()would callunlink(pf_path)with NULL, which is undefined behavior.🛡️ Suggested fix
if (path == dpath) { pf_path = dpath; dpath = NULL; - } else + } else if ((pf_path = strdup(path)) == NULL) { + int error = errno; + (void)close(pf_fd); + pf_fd = -1; + errno = error; + goto out; + } - pf_path = strdup(path); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compat/pidfile.c` around lines 302 - 307, The code uses pf_path = strdup(path) without checking for NULL; if strdup fails pf_path will be NULL while pf_fd stays valid and pidfile_clean() may call unlink(pf_path) on NULL. Update the logic around strdup in the block handling (path vs dpath) to detect a NULL return from strdup(path), handle the error by cleaning up the open descriptor (pf_fd) and any owned dpath (set pf_fd to an invalid value or close it and free dpath), ensure pf_path is left in a safe state (non-NULL or handled so pidfile_clean() won't call unlink on NULL), and propagate/return an error as appropriate; reference the symbols pf_path, pf_fd, pidfile_clean(), strdup(), dpath, and path to locate the change.
79-81:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix non-portable NetBSD/OpenBSD-only
strtoi()usage incompat/pidfile.c
strtoi()is not a standard C/POSIX function; it’s a NetBSD/OpenBSD extension and may be undeclared on other platforms, breaking the build. Replace it withstrtol()plus explicit range/trailing-character checks to preserve the existing1..INT_MAXand “trailing newline is OK” behavior.🐛 Suggested fix using strtol
- pid = (pid_t)strtoi(buf, &eptr, 10, 1, INT_MAX, &error); - if (error != 0 && !(error == ENOTSUP && *eptr == '\n')) + errno = 0; + long val = strtol(buf, &eptr, 10); + if (errno != 0 || eptr == buf || (val < 1 || val > INT_MAX)) return -1; + if (*eptr != '\n' && *eptr != '\0') + return -1; + pid = (pid_t)val;If
errorbecomes unused after this change, remove it to avoid-Werror=unused-variablefailures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compat/pidfile.c` around lines 79 - 81, The code uses the nonportable NetBSD/OpenBSD strtoi() in compat/pidfile.c (the pid = (pid_t)strtoi(buf, &eptr, 10, 1, INT_MAX, &error); block); replace that call with standard strtol(): call strtol(buf, &eptr, 10), check errno for ERANGE, ensure the returned long is within 1..INT_MAX and cast to pid_t only if in range, and validate trailing characters so a newline is allowed (treat *eptr == '\n' or *eptr == '\0' as OK); remove the now-unused error variable if it becomes unused.
182-184:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix build: provide a glibc-safe
getprogname()forcompat/pidfile.c
compat/pidfile.ccallsgetprogname()(lines 182-184), but on the build path whereconfiguresets-DPIDFILE_LOCALand compilescompat/pidfile.c, it includes only"pidfile.h"(not<util.h>), and there’s no linkablegetprogname()available for that translation unit on glibc (the only othergetprogname()found isstaticinsrc/logerr.c, plus an inline stub incompat/setproctitle.hthat isn’t included here).
- Action: add a real
getprogname()shim (and header declaration) used bycompat/pidfile.c, or replace the call with a glibc-safe alternative (e.g.,program_invocation_short_name//proc/self/exebasename).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compat/pidfile.c` around lines 182 - 184, compat/pidfile.c calls getprogname() but that symbol isn’t available for that translation unit under glibc; either add a small shim function and header declaring getprogname() that compat/pidfile.c includes, or replace the call with a glibc-safe alternative: use the external program_invocation_short_name (from <errno.h>/<string.h> availability) or derive a basename from /proc/self/exe; implement the shim as a non-static getprogname() that returns a const char* and ensure its prototype is visible to compat/pidfile.c (or update pidfile.c to use program_invocation_short_name or a /proc/self/exe basename retrieval instead of getprogname()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@compat/pidfile.c`:
- Around line 302-307: The code uses pf_path = strdup(path) without checking for
NULL; if strdup fails pf_path will be NULL while pf_fd stays valid and
pidfile_clean() may call unlink(pf_path) on NULL. Update the logic around strdup
in the block handling (path vs dpath) to detect a NULL return from strdup(path),
handle the error by cleaning up the open descriptor (pf_fd) and any owned dpath
(set pf_fd to an invalid value or close it and free dpath), ensure pf_path is
left in a safe state (non-NULL or handled so pidfile_clean() won't call unlink
on NULL), and propagate/return an error as appropriate; reference the symbols
pf_path, pf_fd, pidfile_clean(), strdup(), dpath, and path to locate the change.
- Around line 79-81: The code uses the nonportable NetBSD/OpenBSD strtoi() in
compat/pidfile.c (the pid = (pid_t)strtoi(buf, &eptr, 10, 1, INT_MAX, &error);
block); replace that call with standard strtol(): call strtol(buf, &eptr, 10),
check errno for ERANGE, ensure the returned long is within 1..INT_MAX and cast
to pid_t only if in range, and validate trailing characters so a newline is
allowed (treat *eptr == '\n' or *eptr == '\0' as OK); remove the now-unused
error variable if it becomes unused.
- Around line 182-184: compat/pidfile.c calls getprogname() but that symbol
isn’t available for that translation unit under glibc; either add a small shim
function and header declaring getprogname() that compat/pidfile.c includes, or
replace the call with a glibc-safe alternative: use the external
program_invocation_short_name (from <errno.h>/<string.h> availability) or derive
a basename from /proc/self/exe; implement the shim as a non-static getprogname()
that returns a const char* and ensure its prototype is visible to
compat/pidfile.c (or update pidfile.c to use program_invocation_short_name or a
/proc/self/exe basename retrieval instead of getprogname()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fb0e453-9b71-4e39-bcce-445d1d3b5c2b
📒 Files selected for processing (3)
compat/pidfile.ccompat/pidfile.hconfigure
🚧 Files skipped from review as they are similar to previous changes (2)
- compat/pidfile.h
- configure
|
@coderabbitai full review combat/pidfile.c |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat compat/pidfile.c
echo "===HEADER==="
cat compat/pidfile.h
echo "===DHCPCD_C==="
cat src/dhcpcd.c
echo "===PRIVSEP_C==="
cat src/privsep.cLength of output: 103851
Overall assessmentThe implementation is a clean, well-structured port of the NetBSD pidfile from 1. 🔴
|
| Area | Assessment |
|---|---|
pf_fd != -1 ⟹ pf_path != NULL invariant |
✅ Maintained in all paths of pidfile_lock() |
pidfile_clean() calls pidfile_unlock() after using pf_path in unlink() |
✅ No use-after-free |
Double pidfile_clean() via atexit is safe |
✅ Second call sees pf_fd == -1, returns -1/EBADF harmlessly |
pidfile_lock() re-entry after fork (rewrite PID) |
✅ Correctly skips open() block, rewrites via ftruncate/lseek/dprintf |
pf_removeable = false skips unlink in pidfile_clean() |
✅ Short-circuit && correctly bypasses unlink |
buf[16] in pidfile_readfd() |
✅ Sufficient for INT_MAX PID (10 digits + \n + \0 = 12 bytes) |
TOCTOU between open() and flock() on non-O_EXLOCK systems |
✅ Known inherent limitation of the flock approach; accepted in practice |
File mode 0644 |
✅ Standard for pidfiles; allows unprivileged readers |
registered_atexit static local in pidfile_lock() |
✅ Safe for single-threaded dhcpcd eloop model |
Summary
| Severity | Issue |
|---|---|
| 🔴 Blocker | strtoi() unavailable in glibc — compilation failure when PIDFILE_LOCAL is set on Linux |
| 🟡 Minor | pidfile_clean() comment understates the return condition |
| 🟡 Minor | getprogname() portability on non-BSD PIDFILE_LOCAL targets |
1 participant
Adapt code, chroot and sandboxing is more readily supported.
ctx->pidfile is now malloced rather than a fixed size.