Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR removes the separate unprivileged control socket, uses peer credentials for privilege classification, changes control messaging to ChangesUnified control socket and privsep privilege flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Control as control.c
participant Root as privsep-root.c
participant Proxy as privsep-control.c
Client->>Control: send control command
Control->>Control: recvmsg and extract uid/gid
Control->>Root: ps_root_user_ispriv(uid, gid)
Root-->>Control: privilege result
Control->>Proxy: ps_ctl_sendmsg(msghdr)
Proxy-->>Client: forward response or listen data
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dhcpcd.c (1)
2570-2576: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDuplicate "spawned manager process" log.
The same
logdebugx(...)now appears both before thestart_manager:label (line 2572) and after it (line 2576). On the daemonise fall-through path both fire, logging the manager PID twice; thegoto start_managerpath only hits 2576. Drop the pre-label copy so both paths log exactly once.🩹 Remove the duplicate
/* We have now forked, setsid, forked once more. * From this point on, we are the controlling daemon. */ - logdebugx("spawned manager process on PID %d", (int)getpid()); start_manager: logdebugx("spawned manager process on PID %d", (int)getpid());🤖 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 `@src/dhcpcd.c` around lines 2570 - 2576, The manager startup debug message is duplicated around the start_manager label in dhcpcd.c, causing the daemonise fall-through path to log the same PID twice. Remove the pre-label logdebugx call and keep the single logdebugx("spawned manager process on PID %d", ...) after start_manager so both the fall-through and goto start_manager paths emit the message exactly once.
🧹 Nitpick comments (4)
src/privsep-root.c (1)
328-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant reassignment of
p.
pis already initialized todataat Line 328, so thep = data;at Line 335 is redundant. Minor cleanup.♻️ Cleanup
- uint8_t *p = data; + uint8_t *p; if (len != sizeof(uid) + sizeof(gid)) { errno = EINVAL; return -1; } p = data;🤖 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 `@src/privsep-root.c` around lines 328 - 338, Remove the redundant reassignment of the local pointer in the privilege-separation parsing logic: in the function that reads `uid` and `gid` from `data`, `p` is already initialized before the length check, so the later `p = data;` is unnecessary. Keep the existing `memcpy` flow intact and simply delete the duplicate assignment while preserving the `uid`/`gid` extraction behavior.src/privsep-control.c (1)
40-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
PS_CTL_FLAGS_PRIVmacro
PS_CTL_FLAGS_PRIVis never referenced; privilege is already carried byPS_CTL_PRIV/PS_CTLinps_ctl_dispatch, so this macro can be dropped.🤖 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 `@src/privsep-control.c` around lines 40 - 42, Remove the unused PS_CTL_FLAGS_PRIV macro from privsep-control.c and keep the PS_CTL_FD helper intact. Verify that ps_ctl_dispatch and the existing PS_CTL_PRIV/PS_CTL macros still provide the privilege handling needed, and delete any related dead definition so the header/source only contains symbols that are actually referenced.src/control.c (2)
453-453: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
listen()backlog usessizeof(ctx->control_fds).
ctx->control_fdsis a TAILQ head, sosizeofyields the struct size (~16), not a meaningful connection backlog. It works but is misleading; consider a named constant.🤖 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 `@src/control.c` at line 453, The backlog passed to listen() in the control socket setup uses sizeof(ctx->control_fds), which is just the TAILQ head size and not a meaningful queue limit. Update the listen() call in the control initialization path to use a clear named constant or defined backlog value instead of sizeof(ctx->control_fds), keeping the existing fd/control_fds logic unchanged.
434-454: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse the
SOCK_MODEmacro and confirm the world-writable socket is intended.
SOCK_MODEis defined at line 434 (same value,0666) butcontrol_start1()chmods with the literal0666, leaving the macro unused. Static analysis also flags the world-writable mode (CWE-732). Under the new credential model any user may connect and privilege is enforced viagetpeereid/FD_PRIV, so world-writable is plausibly intentional — but then thectx->control_groupchown no longer restricts who can connect. Please confirm0666is desired rather than0660+control_group, and either way replace the literal with the macro.♻️ Use the defined macro
if (bind(fd, (struct sockaddr *)&sa, len) == -1 || - chmod(sa.sun_path, 0666) == -1 || + chmod(sa.sun_path, SOCK_MODE) == -1 || (ctx->control_group && chown(sa.sun_path, geteuid(), ctx->control_group) == -1) || listen(fd, sizeof(ctx->control_fds)) == -1)🤖 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 `@src/control.c` around lines 434 - 454, In control_start1(), replace the hardcoded chmod mode with the existing SOCK_MODE macro so the defined socket permissions are used consistently. Also verify the intended access model for the control socket: if world-writable access is deliberate under the new credential/FD_PRIV checks, keep SOCK_MODE and leave a note or related adjustment; otherwise tighten the mode to a group-only setting and ensure ctx->control_group still meaningfully restricts access.Source: Linters/SAST tools
🤖 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 `@src/control.c`:
- Around line 642-676: In control_user_ispriv(), treat an unknown uid from
getpwuid() as non-privileged instead of returning -1, so control_handle_data()
does not drop read-only clients whose user is missing from the passwd database.
Update the early return path in control_user_ispriv() to return 0 for this case,
and make sure any error logging in the caller does not rely on stale errno from
getpwuid() when no real error occurred.
In `@src/privsep-control.c`:
- Around line 111-123: The PS_CTL_PRIV/PS_CTL handling in privsep control is
incorrectly closing the shared proxy work fd when control_recvmsg() fails.
Update the control_new()/control_free() flow so the wrapper is dropped without
closing ctx->ps_ctl->psp_work_fd on the error path, and keep the underlying
shared fd registered for later dispatches; use the PS_CTL_PRIV/PS_CTL case block
and the control_recvmsg() cleanup branch as the places to adjust.
---
Outside diff comments:
In `@src/dhcpcd.c`:
- Around line 2570-2576: The manager startup debug message is duplicated around
the start_manager label in dhcpcd.c, causing the daemonise fall-through path to
log the same PID twice. Remove the pre-label logdebugx call and keep the single
logdebugx("spawned manager process on PID %d", ...) after start_manager so both
the fall-through and goto start_manager paths emit the message exactly once.
---
Nitpick comments:
In `@src/control.c`:
- Line 453: The backlog passed to listen() in the control socket setup uses
sizeof(ctx->control_fds), which is just the TAILQ head size and not a meaningful
queue limit. Update the listen() call in the control initialization path to use
a clear named constant or defined backlog value instead of
sizeof(ctx->control_fds), keeping the existing fd/control_fds logic unchanged.
- Around line 434-454: In control_start1(), replace the hardcoded chmod mode
with the existing SOCK_MODE macro so the defined socket permissions are used
consistently. Also verify the intended access model for the control socket: if
world-writable access is deliberate under the new credential/FD_PRIV checks,
keep SOCK_MODE and leave a note or related adjustment; otherwise tighten the
mode to a group-only setting and ensure ctx->control_group still meaningfully
restricts access.
In `@src/privsep-control.c`:
- Around line 40-42: Remove the unused PS_CTL_FLAGS_PRIV macro from
privsep-control.c and keep the PS_CTL_FD helper intact. Verify that
ps_ctl_dispatch and the existing PS_CTL_PRIV/PS_CTL macros still provide the
privilege handling needed, and delete any related dead definition so the
header/source only contains symbols that are actually referenced.
In `@src/privsep-root.c`:
- Around line 328-338: Remove the redundant reassignment of the local pointer in
the privilege-separation parsing logic: in the function that reads `uid` and
`gid` from `data`, `p` is already initialized before the length check, so the
later `p = data;` is unnecessary. Keep the existing `memcpy` flow intact and
simply delete the duplicate assignment while preserving the `uid`/`gid`
extraction behavior.
🪄 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: 0f1cec4a-8cc9-4b2c-99f5-c3d964783ef1
📒 Files selected for processing (11)
src/control.csrc/control.hsrc/dhcpcd.8.insrc/dhcpcd.csrc/dhcpcd.hsrc/privsep-control.csrc/privsep-control.hsrc/privsep-root.csrc/privsep-root.hsrc/privsep.csrc/privsep.h
Instead workout credentials from the connected socket by getpeereid(2). Remove the limit of only one connected control client. This needs some testing and is only expected to compile on the BSDs right now.
Add --isprivileged to return either true or false so the caller can work this out as we now just have the one socket. Bump version for this.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/privsep-control.c`:
- Around line 178-181: Update the recvmsg error handling around msglen in the
control proxy to detect EAGAIN, EWOULDBLOCK, and EINTR and return to the event
loop without calling eloop_exit for those recoverable failures. Preserve the
existing log-and-exit behavior for all other recvmsg errors.
- Around line 259-262: Update the socketpair creation in the privsep control
setup to preserve non-blocking behavior for both work_fd and listen_fd by
restoring the non-blocking socket flag alongside SOCK_CLOEXEC, unless
xsocketpair or the target platform already guarantees it. Keep both descriptors
suitable for use by the eloop event loop.
- Line 97: Replace the unbounded strcmp check for "--isprivileged" in the
control request handling branch with a length-bounded comparison using the
received len value, matching the surrounding bounded-payload logic and
preventing reads beyond the client buffer.
- Around line 211-230: Update the control-message reads in the receive path
around ps_ctl_listen() so they handle partial length-prefix and payload reads
without stalling the event loop: preserve non-blocking socket setup in
ps_ctl_start() using SOCK_CXNB, or replace these reads with recv(...,
MSG_WAITALL) while retaining appropriate EOF/error handling.
🪄 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: 33712ca1-d250-4d7c-b5ba-e2bded8f15d6
📒 Files selected for processing (5)
src/control.csrc/control.hsrc/defs.hsrc/dhcpcd.csrc/privsep-control.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/control.c
| if (msglen == -1) { | ||
| logerr("%s: read", __func__); | ||
| eloop_exit(ctx->eloop, EXIT_FAILURE); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle non-fatal errors from recvmsg.
If recvmsg fails with EAGAIN, EWOULDBLOCK, or EINTR, terminating the control proxy process via eloop_exit is overly aggressive. This is especially problematic if the socket is reverted to non-blocking mode, where EAGAIN is expected.
Consider ignoring these recoverable errors so the event loop can retry.
🛠️ Proposed fix
if (msglen == -1) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR)
+ return;
logerr("%s: read", __func__);
eloop_exit(ctx->eloop, EXIT_FAILURE);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (msglen == -1) { | |
| logerr("%s: read", __func__); | |
| eloop_exit(ctx->eloop, EXIT_FAILURE); | |
| } | |
| if (msglen == -1) { | |
| if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) | |
| return; | |
| logerr("%s: read", __func__); | |
| eloop_exit(ctx->eloop, EXIT_FAILURE); | |
| } |
🤖 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 `@src/privsep-control.c` around lines 178 - 181, Update the recvmsg error
handling around msglen in the control proxy to detect EAGAIN, EWOULDBLOCK, and
EINTR and return to the event loop without calling eloop_exit for those
recoverable failures. Preserve the existing log-and-exit behavior for all other
recvmsg errors.
| if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, work_fd) == | ||
| -1 || | ||
| xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, listen_fd) == | ||
| -1) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Retain non-blocking behavior for event-loop file descriptors.
Replacing SOCK_CXNB with SOCK_STREAM | SOCK_CLOEXEC drops the SOCK_NONBLOCK flag (assuming SOCK_CXNB provided it). Since work_fd and listen_fd are added to the eloop (lines 292 and 299), they should remain non-blocking to prevent any read() or recvmsg() calls from stalling the single-threaded event loop.
Unless SOCK_CLOEXEC natively implies non-blocking on this target, please restore the non-blocking flag.
🛠️ Proposed fix
- if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, work_fd) ==
- -1 ||
- xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, listen_fd) ==
- -1)
+ if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CXNB, 0, work_fd) ==
+ -1 ||
+ xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CXNB, 0, listen_fd) ==
+ -1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, work_fd) == | |
| -1 || | |
| xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, listen_fd) == | |
| -1) | |
| if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CXNB, 0, work_fd) == | |
| -1 || | |
| xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CXNB, 0, listen_fd) == | |
| -1) |
🤖 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 `@src/privsep-control.c` around lines 259 - 262, Update the socketpair creation
in the privsep control setup to preserve non-blocking behavior for both work_fd
and listen_fd by restoring the non-blocking socket flag alongside SOCK_CLOEXEC,
unless xsocketpair or the target platform already guarantees it. Keep both
descriptors suitable for use by the eloop event loop.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Instead workout credentials from the connected socket by getpeereid(2).
Remove the limit of only one connected control client.
This needs some testing and is only expected to compile on the BSDs right now.