Skip to content

privsep: Change IPC to use SOCK_STREAM #604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rsmarples merged 5 commits into from
May 9, 2026
Merged

privsep: Change IPC to use SOCK_STREAM #604

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ea17d7
privsep: Change IPC to use SOCK_STREAM
rsmarples May 5, 2026
1a95498
Address review comments.
rsmarples May 5, 2026
691ee9e
Address more review comments.
rsmarples May 5, 2026
3f9bc6b
Free root state when stopping
rsmarples May 9, 2026
750a00a
Tear down on invalid recv
rsmarples May 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/dhcpcd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ dhcpcd_daemonise(struct dhcpcd_ctx *ctx)

eloop_event_delete(ctx->eloop, ctx->fork_fd);
exit_code = EXIT_SUCCESS;
if (send(ctx->fork_fd, &exit_code, sizeof(exit_code), MSG_EOR) == -1)
if (send(ctx->fork_fd, &exit_code, sizeof(exit_code), 0) == -1)
logerr(__func__);
Comment on lines +387 to 388
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

close(ctx->fork_fd);
ctx->fork_fd = -1;
Expand Down Expand Up @@ -1498,7 +1498,7 @@ dhcpcd_signal_cb(int sig, void *arg)

if (sig != SIGCHLD && ctx->options & DHCPCD_FORKED) {
if (ctx->fork_fd != -1 && sig != SIGHUP &&
send(ctx->fork_fd, &sig, sizeof(sig), MSG_EOR) == -1)
send(ctx->fork_fd, &sig, sizeof(sig), 0) == -1)
logerr("%s: send", __func__);
return;
}
Expand Down Expand Up @@ -1905,7 +1905,7 @@ dhcpcd_fork_cb(void *arg, unsigned short events)
if (!(events & ELE_READ))
logerrx("%s: unexpected event 0x%04x", __func__, events);

