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.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
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: ACK / 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 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.
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.