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

Open
rsmarples wants to merge 3 commits into
base: master
Choose a base branch
from
Open

privsep: Change IPC to use SOCK_STREAM #604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 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
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
10 changes: 5 additions & 5 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 Expand Up @@ -2785,7 +2785,7 @@ main(int argc, char **argv, char **envp)
#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
Expand Down
33 changes: 24 additions & 9 deletions src/logerr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,22 +218,25 @@ __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]),
};
len = (int)sendmsg(ctx->log_fd, &msg, MSG_EOR);
len = (int)sendmsg(ctx->log_fd, &msg, 0);
}
Comment on lines 235 to 240
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 | 🏗️ Heavy lift

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.

return len;
}
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 == sizeof(buf) ? mlen - 1 : mlen] = '\0';

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
84 changes: 36 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,27 @@ 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);
if (pc->psr_data == NULL)
PSR_ERROR(errno);
} else if (psr_error->psr_datalen > pc->psr_datalen)
PSR_ERROR(EMSGSIZE);

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 +182,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 +192,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 +835,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 +928,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
Loading
Loading