len = read(ctx->fork_fd, &exit_code, sizeof(exit_code));
len = recv(ctx->fork_fd, &exit_code, sizeof(exit_code), MSG_WAITALL);
if (len == -1) {
logerr(__func__);
eloop_exit(ctx->eloop, EXIT_FAILURE);
Expand Down Expand Up @@ -2469,7 +2469,7 @@ main(int argc, char **argv, char **envp)
if (!(ctx.options & DHCPCD_DAEMONISE))
goto start_manager;

if (xsocketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CXNB, 0, fork_fd) ==
if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, fork_fd) ==
-1) {
logerr("socketpair");
goto exit_failure;
Expand Down
31 changes: 23 additions & 8 deletions src/logerr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,17 +218,20 @@ __printflike(2, 0) static int vlogmessage(int pri, const char *fmt,
if (ctx->log_fd != -1) {
pid_t pid = getpid();
char buf[LOGERR_SYSLOGBUF];
size_t mlen = 0;
struct iovec iov[] = {
{ .iov_base = &pri, .iov_len = sizeof(pri) },
{ .iov_base = &pid, .iov_len = sizeof(pid) },
{ .iov_base = &mlen, .iov_len = sizeof(mlen) },
{ .iov_base = buf },
};

len = vsnprintf(buf, sizeof(buf), fmt, args);
if (len != -1) {
if ((size_t)len >= sizeof(buf))
len = (int)sizeof(buf) - 1;
iov[2].iov_len = (size_t)(len + 1);
mlen = (size_t)len + 1;
if (mlen > sizeof(buf))
mlen = sizeof(buf);
iov[3].iov_len = mlen;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
struct msghdr msg = {
.msg_iov = iov,
.msg_iovlen = sizeof(iov) / sizeof(iov[0]),
Expand Down Expand Up @@ -399,10 +402,11 @@ logreadfd(int fd)
int pri;
pid_t pid;
char buf[LOGERR_SYSLOGBUF] = { '\0' };
size_t mlen;
struct iovec iov[] = {
{ .iov_base = &pri, .iov_len = sizeof(pri) },
{ .iov_base = &pid, .iov_len = sizeof(pid) },
{ .iov_base = buf, .iov_len = sizeof(buf) },
{ .iov_base = &mlen, .iov_len = sizeof(mlen) },
};
struct msghdr msg = { .msg_iov = iov,
.msg_iovlen = sizeof(iov) / sizeof(iov[0]) };
Expand All @@ -411,14 +415,25 @@ logreadfd(int fd)
len = recvmsg(fd, &msg, MSG_WAITALL);
if (len == -1 || len == 0)
return len;
/* Ensure we received the minimum and at least one character to log */
if ((size_t)len < sizeof(pri) + sizeof(pid) + 1 ||
msg.msg_flags & MSG_TRUNC) {
if ((size_t)len != sizeof(pri) + sizeof(pid) + sizeof(mlen)) {
errno = EMSGSIZE;
return -1;
}
if (mlen > sizeof(buf)) {
errno = ENOBUFS;
return -1;
}

len = recv(fd, buf, mlen, MSG_WAITALL);
if (len == -1)
return -1;
if ((size_t)len != mlen) {
errno = EINVAL;
return -1;
}

/* Ensure what we receive is NUL terminated */
buf[(size_t)len - (sizeof(pri) + sizeof(pid)) - 1] = '\0';
buf[mlen] = '\0';
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

ctx->log_pid = pid;
logmessage(pri, "%s", buf);
Expand Down
2 changes: 1 addition & 1 deletion src/logerr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ __printflike(2, 3) void logerrmessage(int pri, const char *fmt, ...);
#define logerr(...) log_err(__VA_ARGS__)
#define logerrx(...) log_errx(__VA_ARGS__)

/* For logging in a chroot using SOCK_SEQPACKET */
/* For logging in a chroot using SOCK_STREAM */
int loggetfd(void);
void logsetfd(int);
ssize_t logreadfd(int);
Expand Down
2 changes: 1 addition & 1 deletion src/privsep-bpf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
#include "logerr.h"
#include "privsep.h"

/* We expect to have open 3 SEQPACKET and one RAW fd */
/* We expect to have open 3 SOCK_STREAM and one RAW fd */

static void
ps_bpf_recvbpf(void *arg, unsigned short events)
Expand Down
2 changes: 1 addition & 1 deletion src/privsep-control.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
#include "logerr.h"
#include "privsep.h"

/* We expect to have open 2 SEQPACKET, 2 STREAM and 2 file STREAM fds */
/* We expect to have open 2 privsep STREAM, 2 STREAM and 2 file STREAM fds */

static int
ps_ctl_startcb(struct ps_process *psp)
Expand Down
2 changes: 1 addition & 1 deletion src/privsep-inet.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
#include "logerr.h"
#include "privsep.h"

/* We expect to have open 2 SEQPACKET, 1 udp, 1 udp6 and 1 raw6 fds */
/* We expect to have open 2 SOCK_STREAM, 1 udp, 1 udp6 and 1 raw6 fds */

#ifdef INET
static void
Expand Down
80 changes: 32 additions & 48 deletions src/privsep-root.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include <assert.h>
#include <errno.h>
#include <fcntl.h>
#include <poll.h>
#include <pwd.h>
#include <signal.h>
#include <stddef.h>
Expand Down Expand Up @@ -79,11 +80,6 @@ ps_root_readerrorcb(struct psr_ctx *pc)
struct dhcpcd_ctx *ctx = pc->psr_ctx;
int fd = PS_ROOT_FD(ctx);
struct psr_error *psr_error = &pc->psr_error;
struct iovec iov[] = {
{ .iov_base = psr_error, .iov_len = sizeof(*psr_error) },
{ .iov_base = pc->psr_data, .iov_len = pc->psr_datalen },
};
struct msghdr msg = { .msg_iov = iov, .msg_iovlen = __arraycount(iov) };
ssize_t len;

#define PSR_ERROR(e) \
Expand All @@ -95,45 +91,23 @@ ps_root_readerrorcb(struct psr_ctx *pc)
if (eloop_waitfd(fd) == -1)
PSR_ERROR(errno);

if (!pc->psr_mallocdata)
goto recv;

/* We peek at the psr_error structure to tell us how much of a buffer
* we need to read the whole packet. */
msg.msg_iovlen--;
len = recvmsg(fd, &msg, MSG_PEEK | MSG_WAITALL);
len = recv(fd, psr_error, sizeof(*psr_error), MSG_WAITALL);
if (len == -1)
PSR_ERROR(errno);

/* After this point, we MUST do another recvmsg even on a failure
* to remove the message after peeking. */
if ((size_t)len < sizeof(*psr_error)) {
/* We can't use the header to work out buffers, so
* remove the message and bail. */
(void)recvmsg(fd, &msg, MSG_WAITALL);
if ((size_t)len < sizeof(*psr_error))
PSR_ERROR(EINVAL);
}

/* No data to read? Unlikely but ... */
if (psr_error->psr_datalen == 0)
goto recv;
return len;

pc->psr_data = malloc(psr_error->psr_datalen);
if (pc->psr_data != NULL) {
iov[1].iov_base = pc->psr_data;
iov[1].iov_len = psr_error->psr_datalen;
msg.msg_iovlen++;
}
if (pc->psr_mallocdata)
pc->psr_data = malloc(psr_error->psr_datalen);

recv:
len = recvmsg(fd, &msg, MSG_WAITALL);
len = recv(fd, pc->psr_data, psr_error->psr_datalen, MSG_WAITALL);
if (len == -1)
PSR_ERROR(errno);
else if ((size_t)len < sizeof(*psr_error))
PSR_ERROR(EINVAL);
else if (msg.msg_flags & MSG_TRUNC)
PSR_ERROR(ENOBUFS);
else if ((size_t)len != sizeof(*psr_error) + psr_error->psr_datalen) {
else if ((size_t)len != psr_error->psr_datalen) {
#ifdef PRIVSEP_DEBUG
logerrx("%s: recvmsg returned %zd, expecting %zu", __func__,
len, sizeof(*psr_error) + psr_error->psr_datalen);
Expand Down Expand Up @@ -204,7 +178,7 @@ ps_root_writeerror(struct dhcpcd_ctx *ctx, ssize_t result, void *data,

if (len == 0)
msg.msg_iovlen = 1;
err = sendmsg(fd, &msg, MSG_EOR);
err = sendmsg(fd, &msg, 0);

/* Error sending the message? Try sending the error of sending. */
if (err == -1 && errno != EPIPE) {
Expand All @@ -214,7 +188,7 @@ ps_root_writeerror(struct dhcpcd_ctx *ctx, ssize_t result, void *data,
psr.psr_errno = errno;
psr.psr_datalen = 0;
msg.msg_iovlen = 1;
err = sendmsg(fd, &msg, MSG_EOR);
err = sendmsg(fd, &msg, 0);
}

return err;
Expand Down Expand Up @@ -857,14 +831,14 @@ ps_root_start(struct dhcpcd_ctx *ctx)
int logfd[2] = { -1, -1 }, datafd[2] = { -1, -1 };
pid_t pid;

if (xsocketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CXNB, 0, logfd) == -1)
if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, logfd) == -1)
return -1;
#ifdef PRIVSEP_RIGHTS
if (ps_rights_limit_fdpair(logfd) == -1)
return -1;
#endif

if (xsocketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CXNB, 0, datafd) == -1)
if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, datafd) == -1)
return -1;

if (ps_setbuf_fdpair(datafd) == -1)
Expand Down Expand Up @@ -950,20 +924,30 @@ ps_root_stop(struct dhcpcd_ctx *ctx)
/* drain the log */
if (ctx->ps_log_root_fd != -1) {
ssize_t loglen;

#ifdef __linux__
/* Seems to help to get the last parts,
* sched_yield(2) does not. */
sleep(0);
#endif
do {
struct pollfd pfd = {
.fd = ctx->ps_log_root_fd,
.events = POLLIN
};
int n;

/* the socket is blocking and we may not be able to
* change it to non blocking, so poll for data */
for (;;) {
n = poll(&pfd, 1, 1);
if (n == -1 || n == 0)
break;
loglen = logreadfd(ctx->ps_log_root_fd);
} while (loglen != 0 && loglen != -1);
close(ctx->ps_log_root_fd);
ctx->ps_log_root_fd = -1;
if (loglen == -1 || loglen == 0)
break;
}
}
}

if (ctx->ps_log_root_fd != -1) {
close(ctx->ps_log_root_fd);
ctx->ps_log_root_fd = -1;
}

if (ctx->ps_data_fd != -1) {
eloop_event_delete(ctx->eloop, ctx->ps_data_fd);
close(ctx->ps_data_fd);
Expand Down
36 changes: 25 additions & 11 deletions src/privsep.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ ps_startprocess(struct ps_process *psp,
int fd[2];
pid_t pid;

if (xsocketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CXNB, 0, fd) == -1) {
if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, fd) == -1) {
logerr("%s: socketpair", __func__);
return -1;
}
Expand Down Expand Up @@ -881,6 +881,7 @@ ps_sendpsmmsg(struct dhcpcd_ctx *ctx, int fd, struct ps_msghdr *psm,

psm->ps_namelen = msg->msg_namelen;
psm->ps_controllen = (socklen_t)msg->msg_controllen;
psm->ps_datalen = 0;

iovp->iov_base = msg->msg_name;
iovp->iov_len = msg->msg_namelen;
Expand Down Expand Up @@ -908,10 +909,11 @@ ps_sendpsmmsg(struct dhcpcd_ctx *ctx, int fd, struct ps_msghdr *psm,
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;
}
}

len = sendmsg(fd, &m, MSG_EOR);
len = sendmsg(fd, &m, 0);
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

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.


if (len == -1 && ctx != NULL) {
if (ctx->options & DHCPCD_FORKED &&
Expand Down Expand Up @@ -1064,12 +1066,14 @@ ps_recvpsmsg(struct dhcpcd_ctx *ctx, int fd, unsigned short events,
ssize_t (*callback)(void *, struct ps_msghdr *, struct msghdr *),
void *cbctx)
{
struct ps_msg psm;
struct ps_msghdr psm;
uint8_t ps_data[PS_BUFLEN];
ssize_t len;
size_t dlen;
struct iovec iov[1];
struct msghdr msg = { .msg_iov = iov, .msg_iovlen = 1 };
bool stop = false;
socklen_t cmsg_padlen;

if (events & ELE_HANGUP) {
len = 0;
Expand All @@ -1078,24 +1082,24 @@ ps_recvpsmsg(struct dhcpcd_ctx *ctx, int fd, unsigned short events,
if (!(events & ELE_READ))
logerrx("%s: unexpected event 0x%04x", __func__, events);

len = read(fd, &psm, sizeof(psm));
len = recv(fd, &psm, sizeof(psm), MSG_WAITALL);
#ifdef PRIVSEP_DEBUG
logdebugx("%s: fd=%d %zd", __func__, fd, len);
logdebugx("%s: pid=%d fd=%d len=%zd", __func__, getpid(), fd, len);
#endif

if (len == -1 || len == 0)
stop = true;
else {
dlen = (size_t)len;
if (dlen < sizeof(psm.psm_hdr)) {
if (dlen < sizeof(psm)) {
errno = EINVAL;
return -1;
}

if (psm.psm_hdr.ps_cmd == PS_STOP) {
if (psm.ps_cmd == PS_STOP) {
stop = true;
len = 0;
} else if (psm.psm_hdr.ps_cmd == PS_DAEMONISED) {
} else if (psm.ps_cmd == PS_DAEMONISED) {
ps_daemonised(ctx);
return 0;
}
Expand All @@ -1111,16 +1115,26 @@ ps_recvpsmsg(struct dhcpcd_ctx *ctx, int fd, unsigned short events,
eloop_exit(ctx->eloop, len != -1 ? EXIT_SUCCESS : EXIT_FAILURE);
return len;
}
dlen -= sizeof(psm.psm_hdr);

if (ps_unrollmsg(&msg, &psm.psm_hdr, psm.psm_data, dlen) == -1)
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;
}
}

if (ps_unrollmsg(&msg, &psm, ps_data, dlen) == -1)
return -1;

if (callback == NULL)
return 0;

errno = 0;
return callback(cbctx, &psm.psm_hdr, &msg);
return callback(cbctx, &psm, &msg);
}

struct ps_process *
Expand Down
Loading