Conversation
SOCK_SEQPACKET is not supported for socketpair, so use SOCK_STREAM. Rename svc_* to srv_* so we don't conflict with svc_run RPC command. Detect setproctitle and supply compat for reallocarray. Fixes #1.
WalkthroughRenamed service API from svc_* to srv_, added xsocketpair/xsetfd socket-flag handling, converted IPC to two-phase recv/send, added reallocarray compatibility, broadened Darwin/macOS support and CI, adjusted plugin build rules, and migrated plugins/unpriv consumers to srv_ APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin
participant Service as srv_ctx
participant Daemon as dhcpsd
participant Unpriv as unpriv_helper
participant Kernel
Note over Daemon,Kernel: IPC socket creation
Daemon->>Kernel: xsocketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC)
Kernel-->>Daemon: fd[0], fd[1]
Daemon->>Service: srv_init(..., fd)
Note over Plugin,Service: Request handling
Plugin->>Service: srv_run(request)
Service->>Kernel: sendmsg(fd, header) -- (first phase)
Kernel-->>Service: ack/header consumed
Service->>Kernel: recvmsg(fd, payload) -- (second phase, if sc_datalen)
Kernel-->>Service: payload bytes
Service->>Plugin: dispatch callback (srv_ctx)
Note over Service,Unpriv: Offload lookup
Service->>Unpriv: srv_run(U_GETADDRINFO)
Unpriv->>Kernel: perform lookup
Unpriv-->>Service: srv_send(response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 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)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/eloop.c (1)
628-639:⚠️ Potential issue | 🟠 MajorRemove Apple-specific kqueue() branch to ensure FD_CLOEXEC is set on macOS.
The
__APPLE__branch at line 628–629 bypasses the close-on-exec flag setup that other BSD systems apply. SinceHAVE_KQUEUE1is only defined for NetBSD andKQUEUE_CLOEXECis not used anywhere, macOS will always take the Apple special case and skip the fcntl FD_CLOEXEC handling in the generic branch. This allows the kqueue fd to be inherited acrossexec*calls.Removing the Apple special case lets macOS fall through to the standard fcntl-based setup (lines 630–639), which properly sets FD_CLOEXEC on all systems.
🔧 Proposed fix
`#if` defined(HAVE_KQUEUE1) fd = kqueue1(O_CLOEXEC); `#elif` defined(KQUEUE_CLOEXEC) fd = kqueuex(KQUEUE_CLOEXEC); -#elif defined(USE_KQUEUE) && defined(__APPLE__) - fd = kqueue(); `#elif` defined(USE_KQUEUE) int flags; fd = kqueue(); + if (fd == -1) + return -1; flags = fcntl(fd, F_GETFD, 0); if (!(flags != -1 && !(flags & FD_CLOEXEC) && fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == 0)) { close(fd); return -1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eloop.c` around lines 628 - 639, The Apple-specific kqueue branch bypasses the FD_CLOEXEC setup so kqueue() on macOS can inherit across exec; remove the conditional that returns just fd when __APPLE__ is defined so macOS falls through to the generic USE_KQUEUE path that runs fcntl(F_GETFD)/fcntl(F_SETFD, flags|FD_CLOEXEC). Edit the src/eloop.c branch that currently does "elif defined(USE_KQUEUE) && defined(__APPLE__)" to eliminate that special-case and ensure kqueue(), flags, fcntl, FD_CLOEXEC, close(fd) and the return -1 logic from the generic USE_KQUEUE block are applied on macOS (respecting HAVE_KQUEUE1 / KQUEUE_CLOEXEC semantics elsewhere).src/dhcpsd.h (1)
38-50:⚠️ Potential issue | 🟠 MajorThe old
svc_runsymbol still leaks through the service layer.This header switches
ctx_unprivtostruct srv_ctx *, butsrc/service.hLine 67 still declaressvc_run(...)andsrc/service.cLines 218-226 still define it. The rename is therefore incomplete, and the Darwin/macOSsvc_runcollision this PR is trying to eliminate is still exposed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dhcpsd.h` around lines 38 - 50, The svc_run symbol still leaks despite switching ctx_unpriv to struct srv_ctx*, so remove or rename the public svc_run declaration/definition in service.h and service.c to the new internal symbol (e.g. srv_run) and update all callers to use the new name; ensure the prototype in src/service.h no longer exposes svc_run and that src/service.c defines the renamed function (or makes it static) so the OS-level svc_run collision is eliminated, and verify uses that reference ctx_unpriv/srv_ctx are updated accordingly.src/unpriv.c (1)
47-48:⚠️ Potential issue | 🟡 MinorDuplicate macro definition.
U_GETADDRINFOis defined twice on consecutive lines. This appears to be a copy-paste error.🔧 Suggested fix
`#define` U_GETADDRINFO 1 -#define U_GETADDRINFO 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/unpriv.c` around lines 47 - 48, The file defines the macro U_GETADDRINFO twice consecutively; remove the redundant duplicate so U_GETADDRINFO is only defined once (i.e., delete one of the repeated "#define U_GETADDRINFO 1" lines) to eliminate the duplicate macro definition.
🧹 Nitpick comments (1)
src/dhcpsd.c (1)
280-287:MSG_EORhas no effect onSOCK_STREAMsockets.After switching from
SOCK_SEQPACKETtoSOCK_STREAM, theMSG_EORflag in thesend()call is meaningless.MSG_EORis only meaningful for message-oriented protocols (likeSOCK_SEQPACKET). While this won't cause failures, it's misleading.🔧 Suggested fix
- if (send(ctx->ctx_fork_fd, &exit_code, sizeof(exit_code), MSG_EOR) == + if (send(ctx->ctx_fork_fd, &exit_code, sizeof(exit_code), 0) == -1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dhcpsd.c` around lines 280 - 287, The send() call that uses MSG_EOR on ctx->ctx_fork_fd is misleading because MSG_EOR is for message-oriented sockets; change the flags argument to 0 (or remove MSG_EOR) in the send(ctx->ctx_fork_fd, &exit_code, sizeof(exit_code), MSG_EOR) invocation so it uses no flags for the SOCK_STREAM socket, and optionally add a short comment near this block explaining that MSG_EOR is not applicable to SOCK_STREAM; keep the rest of the flow (logerr on -1, close and reset ctx->ctx_fork_fd) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common.c`:
- Around line 464-490: The xsetfd function mixes descriptor flags and status
flags: keep handling FD_CLOEXEC via fcntl(F_GETFD)/F_SETFD and handle O_NONBLOCK
via a separate fcntl(F_GETFL)/F_SETFL call; specifically, when flags &
SOCK_CLOEXEC modify oflags obtained from F_GETFD and call F_SETFD, and when
flags & SOCK_NONBLOCK get/use a separate var from F_GETFL, OR in O_NONBLOCK and
call F_SETFL if changed (do both operations independently and return error if
either fcntl fails).
In `@src/plugins/lua.c`:
- Line 273: The reply is using the wrong message type constant: change the
srv_send call that currently sends L_LOOKUPADDR to send L_LOOKUPHOSTNAME instead
(the handler is serving hostname lookups); update every occurrence involved (the
srv_send invocation with parameters sctx, p, L_LOOKUPADDR, err, hname, hnamelen
and the corresponding caller sites around the L_LOOKUPHOSTNAME request, e.g.,
the calls near the current caller lines 883-884) so the on-the-wire response is
labeled L_LOOKUPHOSTNAME rather than L_LOOKUPADDR.
In `@src/plugins/Makefile`:
- Around line 28-35: The .SUFFIXES line currently resets suffixes to ".soo .so
.in" but the implicit .c.soo transformation (the .c.soo rule) depends on ".c"
being present, causing plugins like icmp, lua and others (auto, leasefile,
addrinfo, ethers) to lack build recipes; update the .SUFFIXES declaration to
include ".c" (i.e., add ".c" alongside ".soo .so .in") so the .c.soo implicit
rule and .soo.so rule will apply and those plugin targets can be built.
In `@src/service.c`:
- Around line 217-227: Remove the leftover debug log call from svc_run: the
loginfo("SVC RUN") inside svc_run should be deleted (or wrapped behind a
debug/verbose flag if intentional). Locate the svc_run function in service.c and
remove the loginfo line so the function simply prepares the iovec and returns
the srv_runv call; if you prefer conditional logging, guard the loginfo with an
existing debug/log-level check rather than leaving an unconditional info log.
---
Outside diff comments:
In `@src/dhcpsd.h`:
- Around line 38-50: The svc_run symbol still leaks despite switching ctx_unpriv
to struct srv_ctx*, so remove or rename the public svc_run
declaration/definition in service.h and service.c to the new internal symbol
(e.g. srv_run) and update all callers to use the new name; ensure the prototype
in src/service.h no longer exposes svc_run and that src/service.c defines the
renamed function (or makes it static) so the OS-level svc_run collision is
eliminated, and verify uses that reference ctx_unpriv/srv_ctx are updated
accordingly.
In `@src/eloop.c`:
- Around line 628-639: The Apple-specific kqueue branch bypasses the FD_CLOEXEC
setup so kqueue() on macOS can inherit across exec; remove the conditional that
returns just fd when __APPLE__ is defined so macOS falls through to the generic
USE_KQUEUE path that runs fcntl(F_GETFD)/fcntl(F_SETFD, flags|FD_CLOEXEC). Edit
the src/eloop.c branch that currently does "elif defined(USE_KQUEUE) &&
defined(__APPLE__)" to eliminate that special-case and ensure kqueue(), flags,
fcntl, FD_CLOEXEC, close(fd) and the return -1 logic from the generic USE_KQUEUE
block are applied on macOS (respecting HAVE_KQUEUE1 / KQUEUE_CLOEXEC semantics
elsewhere).
In `@src/unpriv.c`:
- Around line 47-48: The file defines the macro U_GETADDRINFO twice
consecutively; remove the redundant duplicate so U_GETADDRINFO is only defined
once (i.e., delete one of the repeated "#define U_GETADDRINFO 1" lines) to
eliminate the duplicate macro definition.
---
Nitpick comments:
In `@src/dhcpsd.c`:
- Around line 280-287: The send() call that uses MSG_EOR on ctx->ctx_fork_fd is
misleading because MSG_EOR is for message-oriented sockets; change the flags
argument to 0 (or remove MSG_EOR) in the send(ctx->ctx_fork_fd, &exit_code,
sizeof(exit_code), MSG_EOR) invocation so it uses no flags for the SOCK_STREAM
socket, and optionally add a short comment near this block explaining that
MSG_EOR is not applicable to SOCK_STREAM; keep the rest of the flow (logerr on
-1, close and reset ctx->ctx_fork_fd) unchanged.
🪄 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: 454d9b32-0630-43b1-b538-087e827325eb
📒 Files selected for processing (17)
.gitignoreconfig-null.mkconfiguresrc/common.csrc/common.hsrc/dhcpsd.csrc/dhcpsd.hsrc/eloop.csrc/if.csrc/plugin.hsrc/plugins/Makefilesrc/plugins/ethers.csrc/plugins/lua.csrc/service.csrc/service.hsrc/unpriv.csrc/unpriv.h
💤 Files with no reviewable changes (1)
- config-null.mk
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/plugins/lua.c (2)
286-290:⚠️ Potential issue | 🔴 CriticalValidate payload length before header copy/shift in address lookup.
memcpyandmemmoverun before anylencheck. Iflen < sizeof(hostname),len - sizeof(hostname)underflows and can cause memory corruption.Proposed fix
const char *addr; char hostname[DHCP_HOSTNAME_LEN]; + if (len < sizeof(hostname) + sizeof(struct bootp)) { + errno = EINVAL; + goto out; + } memcpy(hostname, data, sizeof(hostname)); /* Aligns bootp */ memmove(UNCONST(data), (const char *)data + sizeof(hostname), len - sizeof(hostname)); len -= sizeof(hostname);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/lua.c` around lines 286 - 290, The code copies and shifts packet data using memcpy(hostname, data, sizeof(hostname)) and memmove(UNCONST(data), (const char *)data + sizeof(hostname), len - sizeof(hostname)) without validating len, which can underflow when len < sizeof(hostname); before calling memcpy or memmove in this address-lookup block, add a guard that checks if len >= sizeof(hostname) and handle the error path (returning or logging) when the check fails, only then perform memcpy(hostname, data, sizeof(hostname)) and memmove with the corrected length calculation (len - sizeof(hostname)).
668-678:⚠️ Potential issue | 🔴 CriticalFix pointer-size payload validation bug in lease expiry path.
Line 676 validates against
sizeof(dl)(pointer size), not the lease struct size. This can let undersized payloads pass and then dereference invalid memory.Proposed fix
- if (payloadlen < sizeof(dl)) { + if (payloadlen < sizeof(*dl)) { errno = EINVAL; goto out; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/lua.c` around lines 668 - 678, The payload length check incorrectly uses sizeof(dl) (size of a pointer) which allows undersized payloads; update the validation to compare payloadlen against the actual lease struct size (e.g., sizeof(*dl) or sizeof(struct dhcp_lease)) and ensure the same check guards subsequent uses of dl (in the lease expiry path where dl, dl->dl_clientid, dl->dl_addr are accessed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compat/reallocarray.c`:
- Line 56: Change the overflow errno assignment in reallocarray to use the
standard ENOMEM instead of EOVERFLOW: locate the overflow handling in the
reallocarray implementation (the branch that currently does "errno = EOVERFLOW")
and replace that assignment with "errno = ENOMEM" so callers receive the
expected libc errno on multiplication/size overflow; ensure the function name
reallocarray is referenced when making this change.
- Around line 55-56: The overflow guard in reallocarray uses SIZE_MAX / size
without ensuring size is non-zero, causing division-by-zero when size == 0;
update the conditional in the reallocarray implementation to only evaluate the
division when size != 0 (for example, add an explicit size != 0 check before
testing n > SIZE_MAX / size) so the expression becomes ((n | size) >=
SQRT_SIZE_MAX && size != 0 && n > SIZE_MAX / size), and keep the existing errno
= EOVERFLOW path; refer to the function reallocarray and variables n, size,
SQRT_SIZE_MAX, SIZE_MAX, and errno/EOVERFLOW when making the change.
---
Outside diff comments:
In `@src/plugins/lua.c`:
- Around line 286-290: The code copies and shifts packet data using
memcpy(hostname, data, sizeof(hostname)) and memmove(UNCONST(data), (const char
*)data + sizeof(hostname), len - sizeof(hostname)) without validating len, which
can underflow when len < sizeof(hostname); before calling memcpy or memmove in
this address-lookup block, add a guard that checks if len >= sizeof(hostname)
and handle the error path (returning or logging) when the check fails, only then
perform memcpy(hostname, data, sizeof(hostname)) and memmove with the corrected
length calculation (len - sizeof(hostname)).
- Around line 668-678: The payload length check incorrectly uses sizeof(dl)
(size of a pointer) which allows undersized payloads; update the validation to
compare payloadlen against the actual lease struct size (e.g., sizeof(*dl) or
sizeof(struct dhcp_lease)) and ensure the same check guards subsequent uses of
dl (in the lease expiry path where dl, dl->dl_clientid, dl->dl_addr are
accessed).
🪄 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: dbdb7018-0246-402a-a9ca-64b43bb8e6ca
📒 Files selected for processing (3)
compat/reallocarray.ccompat/reallocarray.hsrc/plugins/lua.c
✅ Files skipped from review due to trivial changes (1)
- compat/reallocarray.h
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/lua.c`:
- Around line 298-310: The deserialized hostname is not validated: ensure
hostname_len is > 0 and does not exceed the destination buffer before memcpy
(reject and set errno/EINVAL and goto out if invalid), and after advancing
datap/len and memmoving the remaining data, check that len >= sizeof(struct
bootp) before assigning l->l_req and l->l_reqlen and before reading htype/hlen;
use the existing symbols hostname_len, hostname, datap, len,
memmove(UNCONST(data), ...), and l->l_req/l->l_reqlen to locate where to add
these checks and early returns.
- Around line 290-304: The parse-error branches jump to the out label before
sin, ltime, and the iov array are initialized, causing srv_sendv() to be called
with indeterminate iov contents; update the out-path to detect the parse failure
(check the error variable, e.g., err == -1) and in that case call srv_send()
with a NULL/empty payload instead of srv_sendv(), or only call srv_sendv() when
err != -1 and sin/ltime/iov have been properly initialized; reference the
existing variables and functions in this block (datap, len, hostname_len,
hostname, sin, ltime, iov, srv_send, srv_sendv, and the out label) and ensure
the control flow uses srv_send for error exits and srv_sendv only for the
successful parse path.
In `@src/service.c`:
- Around line 91-120: The code only assigns sr->sr_result and sr->sr_errno
inside the cmd.sc_datalen != 0 branch causing zero-length replies to keep stale
values; move the assignments of sr->sr_result = cmd.sc_result and sr->sr_errno =
cmd.sc_errno out of that conditional so they are always set from the incoming
header (keep the recvmsg and buffer resize logic inside the datalen check and
leave sr->sr_datalen/sr->sr_data handling as-is).
- Around line 163-164: The send path using sendmsg on sctx->srv_fd must handle
short writes: replace the single sendmsg call in service.c (the block that
returns ssize_t err) with a loop that tracks bytes_sent and repeatedly calls
sendmsg until the entire msghdr/iovec payload is transmitted or a fatal error
occurs; on a partial write update the msghdr->msg_iov and msg_iovlen (adjust
iov_base and iov_len) to account for the bytes already sent and continue, and on
-1 handle EINTR by retrying and treat EAGAIN by retrying (or returning a
retryable status if your I/O model requires); ensure srv_runv and srv_recv will
only see a completed message when the full payload has been sent. Refer to
sctx->srv_fd, sendmsg, the local msghdr/iovec usage, srv_runv, and srv_recv to
locate and update the code.
🪄 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: da42ad20-6dd2-40fc-9682-bc636274dbb7
📒 Files selected for processing (4)
src/plugins/lua.csrc/service.csrc/service.hsrc/unpriv.c
🚧 Files skipped from review as they are similar to previous changes (2)
- src/unpriv.c
- src/service.h
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/dhcpsd.c (1)
207-208:⚠️ Potential issue | 🟠 MajorThe launcher status channel still assumes packet semantics after the
SOCK_STREAMswitch.Lines 207-208 change this channel to a byte stream, but
dhcpsd_fork_cb()still does a singlerecv()ofsizeof(exit_code)and Line 290 still does a singlesend()before closing. A short transfer now turns into a falsetruncated/hangup path and can report the wrong launcher/main status.Also applies to: 290-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dhcpsd.c` around lines 207 - 208, The launcher/main status channel was changed to SOCK_STREAM but the code in dhcpsd_fork_cb() and the send path still assumes single-packet semantics; update the logic that uses fork_fd and exit_code so reads and writes loop until all sizeof(exit_code) bytes are transferred (handle partial reads/writes, EINTR, EAGAIN and EOF) instead of calling a single recv()/send(); specifically modify the receive side in dhcpsd_fork_cb() to accumulate bytes into exit_code until complete and detect EOF as a failure, and modify the send side (the code that writes exit_code to fork_fd) to loop until all bytes are written before closing the FD. Ensure error return paths and close behavior remain correct.src/service.c (1)
170-171:⚠️ Potential issue | 🔴 CriticalLoop until the full frame is written on
SOCK_STREAM.Line 170 does a single
sendmsg(), and Line 192 only treats-1as failure. On a stream socket, a positive short write is still a valid result, so truncated frames are currently accepted as success and the peer's nextsrv_recv()can desynchronize or block inMSG_WAITALL.Also applies to: 192-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/service.c` around lines 170 - 171, The send logic currently does a single sendmsg(sctx->srv_fd, &msg, 0) and treats only -1 as failure, allowing partial writes on SOCK_STREAM to be accepted; change this to loop until the entire frame (total_len derived from the iovec/msg) is written: call sendmsg repeatedly, handle EINTR by retrying, handle EAGAIN/EWOULDBLOCK appropriately, and on a positive return < remaining_bytes adjust the iovec/offset (or rebuild msg) and subtract bytes_sent, continuing until remaining_bytes == 0; ensure you update/inspect the same send path used around sendmsg (use the same msg/iov references) and return an error only for true failures (<0 after retries), not for short writes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/service.c`:
- Around line 130-134: srv_recvl currently ignores the new ssize_t return of
srv_recv; capture the return value (ssize_t ret = srv_recv(arg, e)) and if ret <
0 treat it as an error: perform connection cleanup and stop further callbacks
(e.g., call the existing connection teardown routine such as srv_close(arg) or
otherwise close/free the socket and cancel timers) and return; only continue
when ret >= 0. This ensures protocol/I/O/allocation errors in srv_recv do not
leave the peer hanging.
- Line 252: The control socket created by xsocketpair is blocking which allows
srv_recv (which uses recvmsg with MSG_WAITALL) to block the event loop; make the
pair non-blocking by adding SOCK_NONBLOCK to the xsocketpair flags where fdset
is created (the call to xsocketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, ...,
fdset)) or, if you prefer to change state post-creation, set O_NONBLOCK on both
descriptors (fdset[0] and fdset[1]) via fcntl before registering them with
eloop_event_add so srv_recv will never block the reactor.
---
Duplicate comments:
In `@src/dhcpsd.c`:
- Around line 207-208: The launcher/main status channel was changed to
SOCK_STREAM but the code in dhcpsd_fork_cb() and the send path still assumes
single-packet semantics; update the logic that uses fork_fd and exit_code so
reads and writes loop until all sizeof(exit_code) bytes are transferred (handle
partial reads/writes, EINTR, EAGAIN and EOF) instead of calling a single
recv()/send(); specifically modify the receive side in dhcpsd_fork_cb() to
accumulate bytes into exit_code until complete and detect EOF as a failure, and
modify the send side (the code that writes exit_code to fork_fd) to loop until
all bytes are written before closing the FD. Ensure error return paths and close
behavior remain correct.
In `@src/service.c`:
- Around line 170-171: The send logic currently does a single
sendmsg(sctx->srv_fd, &msg, 0) and treats only -1 as failure, allowing partial
writes on SOCK_STREAM to be accepted; change this to loop until the entire frame
(total_len derived from the iovec/msg) is written: call sendmsg repeatedly,
handle EINTR by retrying, handle EAGAIN/EWOULDBLOCK appropriately, and on a
positive return < remaining_bytes adjust the iovec/offset (or rebuild msg) and
subtract bytes_sent, continuing until remaining_bytes == 0; ensure you
update/inspect the same send path used around sendmsg (use the same msg/iov
references) and return an error only for true failures (<0 after retries), not
for short writes.
🪄 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: 0e354cb1-b17f-4745-96f9-3ba7237be723
📒 Files selected for processing (3)
src/dhcpsd.csrc/plugins/lua.csrc/service.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/lua.c
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/build.yml (1)
46-51: Consider explicitly settingPKG_CONFIGto avoid silent Lua detection failures.The configure script defaults to looking for
pkg-config(seeconfigure:122-125). While Homebrew'spkgconfpackage typically provides apkg-configsymlink, explicitly setting the environment variable ensures Lua detection won't fail silently if the symlink is missing.Proposed fix
- name: Install Lua run: | brew install lua pkgconf - name: Configure - run: ./configure + run: PKG_CONFIG=pkgconf ./configure🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 46 - 51, The workflow's Configure step can miss pkg-config if Homebrew's pkgconf doesn't provide the symlink; after the Install Lua step, ensure the Configure run exports PKG_CONFIG to a valid pkg-config binary (e.g., resolve with which pkg-config or which pkgconf) before invoking ./configure so the configure script's Lua detection uses the explicit PKG_CONFIG environment variable; update the Configure run (the step that runs ./configure) to set PKG_CONFIG accordingly.src/service.c (1)
255-255: This blocking socket design is consistent across the codebase and appears intentional.Both
xsocketpair()calls in the codebase (src/service.c:255andsrc/dhcpsd.c:207) are created withSOCK_STREAM | SOCK_CLOEXECwithoutSOCK_NONBLOCK, despite the codebase having full support for non-blocking sockets (seeSOCK_CXNBmacro insrc/common.h:134). This is a deliberate design choice across the IPC channel creation.The blocking behavior trades off event loop robustness for code simplicity—
MSG_WAITALLguarantees a complete frame or error, eliminating the need for partial read buffering. For local Unix domain sockets between parent and child processes with small control messages, this is generally acceptable, as kernel buffers are sufficient and both peers are trusted.If this PR's scope includes making the IPC channel non-blocking for improved robustness, add
SOCK_NONBLOCKand implement partial read/write handling with buffering. Otherwise, no change is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/service.c` at line 255, The current xsocketpair calls deliberately create blocking Unix domain sockets (SOCK_STREAM | SOCK_CLOEXEC) so no immediate change is required; if you want to make IPC non‑blocking instead, change the socket flags to include the nonblocking macro (use SOCK_CXNB with the existing xsocketpair call sites in service.c and dhcpsd.c) and then implement proper partial read/write handling wherever MSG_WAITALL or blocking reads are relied upon (update the IPC read path to accumulate frames in a buffer and the write path to handle EAGAIN/EWOULDBLOCK and partial writes). Ensure you touch xsocketpair call sites, any code paths using MSG_WAITALL, and the IPC framing/buffering logic to support nonblocking behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Line 40: Update the workflow step that currently references
actions/checkout@v2 to use actions/checkout@v4 in the macOS job; locate the step
containing "uses: actions/checkout@v2" and change the version tag to "@v4" so
the checkout action runs on a supported Node runtime. Ensure the rest of the
step remains unchanged and do not modify the separate ubuntu job (which also
uses v2) as noted in the comment.
- Around line 34-37: The workflow uses an invalid runner label in the matrix
(matrix.os / runs-on) — replace "macos-arm64-latest" with a valid GitHub-hosted
macOS runner such as "macos-14" or "macos-15" (or "macos-latest-large" if you
need the large ARM runner) so the strategy.matrix.os array contains only valid
labels; update the matrix.os values accordingly and keep "macos-latest" or a
specific Intel macOS label if you intend to test both Intel and ARM.
In `@src/service.c`:
- Around line 108-111: The error message compares the wrong values: the check
uses nread != cmd.sc_datalen but the log prints sizeof(cmd) + cmd.sc_datalen,
which is misleading; update the logerrx call in the same block (where nread,
cmd.sc_datalen, logerrx and __func__ are used) to print the payload-only
comparison (e.g., logerrx("%s: read datalen mismatch: %zd != %zd", __func__,
nread, cmd.sc_datalen)) or alternatively change the condition to match the
logged total if that was intended—ensure the logged values exactly match the
comparison performed.
- Around line 130-137: srv_recvl currently treats any -1 from srv_recv as a
fatal error and calls eloop_exit(EXIT_FAILURE), which overwrites a prior clean
shutdown; update the contract so srv_recv returns a distinct value for hangup
(e.g., 0) versus error (-1) or exposes a hangup flag, then modify srv_recvl to
only call eloop_exit(EXIT_FAILURE) when srv_recv returns the error sentinel (-1)
and do nothing (or ensure eloop_exit(EXIT_SUCCESS) is preserved) when srv_recv
indicates a hangup; adjust the logic in srv_recv and the check in srv_recvl
(referencing functions srv_recv and srv_recvl and eloop_exit and the srv_ctx
used there) so graceful peer closes do not get translated into EXIT_FAILURE.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 46-51: The workflow's Configure step can miss pkg-config if
Homebrew's pkgconf doesn't provide the symlink; after the Install Lua step,
ensure the Configure run exports PKG_CONFIG to a valid pkg-config binary (e.g.,
resolve with which pkg-config or which pkgconf) before invoking ./configure so
the configure script's Lua detection uses the explicit PKG_CONFIG environment
variable; update the Configure run (the step that runs ./configure) to set
PKG_CONFIG accordingly.
In `@src/service.c`:
- Line 255: The current xsocketpair calls deliberately create blocking Unix
domain sockets (SOCK_STREAM | SOCK_CLOEXEC) so no immediate change is required;
if you want to make IPC non‑blocking instead, change the socket flags to include
the nonblocking macro (use SOCK_CXNB with the existing xsocketpair call sites in
service.c and dhcpsd.c) and then implement proper partial read/write handling
wherever MSG_WAITALL or blocking reads are relied upon (update the IPC read path
to accumulate frames in a buffer and the write path to handle EAGAIN/EWOULDBLOCK
and partial writes). Ensure you touch xsocketpair call sites, any code paths
using MSG_WAITALL, and the IPC framing/buffering logic to support nonblocking
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c368e44b-5345-46d0-941b-724be710e53c
📒 Files selected for processing (2)
.github/workflows/build.ymlsrc/service.c
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/build.yml (2)
35-37:⚠️ Potential issue | 🟠 MajorReplace unsupported macOS runner label in the matrix.
Line 36 uses
macos-latest-arm64, which is not a valid GitHub-hosted label and will fail job scheduling.Proposed fix
strategy: matrix: - os: [ macos-latest, macos-latest-arm64 ] + os: [ macos-15-intel, macos-15 ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 35 - 37, The matrix uses an unsupported runner label "macos-latest-arm64" which will fail scheduling; update the matrix configuration (the matrix: os array and the runs-on expression) to use valid GitHub-hosted labels such as "macos-latest" and "macos-12-arm64" or remove the invalid entry, ensuring the runs-on value remains ${{ matrix.os }} and the matrix entries match supported runner labels.
40-40:⚠️ Potential issue | 🟠 MajorUpgrade checkout action to v4 in the macOS job.
Line 40 still uses
actions/checkout@v2, which is too old for current hosted-runner environments.Proposed fix
- - uses: actions/checkout@v2 + - uses: actions/checkout@v4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml at line 40, Replace the deprecated actions/checkout@v2 usage in the macOS job with actions/checkout@v4; locate the macOS job block that contains the line "uses: actions/checkout@v2" and update it to "uses: actions/checkout@v4" (keep the existing inputs such as persist-credentials or fetch-depth if present) so the workflow uses the current checkout action version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 42-44: The workflow step with id "set-up-homebrew" currently uses
the mutable ref "uses: Homebrew/actions/setup-homebrew@main"; replace the branch
ref with the action's immutable commit SHA (pin the action to a full commit
hash) so the step "uses: Homebrew/actions/setup-homebrew@..." references a
specific commit SHA instead of `@main` to ensure reproducible and secure CI runs.
---
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 35-37: The matrix uses an unsupported runner label
"macos-latest-arm64" which will fail scheduling; update the matrix configuration
(the matrix: os array and the runs-on expression) to use valid GitHub-hosted
labels such as "macos-latest" and "macos-12-arm64" or remove the invalid entry,
ensuring the runs-on value remains ${{ matrix.os }} and the matrix entries match
supported runner labels.
- Line 40: Replace the deprecated actions/checkout@v2 usage in the macOS job
with actions/checkout@v4; locate the macOS job block that contains the line
"uses: actions/checkout@v2" and update it to "uses: actions/checkout@v4" (keep
the existing inputs such as persist-credentials or fetch-depth if present) so
the workflow uses the current checkout action version.
🪄 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: ff85f502-0ebd-4bbf-992b-b8ab8c2b3f61
📒 Files selected for processing (1)
.github/workflows/build.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 42-50: The workflow's Lua build can silently skip the plugin
because Homebrew's pkg-config path isn't exported before running ./configure;
update the CI steps around "Install Lua" and "Configure" to export the Homebrew
pkg-config directory into PKG_CONFIG_PATH (the Homebrew lib/pkgconfig location)
so pkg-config can find Lua .pc files before invoking ./configure, and add a
post-configure verification that checks config.mk for LUA_PLUGIN (e.g., fail the
job if LUA_PLUGIN is not present) to ensure the Lua plugin was detected; target
the existing steps invoking brew install lua, ./configure and make and the
config.mk/LUA_PLUGIN symbols when making these changes.
🪄 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: e2e0576a-c2b7-4c7c-966b-2001f71df1cf
📒 Files selected for processing (2)
.github/workflows/build.ymlsrc/service.c
✅ Files skipped from review due to trivial changes (1)
- src/service.c
Comment on lines
+42
to
+50
| - name: Install Lua | ||
| run: | | ||
| brew install lua | ||
|
|
||
| - name: Configure | ||
| run: ./configure | ||
|
|
||
| - name: Build | ||
| run: make |
There was a problem hiding this comment.
Lua plugin may silently fail to build — pkg-config configuration needed.
The ./configure script detects Lua via pkg-config (see configure:695-720). On macOS, Homebrew-installed Lua's .pc files may not be indexed by pkg-config without additional setup. If detection fails, the build will succeed but skip the Lua plugin entirely, defeating the test purpose.
Consider adding the Homebrew pkg-config path:
Proposed fix
- name: Install Lua
run: |
brew install lua
+
+ - name: Configure
+ run: |
+ export PKG_CONFIG_PATH="$(brew --prefix lua)/lib/pkgconfig:$PKG_CONFIG_PATH"
+ ./configure
- - name: Configure
- run: ./configure
-
- name: Build
run: makeAlternatively, you could add a post-configure check that verifies LUA_PLUGIN was detected:
grep -q 'LUA_PLUGIN' config.mk || { echo "Lua plugin not detected"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build.yml around lines 42 - 50, The workflow's Lua build
can silently skip the plugin because Homebrew's pkg-config path isn't exported
before running ./configure; update the CI steps around "Install Lua" and
"Configure" to export the Homebrew pkg-config directory into PKG_CONFIG_PATH
(the Homebrew lib/pkgconfig location) so pkg-config can find Lua .pc files
before invoking ./configure, and add a post-configure verification that checks
config.mk for LUA_PLUGIN (e.g., fail the job if LUA_PLUGIN is not present) to
ensure the Lua plugin was detected; target the existing steps invoking brew
install lua, ./configure and make and the config.mk/LUA_PLUGIN symbols when
making these changes.
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/common.c (1)
470-478:⚠️ Potential issue | 🔴 CriticalCritical: Line 476 uses
O_NONBLOCKinstead ofFD_CLOEXECwithF_SETFD.The
SOCK_CLOEXEChandling is setting the wrong flag.F_SETFDonly accepts descriptor flags (FD_CLOEXEC), not status flags (O_NONBLOCK).🐛 Proposed fix
`#ifndef` HAVE_SOCK_CLOEXEC if (flags & SOCK_CLOEXEC) { oflags = fcntl(fd, F_GETFD); if (oflags == -1) return -1; if (!(oflags & FD_CLOEXEC) && - fcntl(fd, F_SETFD, oflags | O_NONBLOCK) == -1) + fcntl(fd, F_SETFD, oflags | FD_CLOEXEC) == -1) return -1; } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.c` around lines 470 - 478, The SOCK_CLOEXEC handling uses the wrong flag when calling fcntl(F_SETFD): replace the status flag O_NONBLOCK with the descriptor flag FD_CLOEXEC so F_SETFD sets the close-on-exec bit; locate the block guarded by HAVE_SOCK_CLOEXEC that checks (flags & SOCK_CLOEXEC), inspect variables oflags and the fcntl calls (F_GETFD / F_SETFD) and change the value passed to F_SETFD from O_NONBLOCK to oflags | FD_CLOEXEC so the descriptor flag is set correctly.src/service.c (1)
108-111:⚠️ Potential issue | 🟡 MinorFormat specifier mismatch for
size_t.
cmd.sc_datalenissize_t(unsigned) but uses%zd(signed). Use%zufor unsigned size types.🔧 Suggested fix
if ((size_t)nread != cmd.sc_datalen) { - logerrx("%s: read datalen mismatch: %zd != %zd", + logerrx("%s: read datalen mismatch: %zd != %zu", __func__, nread, cmd.sc_datalen); return -1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/service.c` around lines 108 - 111, Format specifier mismatch: change the logerrx call that prints nread and cmd.sc_datalen so the signed nread uses %zd and the unsigned cmd.sc_datalen uses %zu; update the format string in the logerrx invocation (referencing __func__, nread, cmd.sc_datalen) to "%s: read datalen mismatch: %zd != %zu" so the types match their specifiers.
🧹 Nitpick comments (3)
compat/reallocarray.h (1)
1-1: Incorrect CVS tag in header file.The CVS ID tag references
reallocarr.cbut this isreallocarray.h. This appears to be a copy-paste artifact.📝 Proposed fix
-/* $NetBSD: reallocarr.c,v 1.4 2015/08/20 20:08:04 joerg Exp $ */ +/* $NetBSD: reallocarray.h */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compat/reallocarray.h` at line 1, The CVS ID header in compat/reallocarray.h incorrectly references "reallocarr.c"; update the header comment to refer to the correct filename (reallocarray.h) so the CVS tag matches the file, e.g. change the string "reallocarr.c" to "reallocarray.h" in the top comment; ensure you only modify the comment and preserve the existing timestamp/format.src/if.c (1)
67-67: Unnecessary removal of const qualifiers.The change from
const struct sockaddr_dl *sdltostruct sockaddr_dl *sdl(and similarly forsll) removes const-correctness without apparent necessity. The pointers are only read, not written to.♻️ Preserve const-correctness
`#ifdef` AF_LINK - struct sockaddr_dl *sdl; + const struct sockaddr_dl *sdl; `#ifdef` IFLR_ACTIVE struct if_laddrreq iflr = { .flags = IFLR_PREFIX }; `#endif` `#elif` defined(AF_PACKET) - struct sockaddr_ll *sll; + const struct sockaddr_ll *sll; `#endif`The casts on lines 87, 110, and 156 should then be
(const void *)to maintain type safety.Also applies to: 72-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/if.c` at line 67, Restore const-correctness by changing the local pointer declarations back to const: use "const struct sockaddr_dl *sdl" and "const struct sockaddr_ll *sll" (or whatever sll identifier exists) since the buffers are only read. Update the corresponding casts where those pointers are passed to functions so they cast to "(const void *)" instead of "(void *)" to match the const types; ensure any other uses that expect non-const remain unchanged or perform safe, explicit copies if mutation is required.configure (1)
612-614: Inverted condition for HAVE_SETPROCTITLE.The condition
[ "$SETPROCTITLE" != no ]will defineHAVE_SETPROCTITLEeven whenSETPROCTITLEis explicitly set to something other than "yes" or "no" (e.g., empty). However, since lines 605-608 explicitly set it to "yes" or "no", this should work correctly in practice.For consistency with other feature checks (e.g.,
STRLCPY,RBTREE), consider using the positive check:📝 Suggested improvement for consistency
-if [ "$SETPROCTITLE" != no ]; then +if [ "$SETPROCTITLE" = yes ]; then echo "#define HAVE_SETPROCTITLE" >>$CONFIG_H fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configure` around lines 612 - 614, The conditional that defines HAVE_SETPROCTITLE should only trigger when SETPROCTITLE is explicitly "yes"; change the test from the negative form ([ "$SETPROCTITLE" != no ]) to a positive equality check ([ "$SETPROCTITLE" = yes ]) so HAVE_SETPROCTITLE is defined only when SETPROCTITLE is "yes", matching the style used for other feature checks like STRLCPY and RBTREE and referencing the SETPROCTITLE variable and HAVE_SETPROCTITLE macro.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/eloop.c`:
- Around line 628-633: The current comment and behavior around kqueue() are
incorrect: kqueue FDs are inherited across fork() and not automatically closed,
and macOS does allow setting FD_CLOEXEC; update the code that calls kqueue()
(the fd = kqueue() site) to immediately set the close-on-exec flag using
fcntl(fd, F_SETFD, FD_CLOEXEC) and check for errors, and replace the comment to
state that we set FD_CLOEXEC to prevent the descriptor from leaking into
children (and note that fork-only children inherit FDs unless FD_CLOEXEC is
set).
In `@src/service.c`:
- Around line 104-106: The error message incorrectly states "recvmsg cmd" when
the recvmsg call is actually reading the data payload; update the log in the
nread == -1 branch inside the function shown (uses __func__) to accurately
reflect that this failure occurred while receiving the payload/data (e.g.,
"recvmsg payload" or "recvmsg data"), leaving the error handling and return -1
behavior unchanged so callers still get the same failure path.
---
Duplicate comments:
In `@src/common.c`:
- Around line 470-478: The SOCK_CLOEXEC handling uses the wrong flag when
calling fcntl(F_SETFD): replace the status flag O_NONBLOCK with the descriptor
flag FD_CLOEXEC so F_SETFD sets the close-on-exec bit; locate the block guarded
by HAVE_SOCK_CLOEXEC that checks (flags & SOCK_CLOEXEC), inspect variables
oflags and the fcntl calls (F_GETFD / F_SETFD) and change the value passed to
F_SETFD from O_NONBLOCK to oflags | FD_CLOEXEC so the descriptor flag is set
correctly.
In `@src/service.c`:
- Around line 108-111: Format specifier mismatch: change the logerrx call that
prints nread and cmd.sc_datalen so the signed nread uses %zd and the unsigned
cmd.sc_datalen uses %zu; update the format string in the logerrx invocation
(referencing __func__, nread, cmd.sc_datalen) to "%s: read datalen mismatch: %zd
!= %zu" so the types match their specifiers.
---
Nitpick comments:
In `@compat/reallocarray.h`:
- Line 1: The CVS ID header in compat/reallocarray.h incorrectly references
"reallocarr.c"; update the header comment to refer to the correct filename
(reallocarray.h) so the CVS tag matches the file, e.g. change the string
"reallocarr.c" to "reallocarray.h" in the top comment; ensure you only modify
the comment and preserve the existing timestamp/format.
In `@configure`:
- Around line 612-614: The conditional that defines HAVE_SETPROCTITLE should
only trigger when SETPROCTITLE is explicitly "yes"; change the test from the
negative form ([ "$SETPROCTITLE" != no ]) to a positive equality check ([
"$SETPROCTITLE" = yes ]) so HAVE_SETPROCTITLE is defined only when SETPROCTITLE
is "yes", matching the style used for other feature checks like STRLCPY and
RBTREE and referencing the SETPROCTITLE variable and HAVE_SETPROCTITLE macro.
In `@src/if.c`:
- Line 67: Restore const-correctness by changing the local pointer declarations
back to const: use "const struct sockaddr_dl *sdl" and "const struct sockaddr_ll
*sll" (or whatever sll identifier exists) since the buffers are only read.
Update the corresponding casts where those pointers are passed to functions so
they cast to "(const void *)" instead of "(void *)" to match the const types;
ensure any other uses that expect non-const remain unchanged or perform safe,
explicit copies if mutation is required.
🪄 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: 5ca7e466-7c4f-4bf6-ab6e-04fcf8ce61c3
📒 Files selected for processing (20)
.github/workflows/build.yml.gitignorecompat/reallocarray.ccompat/reallocarray.hconfig-null.mkconfiguresrc/common.csrc/common.hsrc/dhcpsd.csrc/dhcpsd.hsrc/eloop.csrc/if.csrc/plugin.hsrc/plugins/Makefilesrc/plugins/ethers.csrc/plugins/lua.csrc/service.csrc/service.hsrc/unpriv.csrc/unpriv.h
💤 Files with no reviewable changes (1)
- config-null.mk
Comment on lines
+628
to
+633
| #elif defined(USE_KQUEUE) && defined(__APPLE__) | ||
| /* macOS does not allow setting CLOEXEC on kqueue. | ||
| * This should not be a problem because eloop consumers | ||
| * fork and exec rather than just exec and kqueue is | ||
| * automatically closed on fork. */ | ||
| fd = kqueue(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that all fork() calls in the codebase are followed by exec() variants
# to confirm the CLOEXEC omission is safe
echo "=== Searching for fork() calls and their context ==="
rg -n -A10 '\bfork\s*\(\s*\)' --type=c
echo ""
echo "=== Checking for exec family calls ==="
rg -n '\b(execl|execle|execlp|execv|execve|execvp|execvpe)\s*\(' --type=cRepository: NetworkConfiguration/dhcpsd
Length of output: 1449
🏁 Script executed:
# Check if eloop.c exists and examine the file
find . -name "eloop.c" -type fRepository: NetworkConfiguration/dhcpsd
Length of output: 84
🏁 Script executed:
# Get more context around the fork() calls to see if exec is called
# Let's examine src/dhcpsd.c more closely around the fork calls
head -n 280 src/dhcpsd.c | tail -n 70Repository: NetworkConfiguration/dhcpsd
Length of output: 1725
🏁 Script executed:
# Search for exec calls more broadly (might be escaped or formatted differently)
rg -n 'exec' --type=c | head -20Repository: NetworkConfiguration/dhcpsd
Length of output: 140
🏁 Script executed:
# Read src/eloop.c lines 620-640 to see the full context
sed -n '620,640p' src/eloop.cRepository: NetworkConfiguration/dhcpsd
Length of output: 675
macOS kqueue FD without CLOEXEC — unsafe assumption about fork/exec pattern.
The comment contains two issues:
- It states kqueue is "automatically closed on fork," which is backwards — kqueue FDs are inherited across
fork()and only closed onexec()when CLOEXEC is set. - The claim that "eloop consumers fork and exec" does not match actual usage: the codebase uses
fork()withoutexec()(child processes continue as daemons viasetsid()andeloop_forked()).
Since child processes inherit the kqueue FD without CLOEXEC, and no exec() call closes it, the FD remains open in forked children, potentially causing resource leaks or unexpected behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/eloop.c` around lines 628 - 633, The current comment and behavior around
kqueue() are incorrect: kqueue FDs are inherited across fork() and not
automatically closed, and macOS does allow setting FD_CLOEXEC; update the code
that calls kqueue() (the fd = kqueue() site) to immediately set the
close-on-exec flag using fcntl(fd, F_SETFD, FD_CLOEXEC) and check for errors,
and replace the comment to state that we set FD_CLOEXEC to prevent the
descriptor from leaking into children (and note that fork-only children inherit
FDs unless FD_CLOEXEC is set).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/service.c (1)
255-264:⚠️ Potential issue | 🟠 MajorClose both
socketpair()fds iffork()fails.On the
fork()error path,sctx->srv_fdis still-1, sosrv_free()does not close either entry infdset. That leaks both descriptors exactly when the process is already under resource pressure.🧹 Minimal cleanup fix
case -1: logerr("%s: fork", __func__); + close(fdset[0]); + close(fdset[1]); goto error;Also applies to: 316-318
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/service.c` around lines 255 - 264, The fork() error path leaks both socketpair fds because sctx->srv_fd remains -1 and srv_free() won't close fdset; in the case -1 branch after xsocketpair(...) and before goto error, explicitly close fdset[0] and fdset[1] (or call a small cleanup helper) and/or set sctx->srv_fd to a valid value so srv_free() will close them; apply the same cleanup to the other similar fork-failure location (the second occurrence around where pid is checked again) to ensure both descriptors are closed on fork failure.
♻️ Duplicate comments (2)
src/service.c (2)
173-174:⚠️ Potential issue | 🔴 CriticalWrite the entire frame before reporting success.
On a
SOCK_STREAMsocket,sendmsg()can legally return a short positive byte count. Returning that as success leaves the header/payload half-written, and the nextsrv_recv()can lose framing.On POSIX/Unix stream sockets, can `sendmsg()` legally return a short positive byte count, and should callers loop until the full `iovec` payload is transmitted?🔁 Sketch of the required send loop
- ssize_t err = sendmsg(sctx->srv_fd, &msg, 0); - return err; + for (;;) { + ssize_t n = sendmsg(sctx->srv_fd, &msg, 0); + size_t off; + + if (n == -1) { + if (errno == EINTR) + continue; + return -1; + } + if (n == 0) { + errno = EPIPE; + return -1; + } + + off = (size_t)n; + while (off != 0 && msg.msg_iovlen != 0) { + if (off >= msg.msg_iov[0].iov_len) { + off -= msg.msg_iov[0].iov_len; + msg.msg_iov++; + msg.msg_iovlen--; + } else { + msg.msg_iov[0].iov_base = + (char *)msg.msg_iov[0].iov_base + off; + msg.msg_iov[0].iov_len -= off; + off = 0; + } + } + + if (msg.msg_iovlen == 0) + return (ssize_t)(sizeof(sc) + sc.sc_datalen); + }Also applies to: 195-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/service.c` around lines 173 - 174, The code currently calls sendmsg once (ssize_t err = sendmsg(sctx->srv_fd, &msg, 0); return err;) which can return a short positive byte count on SOCK_STREAM; change to a loop that repeatedly calls sendmsg (or send) until the full frame bytes from the iovec(s) are written, tracking bytes_sent vs total_len and advancing/adjusting the iovec base/length accordingly, while properly handling EINTR/EAGAIN and returning on fatal errors; apply the same fix to the other send site mentioned (lines ~195-197) and ensure srv_recv() sees complete frames only after the loop finishes successfully.
68-71:⚠️ Potential issue | 🟠 MajorKeep clean EOF distinct from reply and transport error states.
A zero-byte read on a plain
ELE_READevent still jumps tohangup:and returns-1, sosrv_recvl()overwrites the earlierEXIT_SUCCESSwithEXIT_FAILURE. Zero-length replies are valid now, so EOF needs its own negative sentinel (or a separate status flag) rather than reusing the generic error path.🧭 One way to preserve the clean-shutdown path
+enum { SRV_RECV_HANGUP = -2 }; + static ssize_t srv_recv(struct srv_ctx *sctx, unsigned short e) { @@ if (e & ELE_HANGUP) { hangup: eloop_exit(sctx->srv_ctx->ctx_eloop, EXIT_SUCCESS); - return -1; + return SRV_RECV_HANGUP; } @@ static void srv_recvl(void *arg, unsigned short e) { struct srv_ctx *sctx = arg; + ssize_t ret; - if (srv_recv(sctx, e) == -1 && !(e & ELE_HANGUP)) + ret = srv_recv(sctx, e); + if (ret == -1) eloop_exit(sctx->srv_ctx->ctx_eloop, EXIT_FAILURE); } @@ - if (srv_recv(sctx, (unsigned short)events) == -1) + if (srv_recv(sctx, (unsigned short)events) < 0) return -1;Also applies to: 135-136, 200-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/service.c` around lines 68 - 71, The current handler jumps to the hangup path (label hangup) for ELE_HANGUP and also for zero-byte reads on ELE_READ, causing eloop_exit(sctx->srv_ctx->ctx_eloop, EXIT_SUCCESS) to be overwritten by srv_recvl() returning -1; change the logic so EOF (zero-length read) is treated distinctly from transport/reply errors by returning a separate negative sentinel (e.g., ERR_EOF) or setting a status flag instead of jumping to hangup, and update srv_recvl(), the ELE_READ handling, and any other uses of the hangup label (including the contexts around ELE_HANGUP and the places noted near the other occurrences) to check for that EOF sentinel/flag and preserve the EXIT_SUCCESS clean-shutdown call invoked at hangup.
🧹 Nitpick comments (2)
.github/workflows/build.yml (1)
52-55: Consider removing disabled BSD jobs instead of usingif: false.Using
if: falseto disable jobs is a code smell flagged by actionlint. These jobs still appear in the workflow file and contribute to maintenance burden. Consider either:
- Removing the jobs entirely and tracking re-enablement in an issue
- Using a repository variable or secret to control enablement (e.g.,
if: vars.ENABLE_BSD_CI == 'true')- Moving to a separate workflow file that can be enabled later
If keeping them disabled temporarily is intentional, adding a comment explaining when/why they'll be re-enabled would help maintainability. The current comment "The BSDs are currently broken on github" is helpful but could include an issue reference for tracking.
Also applies to: 74-75, 94-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 52 - 55, The workflow currently keeps disabled BSD jobs (e.g., the "openbsd" job and other BSD blocks at the referenced sections) using "if: false", which actionlint flags; remove these disabled job blocks entirely or replace the conditional with a configurable toggle (for example change to use a repository variable like "if: vars.ENABLE_BSD_CI == 'true'") or move them to a separate workflow file, and if you choose to keep a temporary disablement add a clear comment referencing an issue/plan for re-enablement so maintainers can track it.src/plugins/Makefile (1)
37-37: Consider declaringalland other non-file targets as.PHONY.The
alltarget (and likelylint,install,clean,proginstall,maninstall) should be declared as.PHONYto ensure they always run even if a file with that name exists, and to improve make's performance by not checking for file timestamps.Proposed addition near the top of the file
CLEANFILES+= ${OBJS} ${PLUGS} CLEANFILES+= dhcpsd_leasefile.8 ${LUA_MAN8} +.PHONY: all lint install proginstall maninstall clean + .SUFFIXES: .c .o .so .in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/Makefile` at line 37, Declare the non-file make targets as phony so make won't treat them as files: add a .PHONY line that lists targets like all, lint, install, clean, proginstall, maninstall (and any other meta-targets) near the top of the Makefile; update the Makefile so the .PHONY declaration references the unique targets "all", "lint", "install", "clean", "proginstall", and "maninstall" to ensure they always execute regardless of existing filenames.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 33-50: The macOS build step must export Homebrew's pkg-config path
before running ./configure so pkg-config can find Lua; update the workflow so
the "Configure" step (or immediately before it, e.g., after "Install Lua") sets
PKG_CONFIG_PATH to include `$(brew --prefix)/opt/lua/lib/pkgconfig` (and/or
`$(brew --prefix)/lib/pkgconfig`) so configure detects the Lua plugin, then run
./configure; also add a post-configure step named "Verify Lua plugin detected"
that greps for 'LUA_PLUGIN' in config.mk and fails the job if not found to
ensure the plugin was detected.
In `@src/plugins/Makefile`:
- Around line 26-32: The .SUFFIXES line is missing the .c and .o suffixes so the
implicit suffix rules .c.o and .o.so aren't registered; update the .SUFFIXES
declaration to include .c and .o (so the declared .c.o and .o.so rules become
active and targets like auto.o, leasefile.o, addrinfo.o, ethers.o and their .so
counterparts can be built). Ensure you modify the existing .SUFFIXES entry (the
one currently listing .in .so) to also contain .c and .o so the .c.o and .o.so
suffix transformation rules are recognized.
---
Outside diff comments:
In `@src/service.c`:
- Around line 255-264: The fork() error path leaks both socketpair fds because
sctx->srv_fd remains -1 and srv_free() won't close fdset; in the case -1 branch
after xsocketpair(...) and before goto error, explicitly close fdset[0] and
fdset[1] (or call a small cleanup helper) and/or set sctx->srv_fd to a valid
value so srv_free() will close them; apply the same cleanup to the other similar
fork-failure location (the second occurrence around where pid is checked again)
to ensure both descriptors are closed on fork failure.
---
Duplicate comments:
In `@src/service.c`:
- Around line 173-174: The code currently calls sendmsg once (ssize_t err =
sendmsg(sctx->srv_fd, &msg, 0); return err;) which can return a short positive
byte count on SOCK_STREAM; change to a loop that repeatedly calls sendmsg (or
send) until the full frame bytes from the iovec(s) are written, tracking
bytes_sent vs total_len and advancing/adjusting the iovec base/length
accordingly, while properly handling EINTR/EAGAIN and returning on fatal errors;
apply the same fix to the other send site mentioned (lines ~195-197) and ensure
srv_recv() sees complete frames only after the loop finishes successfully.
- Around line 68-71: The current handler jumps to the hangup path (label hangup)
for ELE_HANGUP and also for zero-byte reads on ELE_READ, causing
eloop_exit(sctx->srv_ctx->ctx_eloop, EXIT_SUCCESS) to be overwritten by
srv_recvl() returning -1; change the logic so EOF (zero-length read) is treated
distinctly from transport/reply errors by returning a separate negative sentinel
(e.g., ERR_EOF) or setting a status flag instead of jumping to hangup, and
update srv_recvl(), the ELE_READ handling, and any other uses of the hangup
label (including the contexts around ELE_HANGUP and the places noted near the
other occurrences) to check for that EOF sentinel/flag and preserve the
EXIT_SUCCESS clean-shutdown call invoked at hangup.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 52-55: The workflow currently keeps disabled BSD jobs (e.g., the
"openbsd" job and other BSD blocks at the referenced sections) using "if:
false", which actionlint flags; remove these disabled job blocks entirely or
replace the conditional with a configurable toggle (for example change to use a
repository variable like "if: vars.ENABLE_BSD_CI == 'true'") or move them to a
separate workflow file, and if you choose to keep a temporary disablement add a
clear comment referencing an issue/plan for re-enablement so maintainers can
track it.
In `@src/plugins/Makefile`:
- Line 37: Declare the non-file make targets as phony so make won't treat them
as files: add a .PHONY line that lists targets like all, lint, install, clean,
proginstall, maninstall (and any other meta-targets) near the top of the
Makefile; update the Makefile so the .PHONY declaration references the unique
targets "all", "lint", "install", "clean", "proginstall", and "maninstall" to
ensure they always execute regardless of existing filenames.
🪄 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: 0c51af25-074b-4146-8153-15f4d7507ae6
📒 Files selected for processing (6)
.github/workflows/build.ymlcompat/reallocarray.csrc/common.csrc/eloop.csrc/plugins/Makefilesrc/service.c
🚧 Files skipped from review as they are similar to previous changes (2)
- compat/reallocarray.c
- src/eloop.c
Comment on lines
+33
to
+50
| macos: | ||
| strategy: | ||
| matrix: | ||
| os: [ macos-15, macos-26 ] | ||
| runs-on: ${{ matrix.os }} | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v6 | ||
|
|
||
| - name: Install Lua | ||
| run: | | ||
| brew install lua | ||
|
|
||
| - name: Configure | ||
| run: ./configure | ||
|
|
||
| - name: Build | ||
| run: make |
There was a problem hiding this comment.
Lua plugin may silently fail to build on macOS — pkg-config path still not configured.
The ./configure script detects Lua via pkg-config (see configure:695-720). On macOS, Homebrew-installed packages are not in pkg-config's default search path. Without setting PKG_CONFIG_PATH, the Lua detection will fail silently and the plugin will be skipped, defeating the test purpose.
Proposed fix to export Homebrew's pkg-config path
- name: Install Lua
run: |
brew install lua
- name: Configure
- run: ./configure
+ run: |
+ export PKG_CONFIG_PATH="$(brew --prefix lua)/lib/pkgconfig:$PKG_CONFIG_PATH"
+ ./configure
- name: Build
run: makeOptionally, add a post-configure verification step:
- name: Verify Lua plugin detected
run: grep -q 'LUA_PLUGIN' config.mk || { echo "Lua plugin not detected"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build.yml around lines 33 - 50, The macOS build step must
export Homebrew's pkg-config path before running ./configure so pkg-config can
find Lua; update the workflow so the "Configure" step (or immediately before it,
e.g., after "Install Lua") sets PKG_CONFIG_PATH to include `$(brew
--prefix)/opt/lua/lib/pkgconfig` (and/or `$(brew --prefix)/lib/pkgconfig`) so
configure detects the Lua plugin, then run ./configure; also add a
post-configure step named "Verify Lua plugin detected" that greps for
'LUA_PLUGIN' in config.mk and fails the job if not found to ensure the plugin
was detected.
Comment on lines
+26
to
+32
| .SUFFIXES: .in .so | ||
|
|
||
| .c.So: | ||
| .c.o: | ||
| ${CC} ${PICFLAG} -DPIC ${CFLAGS} ${CDBGFLAGS} ${CPPFLAGS} -c $< -o $@ | ||
|
|
||
| .So.so: | ||
| ${CC} ${LDFLAGS} -shared -Wl,-x -o $@ -Wl,-soname,$@ $< ${LIBS} | ||
| .o.so: | ||
| ${CC} ${LDFLAGS} ${LDFLAGS_SO} -shared -Wl,-x -o $@ $< ${LIBS} |
There was a problem hiding this comment.
Critical: Add .c and .o to .SUFFIXES for suffix rules to work.
The .SUFFIXES declaration on line 26 only includes .in .so, but the implicit suffix rules .c.o (line 28) and .o.so (line 31) require both their source and target suffixes to be declared in .SUFFIXES for make to recognize them as suffix transformation rules.
Without .c and .o in the suffix list:
icmp.oandlua.owill build (they have explicit rules on lines 40-44)lua.sowill build (explicit rule on line 46)auto.o,leasefile.o,addrinfo.o,ethers.owill fail — no build recipeauto.so,leasefile.so,icmp.so,addrinfo.so,ethers.sowill fail — the.o.sorule won't be recognized
Proposed fix
-.SUFFIXES: .in .so
+.SUFFIXES: .c .o .so .in📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .SUFFIXES: .in .so | |
| .c.So: | |
| .c.o: | |
| ${CC} ${PICFLAG} -DPIC ${CFLAGS} ${CDBGFLAGS} ${CPPFLAGS} -c $< -o $@ | |
| .So.so: | |
| ${CC} ${LDFLAGS} -shared -Wl,-x -o $@ -Wl,-soname,$@ $< ${LIBS} | |
| .o.so: | |
| ${CC} ${LDFLAGS} ${LDFLAGS_SO} -shared -Wl,-x -o $@ $< ${LIBS} | |
| .SUFFIXES: .c .o .so .in | |
| .c.o: | |
| ${CC} ${PICFLAG} -DPIC ${CFLAGS} ${CDBGFLAGS} ${CPPFLAGS} -c $< -o $@ | |
| .o.so: | |
| ${CC} ${LDFLAGS} ${LDFLAGS_SO} -shared -Wl,-x -o $@ $< ${LIBS} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/Makefile` around lines 26 - 32, The .SUFFIXES line is missing the
.c and .o suffixes so the implicit suffix rules .c.o and .o.so aren't
registered; update the .SUFFIXES declaration to include .c and .o (so the
declared .c.o and .o.so rules become active and targets like auto.o,
leasefile.o, addrinfo.o, ethers.o and their .so counterparts can be built).
Ensure you modify the existing .SUFFIXES entry (the one currently listing .in
.so) to also contain .c and .o so the .c.o and .o.so suffix transformation rules
are recognized.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SOCK_SEQPACKET is not supported for socketpair, so use SOCK_STREAM. Rename svc_* to srv_* so we don't conflict with svc_run RPC command. Detect setproctitle and supply compat for reallocarray.
Fixes #1.