Conversation
macOS does not support SOCK_SEQPACKET. All our messages use a fixed header which includes the lengths of all parts sent. We can use these limits with MSG_WAITALL on blocking sockets in place of MSG_EOR to get the same effect. Start of the work for #524.
|
ℹ️ 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)
WalkthroughIPC between dhcpcd components was migrated from packet-oriented SOCK_SEQPACKET with MSG_EOR boundaries to stream-oriented SOCK_STREAM with explicit length-prefixed framing; send/receive code, log framing, and socketpair creation were updated accordingly across privsep, root, dhcpcd, and logging code paths. ChangesIPC Stream Migration
Sequence Diagram(s)sequenceDiagram
autonumber
participant Launcher
participant Manager
participant PrivsepRoot
participant LogReader
Launcher->>Manager: open SOCK_STREAM socketpair (CLOEXEC)
Manager->>PrivsepRoot: send framed header (ps_msghdr) + payload (sendmsg flags=0)
PrivsepRoot->>PrivsepRoot: recv header with MSG_WAITALL, read payload, validate
PrivsepRoot->>LogReader: send log (pri,pid,mlen,payload) over stream
LogReader->>PrivsepRoot: drain on shutdown (poll/read until EOF)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/privsep.c (1)
1069-1131:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftUnbounded payload read into a fixed stack buffer.
ps_dataisuint8_t ps_data[PS_BUFLEN], butdlen(Line 1121) is computed from peer-supplied fields (ps_namelen + ps_controllen + cmsg_padlen + ps_datalen) and then passed straight torecv(fd, ps_data, dlen, MSG_WAITALL)without checkingdlen <= sizeof(ps_data). Under the previousSOCK_SEQPACKET,recvmsgwould have surfaced oversized messages viaMSG_TRUNC; with the newSOCK_STREAM+MSG_WAITALLframing, any peer (or buggy local sender) that reports adlenlarger thanPS_BUFLENoverruns the stack buffer.Two smaller issues in the same block:
- When
recvreturns-1, the subsequenterrno = EINVALoverwrites the real error cause from the kernel.- The arithmetic forming
dlenissocklen_t + socklen_t + socklen_t + size_t; on platforms where the sum can overflowsize_tan attacker-trusted-but-buggy peer could pass the bounds check trivially.🛡️ Suggested guards
- cmsg_padlen = CALC_CMSG_PADLEN(psm.ps_controllen, - psm.ps_namelen); - dlen = psm.ps_namelen + psm.ps_controllen + cmsg_padlen + psm.ps_datalen; - if (dlen != 0) { - len = recv(fd, ps_data, dlen, MSG_WAITALL); - if ((size_t)len != dlen) { - errno = EINVAL; - return -1; - } - } + cmsg_padlen = CALC_CMSG_PADLEN(psm.ps_controllen, + psm.ps_namelen); + dlen = (size_t)psm.ps_namelen + (size_t)psm.ps_controllen + + (size_t)cmsg_padlen + psm.ps_datalen; + if (dlen > sizeof(ps_data)) { + errno = EMSGSIZE; + return -1; + } + if (dlen != 0) { + len = recv(fd, ps_data, dlen, MSG_WAITALL); + if (len == -1) + return -1; + if ((size_t)len != dlen) { + errno = EINVAL; + return -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.c` around lines 1069 - 1131, The code reads a peer-controlled dlen into a fixed stack buffer ps_data[PS_BUFLEN] without bounds or overflow checks and clobbers real errno on recv failure; fix by validating and capping/calculating dlen safely before any recv: compute each component (ps_namelen, ps_controllen, ps_datalen, cmsg_padlen) as size_t and check each addition for overflow (e.g., ensure running_sum + next <= PS_BUFLEN and detect overflow by comparing running_sum + next >= running_sum), then ensure final dlen <= PS_BUFLEN before calling recv into ps_data; when recv returns -1, do not overwrite errno (propagate/return -1 immediately), and only set errno = EINVAL when recv succeeds but returns an unexpected short read; adjust checks around the recv(fd, ps_data, dlen, MSG_WAITALL) and references to CALC_CMSG_PADLEN/ps_unrollmsg/ps_data/PS_BUFLEN accordingly.src/privsep-root.c (1)
94-117:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftTwo regressions in the new
MSG_WAITALLread path.Now that the channel is
SOCK_STREAM, the code reads exactlypsr_error->psr_datalenbytes regardless of the caller's buffer — the previousSOCK_SEQPACKETframing implicitly truncated oversized payloads. There are two issues:
- Line 105: missing NULL check after
malloc. Ifmallocfails,pc->psr_dataisNULLand the subsequentrecv(fd, NULL, ...)is undefined behaviour. The error path aterror:also dereferencespc->psr_dataonly for free, so the failure is silently masked.- Line 107: no bound check against the caller-provided buffer in the non-malloc path. When
pc->psr_mallocdata == false,pc->psr_datalenis the caller's capacity (e.g.ps_root_readerror(ctx, NULL, 0)orps_root_filemtime'ssizeof(time_t)). A peer that reports a largerpsr_datalenwill overrun the caller's buffer (or be written throughNULL).🛡️ Suggested guard
if (psr_error->psr_datalen == 0) return len; - if (pc->psr_mallocdata) + if (pc->psr_mallocdata) { pc->psr_data = malloc(psr_error->psr_datalen); + if (pc->psr_data == NULL) + PSR_ERROR(errno); + } else if (psr_error->psr_datalen > pc->psr_datalen || + pc->psr_data == NULL) { + PSR_ERROR(EMSGSIZE); + } len = recv(fd, pc->psr_data, psr_error->psr_datalen, MSG_WAITALL);🤖 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 94 - 117, The recv path must validate allocation and avoid overrunning caller buffers: after seeing psr_error->psr_datalen > 0, if pc->psr_mallocdata is true call malloc and immediately check the return and handle OOM (log/return error) before calling recv; only free pc->psr_data on the error path if malloc actually succeeded. If pc->psr_mallocdata is false, do not blindly recv psr_error->psr_datalen into pc->psr_data — instead compare the incoming psr_error->psr_datalen to the caller-provided capacity stored on pc (the field used when not mallocing) and only read up to that capacity; if the peer claims more bytes than the caller can hold, either return an error (EBADMSG) and consume/discard the excess bytes from the socket to keep the stream in sync, or read the excess into a temporary discard buffer before returning an error. Update the code paths around recv, pc->psr_data, psr_error->psr_datalen and pc->psr_mallocdata accordingly.src/dhcpcd.c (1)
2787-2790:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop
MSG_EORflag from stream socket send — inconsistent with migration.
ctx.fork_fdisSOCK_STREAM(line 2472), and the two other send calls on it (lines 387, 1501) use flags0. The recent migration toSOCK_STREAMreplacedMSG_EOR(appropriate forSOCK_SEQPACKETonly) withMSG_WAITALLor flags0. This call at line 2788 was missed during the migration and will spuriously log send errors on platforms that rejectMSG_EORfor stream sockets per POSIX semantics.Proposed fix
`#ifdef` USE_SIGNALS /* If still attached, detach from the launcher */ if (ctx.options & DHCPCD_STARTED && ctx.fork_fd != -1) { - if (send(ctx.fork_fd, &i, sizeof(i), MSG_EOR) == -1) + if (send(ctx.fork_fd, &i, sizeof(i), 0) == -1) logerr("%s: send", __func__); } `#endif`🤖 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 2787 - 2790, The send call that writes to ctx.fork_fd uses MSG_EOR which is only valid for datagram/seqpacket sockets; since ctx.fork_fd is a SOCK_STREAM and other sends to it (in functions/locations using ctx.fork_fd) pass flags 0, change this send(...) invocation to use flags 0 (i.e., remove MSG_EOR) so it matches the other stream-socket sends and avoids spurious send errors; update the send invocation around the code using ctx.fork_fd and the surrounding error handling (logerr("%s: send", __func__)) remains the same.src/logerr.c (1)
218-242:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
MSG_EORis incompatible withSOCK_STREAMand there is a critical stack buffer overflow in the receive path.
src/logerr.hline 79 describes this channel asSOCK_STREAM, created insrc/privsep-root.cline 834 withSOCK_STREAM | SOCK_CLOEXEC. Per POSIX,MSG_EORis defined forSOCK_SEQPACKET; Linux will reject it withEOPNOTSUPPonSOCK_STREAMsockets. Since the receive path already uses explicitmlenframing, removeMSG_EOR.More critically, there is a stack buffer overflow at
src/logerr.cline 436. The sender clampsmlentosizeof(buf)whenmlen > sizeof(buf), but the receiver only rejects whenmlen > sizeof(buf)(line 422), allowingmlen == sizeof(buf)to pass through. Whenmlen == LOGERR_SYSLOGBUF(1036), the linebuf[mlen] = '\0'writes one past the end of the 1036-byte buffer, corrupting the stack.Fix line 422 to reject
mlen >= sizeof(buf), or alternatively adjust the sender to use>=in the clamp check somlenstays strictly less than buffer size.Proposed fixes
Option A (receiver validation):
- if (mlen > sizeof(buf)) { + if (mlen >= sizeof(buf)) {Option B (sender clamping, also fixes MSG_EOR):
- if (mlen > sizeof(buf)) + if (mlen >= sizeof(buf)) mlen = sizeof(buf); - len = (int)sendmsg(ctx->log_fd, &msg, MSG_EOR); + len = (int)sendmsg(ctx->log_fd, &msg, 0);🤖 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/logerr.c` around lines 218 - 242, In the send path (the block using ctx->log_fd, buf, mlen and calling sendmsg) remove the MSG_EOR flag passed to sendmsg (MSG_EOR is invalid for SOCK_STREAM) and ensure the sender still clamps mlen correctly; and in the receive path fix the off-by-one validation by changing the mlen check from "mlen > sizeof(buf)" to "mlen >= sizeof(buf)" (so mlen equal to LOGERR_SYSLOGBUF is rejected), preventing the buf[mlen] = '\0' overflow; reference sendmsg/MSG_EOR, LOGERR_SYSLOGBUF, the local buf and mlen variables to locate the changes.
🧹 Nitpick comments (1)
src/privsep-root.c (1)
925-949: 💤 Low valueLog-drain on shutdown looks reasonable; minor note.
Using
poll()with a 1 ms timeout works around the blocking socket without adding non-blocking state. Two small things to consider: a)poll()may return-1withEINTRand would silently terminate the drain — acceptable on a shutdown path but worth a comment; b) the loop has no upper bound on iterations, so a misbehaving peer that keeps writing log records could delay shutdown. Given this only runs in the privileged-root shutdown path with a trusted peer, both are minor.🤖 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 925 - 949, The poll loop draining ctx->ps_log_root_fd can prematurely exit on poll() returning -1 due to EINTR and currently has no upper bound on iterations; update the loop around poll(&pfd,1,1) in the shutdown path that calls logreadfd so that if poll() returns -1 and errno == EINTR it continues (rather than breaking), and add an iteration cap or overall timeout (e.g., max_loops counter or deadline checked with clock_gettime) to ensure the loop cannot spin indefinitely if the peer keeps writing; after the cap/timeout, fall back to closing ctx->ps_log_root_fd as the current cleanup does.
🤖 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/logerr.c`:
- Around line 422-436: The code allows mlen == sizeof(buf) leading to buf[mlen]
= '\0' writing one byte past the stack buffer (buf is char
buf[LOGERR_SYSLOGBUF]) after recv; fix by either rejecting that case (change the
earlier check from mlen > sizeof(buf) to mlen >= sizeof(buf)) so recv never
returns a full-size length, or (if you want to accept full-size payloads) change
the defensive NUL write to set buf[mlen-1] = '\0' but only after verifying mlen
> 0; update the logic around recv, mlen and the NUL-termination to avoid
out-of-bounds writes (references: buf, LOGERR_SYSLOGBUF, mlen, recv).
---
Outside diff comments:
In `@src/dhcpcd.c`:
- Around line 2787-2790: The send call that writes to ctx.fork_fd uses MSG_EOR
which is only valid for datagram/seqpacket sockets; since ctx.fork_fd is a
SOCK_STREAM and other sends to it (in functions/locations using ctx.fork_fd)
pass flags 0, change this send(...) invocation to use flags 0 (i.e., remove
MSG_EOR) so it matches the other stream-socket sends and avoids spurious send
errors; update the send invocation around the code using ctx.fork_fd and the
surrounding error handling (logerr("%s: send", __func__)) remains the same.
In `@src/logerr.c`:
- Around line 218-242: In the send path (the block using ctx->log_fd, buf, mlen
and calling sendmsg) remove the MSG_EOR flag passed to sendmsg (MSG_EOR is
invalid for SOCK_STREAM) and ensure the sender still clamps mlen correctly; and
in the receive path fix the off-by-one validation by changing the mlen check
from "mlen > sizeof(buf)" to "mlen >= sizeof(buf)" (so mlen equal to
LOGERR_SYSLOGBUF is rejected), preventing the buf[mlen] = '\0' overflow;
reference sendmsg/MSG_EOR, LOGERR_SYSLOGBUF, the local buf and mlen variables to
locate the changes.
In `@src/privsep-root.c`:
- Around line 94-117: The recv path must validate allocation and avoid
overrunning caller buffers: after seeing psr_error->psr_datalen > 0, if
pc->psr_mallocdata is true call malloc and immediately check the return and
handle OOM (log/return error) before calling recv; only free pc->psr_data on the
error path if malloc actually succeeded. If pc->psr_mallocdata is false, do not
blindly recv psr_error->psr_datalen into pc->psr_data — instead compare the
incoming psr_error->psr_datalen to the caller-provided capacity stored on pc
(the field used when not mallocing) and only read up to that capacity; if the
peer claims more bytes than the caller can hold, either return an error
(EBADMSG) and consume/discard the excess bytes from the socket to keep the
stream in sync, or read the excess into a temporary discard buffer before
returning an error. Update the code paths around recv, pc->psr_data,
psr_error->psr_datalen and pc->psr_mallocdata accordingly.
In `@src/privsep.c`:
- Around line 1069-1131: The code reads a peer-controlled dlen into a fixed
stack buffer ps_data[PS_BUFLEN] without bounds or overflow checks and clobbers
real errno on recv failure; fix by validating and capping/calculating dlen
safely before any recv: compute each component (ps_namelen, ps_controllen,
ps_datalen, cmsg_padlen) as size_t and check each addition for overflow (e.g.,
ensure running_sum + next <= PS_BUFLEN and detect overflow by comparing
running_sum + next >= running_sum), then ensure final dlen <= PS_BUFLEN before
calling recv into ps_data; when recv returns -1, do not overwrite errno
(propagate/return -1 immediately), and only set errno = EINVAL when recv
succeeds but returns an unexpected short read; adjust checks around the recv(fd,
ps_data, dlen, MSG_WAITALL) and references to
CALC_CMSG_PADLEN/ps_unrollmsg/ps_data/PS_BUFLEN accordingly.
---
Nitpick comments:
In `@src/privsep-root.c`:
- Around line 925-949: The poll loop draining ctx->ps_log_root_fd can
prematurely exit on poll() returning -1 due to EINTR and currently has no upper
bound on iterations; update the loop around poll(&pfd,1,1) in the shutdown path
that calls logreadfd so that if poll() returns -1 and errno == EINTR it
continues (rather than breaking), and add an iteration cap or overall timeout
(e.g., max_loops counter or deadline checked with clock_gettime) to ensure the
loop cannot spin indefinitely if the peer keeps writing; after the cap/timeout,
fall back to closing ctx->ps_log_root_fd as the current cleanup does.
🪄 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: 4e13c450-2f8b-4b87-97a6-bfa472c135f8
📒 Files selected for processing (8)
src/dhcpcd.csrc/logerr.csrc/logerr.hsrc/privsep-bpf.csrc/privsep-control.csrc/privsep-inet.csrc/privsep-root.csrc/privsep.c
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 (2)
src/privsep.c (1)
904-913:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the iovec bounds check before it writes past
iov[].The guard is off by one. When
m.msg_iovlenreaches__arraycount(iov), the>check still passes andiovp->iov_basewrites toiov[7], which is out of bounds.💡 Suggested fix
for (i = 0; i < (int)msg->msg_iovlen; i++) { - if ((size_t)(m.msg_iovlen++) > __arraycount(iov)) { + if ((size_t)m.msg_iovlen >= __arraycount(iov)) { errno = ENOBUFS; return -1; } + m.msg_iovlen++; iovp->iov_base = msg->msg_iov[i].iov_base; iovp->iov_len = msg->msg_iov[i].iov_len; iovp++; psm->ps_datalen += msg->msg_iov[i].iov_len; }🤖 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.c` around lines 904 - 913, The iovec bounds check is off-by-one and lets writes past the iov[] buffer; in the loop that copies msg->msg_iov into the local iov (using m.msg_iovlen, iovp and ps_datalen) replace the current check that uses post-increment (m.msg_iovlen++) and ">" with a pre-check like "if ((size_t)m.msg_iovlen >= __arraycount(iov)) { errno = ENOBUFS; return -1; }" and then increment m.msg_iovlen only after the check (or change the comparison to ">=" without post-increment) so you never write to iov out of bounds before returning.src/privsep-root.c (1)
183-195:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSend the full root reply frame, not just the first fragment.
Both
sendmsgcalls here assume a single write completes the whole header+payload reply. OnSOCK_STREAM, a short write is legal; if it happens, the caller side inps_root_readerrorcb()can block indefinitely or lose framing because it reads the header and payload withMSG_WAITALL. Please wrap this in a send-all helper that advances the iovecs until every byte is written.🤖 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 183 - 195, The current sendmsg usage in functions around sendmsg(fd, &msg, 0) (including the error-reply path that sets psr.psr_result/psr_errno and calls sendmsg again) assumes a single write will send the entire iovec; on SOCK_STREAM a short write can occur and breaks framing in ps_root_readerrorcb(). Fix by creating and calling a send_all helper (e.g. sendmsg_all) that loops until all bytes in msg.msg_iov/msg.msg_iovlen are consumed: call sendmsg repeatedly, advance the iovecs (adjust iov_base and iov_len or skip finished iov entries and decrement msg.msg_iovlen), handle EINTR/EAGAIN by retrying, propagate EPIPE/other errors back, and replace both direct sendmsg(fd,&msg,0) calls with this helper so the full root reply frame (header+payload) is always transmitted.
🤖 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/dhcpcd.c`:
- Around line 387-388: The three uses of send that write an int (e.g.,
send(ctx->fork_fd, &exit_code, sizeof(exit_code), 0)) can perform short writes
and must be replaced with a helper that loops until all bytes are sent; add a
send_int_full(int fd, int value) helper that writes sizeof(int) bytes (handling
EINTR and partial writes, returning error on failure) and use it wherever the
code currently calls send for an int (the spots writing exit_code via
ctx->fork_fd and the other two send sites referenced in the review). Ensure
callers check the helper's return and call logerr(__func__) on failure as
before.
In `@src/logerr.c`:
- Around line 235-240: The sendmsg call can short-write on SOCK_STREAM and must
be wrapped in a write-all loop: after building struct iovec iov[] and struct
msghdr msg (as shown), repeatedly call sendmsg(ctx->log_fd, &msg, 0) until the
total bytes sent equals the full framed record length; on each partial write
advance the iovecs (adjust iov[i].iov_base and iov[i].iov_len or reduce
msg.msg_iovlen) to reflect remaining data, retry on EINTR, handle EAGAIN
appropriately, and only return once the entire record is written so logreadfd
(which uses MSG_WAITALL) stays in sync.
- Around line 231-234: The sender sets mlen = (size_t)len + 1 after vsnprintf,
but when vsnprintf returns sizeof(buf)-1 this yields mlen == sizeof(buf) which
the receiver rejects; fix by capping mlen to sizeof(buf)-1 instead of
sizeof(buf). Update the logic around vsnprintf/len/mlen (the code that assigns
mlen and iov[3].iov_len) so that after computing mlen = (size_t)len + 1 you do
if (mlen > sizeof(buf) - 1) mlen = sizeof(buf) - 1; then assign iov[3].iov_len =
mlen to keep the max log-frame length consistent with the receiver.
In `@src/privsep.c`:
- Line 916: The single sendmsg(fd, &m, 0) can result in a short write on the
SOCK_STREAM and desynchronize ps_recvpsmsg(); replace it with a full-write loop
that retries until the entire composed frame (the sum of bytes in m.msg_iov /
m.msg_iovlen) is written. On each iteration call sendmsg(fd, &m, 0), treat -1
with EINTR as a retry, treat other -1 as an error, and if sendmsg returns a
positive count less than the total advance the msghdr's iov pointer/lengths (or
rebuild m.msg_iov to reflect the consumed bytes) and subtract the written bytes,
then loop until all bytes are sent; keep using the same msghdr structure and fd
used at the current sendmsg call so ps_recvpsmsg() stays synchronized.
---
Outside diff comments:
In `@src/privsep-root.c`:
- Around line 183-195: The current sendmsg usage in functions around sendmsg(fd,
&msg, 0) (including the error-reply path that sets psr.psr_result/psr_errno and
calls sendmsg again) assumes a single write will send the entire iovec; on
SOCK_STREAM a short write can occur and breaks framing in ps_root_readerrorcb().
Fix by creating and calling a send_all helper (e.g. sendmsg_all) that loops
until all bytes in msg.msg_iov/msg.msg_iovlen are consumed: call sendmsg
repeatedly, advance the iovecs (adjust iov_base and iov_len or skip finished iov
entries and decrement msg.msg_iovlen), handle EINTR/EAGAIN by retrying,
propagate EPIPE/other errors back, and replace both direct sendmsg(fd,&msg,0)
calls with this helper so the full root reply frame (header+payload) is always
transmitted.
In `@src/privsep.c`:
- Around line 904-913: The iovec bounds check is off-by-one and lets writes past
the iov[] buffer; in the loop that copies msg->msg_iov into the local iov (using
m.msg_iovlen, iovp and ps_datalen) replace the current check that uses
post-increment (m.msg_iovlen++) and ">" with a pre-check like "if
((size_t)m.msg_iovlen >= __arraycount(iov)) { errno = ENOBUFS; return -1; }" and
then increment m.msg_iovlen only after the check (or change the comparison to
">=" without post-increment) so you never write to iov out of bounds before
returning.
🪄 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: 54385ecb-4854-47b0-89e6-583eddb0cc74
📒 Files selected for processing (4)
src/dhcpcd.csrc/logerr.csrc/privsep-root.csrc/privsep.c
| if (send(ctx->fork_fd, &exit_code, sizeof(exit_code), 0) == -1) | ||
| logerr(__func__); |
There was a problem hiding this comment.
Make the fork-control writes send the whole int.
Line 1908 now waits for exactly sizeof(int) bytes on a SOCK_STREAM socket. These three send sites still assume that one call writes the full value, but a short write is legal and would leave the launcher/manager channel out of sync during daemonise, signal forwarding, or shutdown. A tiny send_int_full() helper would make this robust.
Also applies to: 1501-1502, 2788-2789
🤖 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 387 - 388, The three uses of send that write an
int (e.g., send(ctx->fork_fd, &exit_code, sizeof(exit_code), 0)) can perform
short writes and must be replaced with a helper that loops until all bytes are
sent; add a send_int_full(int fd, int value) helper that writes sizeof(int)
bytes (handling EINTR and partial writes, returning error on failure) and use it
wherever the code currently calls send for an int (the spots writing exit_code
via ctx->fork_fd and the other two send sites referenced in the review). Ensure
callers check the helper's return and call logerr(__func__) on failure as
before.
| struct msghdr msg = { | ||
| .msg_iov = iov, | ||
| .msg_iovlen = sizeof(iov) / sizeof(iov[0]), | ||
| }; | ||
| len = (int)sendmsg(ctx->log_fd, &msg, MSG_EOR); | ||
| len = (int)sendmsg(ctx->log_fd, &msg, 0); | ||
| } |
There was a problem hiding this comment.
Write the entire framed log record before returning.
sendmsg on SOCK_STREAM can complete with a short write. If that happens here, logreadfd() will block or desynchronize because it now relies on exact framing with MSG_WAITALL. This path needs a write-all loop that advances the iovecs until the full record is sent.
🤖 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/logerr.c` around lines 235 - 240, The sendmsg call can short-write on
SOCK_STREAM and must be wrapped in a write-all loop: after building struct iovec
iov[] and struct msghdr msg (as shown), repeatedly call sendmsg(ctx->log_fd,
&msg, 0) until the total bytes sent equals the full framed record length; on
each partial write advance the iovecs (adjust iov[i].iov_base and iov[i].iov_len
or reduce msg.msg_iovlen) to reflect remaining data, retry on EINTR, handle
EAGAIN appropriately, and only return once the entire record is written so
logreadfd (which uses MSG_WAITALL) stays in sync.
| } | ||
|
|
||
| len = sendmsg(fd, &m, MSG_EOR); | ||
| len = sendmsg(fd, &m, 0); |
There was a problem hiding this comment.
The new stream transport needs a full-write loop here.
ps_recvpsmsg() now reconstructs frames by reading an exact header and then an exact payload. A short sendmsg on this SOCK_STREAM fd will silently truncate a message and permanently desynchronize the IPC channel. This helper needs to keep sending until the entire composed frame has been written.
🤖 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.c` at line 916, The single sendmsg(fd, &m, 0) can result in a
short write on the SOCK_STREAM and desynchronize ps_recvpsmsg(); replace it with
a full-write loop that retries until the entire composed frame (the sum of bytes
in m.msg_iov / m.msg_iovlen) is written. On each iteration call sendmsg(fd, &m,
0), treat -1 with EINTR as a retry, treat other -1 as an error, and if sendmsg
returns a positive count less than the total advance the msghdr's iov
pointer/lengths (or rebuild m.msg_iov to reflect the consumed bytes) and
subtract the written bytes, then loop until all bytes are sent; keep using the
same msghdr structure and fd used at the current sendmsg call so ps_recvpsmsg()
stays synchronized.
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 (1)
src/privsep.c (1)
1086-1133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle partial
recv()reads on stream IPC as fatal errors to prevent stream desynchronization.
recv(..., MSG_WAITALL)onSOCK_STREAMis not guaranteed to return the full requested length — it may return a short count if a signal is caught, the peer disconnects mid-record, or a socket error occurs. Two critical paths handle this incorrectly:
- Header (lines 1095–1098): A partial header read (
0 < len < sizeof(psm)) returns-1witherrno = EINVALwithout reaching thestop:label. The stream is now misaligned and the eloop callback will re-invoke on the next event, failing repeatedly.- Payload (lines 1128–1132): Same pattern. Additionally, when
recvreturns-1, the cast(size_t)len != dlensucceeds trivially, anderrnois clobbered byEINVAL, masking the real failure (ECONNRESET,EINTR, etc.).Both must jump to the existing
stop:label to triggereloop_exit()and tear down cleanly, and must preserve the originalerrnofromrecvon error.Suggested fix
len = recv(fd, &psm, sizeof(psm), MSG_WAITALL); - if (len == -1 || len == 0) + if (len <= 0) stop = true; else { dlen = (size_t)len; if (dlen < sizeof(psm)) { - errno = EINVAL; - return -1; + len = -1; + errno = EPROTO; + goto stop; } if (psm.ps_cmd == PS_STOP) { @@ -1127,9 +1131,11 @@ } len = recv(fd, ps_data, dlen, MSG_WAITALL); - if ((size_t)len != dlen) { - errno = EINVAL; + if (len == -1) + goto stop; /* preserve recv's errno */ + if ((size_t)len != dlen) { + len = -1; + errno = EPROTO; + goto stop; } }🤖 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.c` around lines 1086 - 1133, The code must treat any short or failed recv() on the stream IPC as a fatal error and jump to the existing stop: teardown path while preserving the original errno; change the header read path (recv(fd, &psm,...)) so that if 0 < len < sizeof(psm) you set a saved_errno to EINVAL (or preserve errno when recv returned -1) and goto stop instead of returning -1, and change the payload read path (recv(fd, ps_data, dlen,...)) so that if (size_t)len != dlen you capture errno into a local saved_errno when recv returned -1 or set saved_errno = EINVAL for a short read, assign errno = saved_errno and goto stop (do not return -1); refer to recv, psm, ps_data, stop:, ctx, and eloop_exit when making the changes.
♻️ Duplicate comments (2)
src/logerr.c (1)
235-240:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftShort-write on
sendmsgstill not handled.
sendmsgonSOCK_STREAMmay return fewer bytes than the composed iov total (especially under load or when the peer's socket buffer fills). The receiver inlogreadfdnow relies on exact[pri][pid][mlen]header framing followed by a separate payloadrecvwithMSG_WAITALL; a single short write here will permanently desynchronize the log channel — the nextrecvmsgwill read part of the previous payload as the next header. A write-all loop that advances iov bases/lengths until the full record is sent (and retries onEINTR) is required for stream IPC.🐛 Sketch of write-all loop
- struct msghdr msg = { - .msg_iov = iov, - .msg_iovlen = sizeof(iov) / sizeof(iov[0]), - }; - len = (int)sendmsg(ctx->log_fd, &msg, 0); + struct msghdr msg = { + .msg_iov = iov, + .msg_iovlen = sizeof(iov) / sizeof(iov[0]), + }; + size_t total = sizeof(pri) + sizeof(pid) + sizeof(mlen) + mlen; + size_t sent = 0; + ssize_t n; + while (sent < total) { + n = sendmsg(ctx->log_fd, &msg, 0); + if (n == -1) { + if (errno == EINTR) + continue; + len = -1; + break; + } + sent += (size_t)n; + if (sent == total) + break; + /* advance iovecs by n bytes — helper omitted for brevity */ + } + len = (len == -1) ? -1 : (int)sent;🤖 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/logerr.c` around lines 235 - 240, The current send of the composed iovec via sendmsg in logerr.c can short-write and desynchronize the stream; replace the single sendmsg(&msg) call with a write-all loop that repeatedly calls sendmsg on ctx->log_fd, handles EINTR by retrying, and on partial sends advances the iov base pointers and reduces iov_len for the remaining bytes until the full record (all iov entries) are transmitted; ensure the logic updates the struct msghdr/msg_iov and msg_iovlen (or rebuilds a trimmed iov array) between iterations and returns/propagates any non-recoverable error, so logreadfd’s header/payload framing remains synchronized.src/privsep.c (1)
884-924:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftShort-write on
sendmsgstill not handled inps_sendpsmmsg.The receiver (
ps_recvpsmsg) now reads an exactsizeof(struct ps_msghdr)header followed by an exactpsm.ps_namelen + psm.ps_controllen + cmsg_padlen + psm.ps_datalenpayload usingMSG_WAITALL. A single shortsendmsgon thisSOCK_STREAMfd — which can occur whenever the peer's receive buffer is near full or under signal/EINTR — will silently truncate this side's frame and permanently desync the IPC channel: the receiver will read the next frame's header bytes as the tail of this one. The fix requires a write-all loop that advancesm.msg_iov/msg_iovlenuntil allsizeof(*psm) + name + padding + control + ps_datalenbytes are sent, retrying onEINTR, and only reporting success when fully drained.Note that ancillary control data (passed fds via
SCM_RIGHTS) must be sent with the firstsendmsgcall only; subsequent partial-write iterations should send only the remaining payload bytes withmsg_control = NULL/msg_controllen = 0to avoid duplicating fds.🤖 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.c` around lines 884 - 924, ps_sendpsmmsg must handle short writes on the SOCK_STREAM by implementing a write-all loop: calculate the total frame length (sizeof(struct ps_msghdr) + psm.ps_namelen + cmsg_padlen + psm.ps_controllen + psm.ps_datalen), call sendmsg repeatedly until that many bytes are sent, retrying on EINTR, and return -1 on other errors. On each retry advance the m.msg_iov pointers and adjust iov_len/msg_iovlen to reflect bytes already sent so the next sendmsg only transmits remaining payload; ensure the ancillary data (msg_control/msg_controllen carrying SCM_RIGHTS) is attached only to the very first sendmsg and set msg_control = NULL and msg_controllen = 0 for subsequent partial sends to avoid duplicating fds; only treat the operation as successful when the full frame is drained. Use the existing symbols ps_sendpsmmsg, struct ps_msghdr/psm, m.msg_iov/m.msg_iovlen and msg->msg_control/msg->msg_controllen to locate and change the logic.
🤖 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 `@src/privsep.c`:
- Around line 1086-1133: The code must treat any short or failed recv() on the
stream IPC as a fatal error and jump to the existing stop: teardown path while
preserving the original errno; change the header read path (recv(fd, &psm,...))
so that if 0 < len < sizeof(psm) you set a saved_errno to EINVAL (or preserve
errno when recv returned -1) and goto stop instead of returning -1, and change
the payload read path (recv(fd, ps_data, dlen,...)) so that if (size_t)len !=
dlen you capture errno into a local saved_errno when recv returned -1 or set
saved_errno = EINVAL for a short read, assign errno = saved_errno and goto stop
(do not return -1); refer to recv, psm, ps_data, stop:, ctx, and eloop_exit when
making the changes.
---
Duplicate comments:
In `@src/logerr.c`:
- Around line 235-240: The current send of the composed iovec via sendmsg in
logerr.c can short-write and desynchronize the stream; replace the single
sendmsg(&msg) call with a write-all loop that repeatedly calls sendmsg on
ctx->log_fd, handles EINTR by retrying, and on partial sends advances the iov
base pointers and reduces iov_len for the remaining bytes until the full record
(all iov entries) are transmitted; ensure the logic updates the struct
msghdr/msg_iov and msg_iovlen (or rebuilds a trimmed iov array) between
iterations and returns/propagates any non-recoverable error, so logreadfd’s
header/payload framing remains synchronized.
In `@src/privsep.c`:
- Around line 884-924: ps_sendpsmmsg must handle short writes on the SOCK_STREAM
by implementing a write-all loop: calculate the total frame length
(sizeof(struct ps_msghdr) + psm.ps_namelen + cmsg_padlen + psm.ps_controllen +
psm.ps_datalen), call sendmsg repeatedly until that many bytes are sent,
retrying on EINTR, and return -1 on other errors. On each retry advance the
m.msg_iov pointers and adjust iov_len/msg_iovlen to reflect bytes already sent
so the next sendmsg only transmits remaining payload; ensure the ancillary data
(msg_control/msg_controllen carrying SCM_RIGHTS) is attached only to the very
first sendmsg and set msg_control = NULL and msg_controllen = 0 for subsequent
partial sends to avoid duplicating fds; only treat the operation as successful
when the full frame is drained. Use the existing symbols ps_sendpsmmsg, struct
ps_msghdr/psm, m.msg_iov/m.msg_iovlen and msg->msg_control/msg->msg_controllen
to locate and change the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c06fbd3-d87e-413b-857e-db88cb14a991
📒 Files selected for processing (3)
src/logerr.csrc/privsep-root.csrc/privsep.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/privsep-root.c
1 participant
macOS does not support SOCK_SEQPACKET.
All our messages use a fixed header which includes the lengths of all parts sent.
We can use these limits with MSG_WAITALL on blocking sockets in place of MSG_EOR to get the same effect.
Start of the work for #524.