Conversation
The interface you may want to configure might not exist when dhcpsd first starts. The -w option will cause dhcpcd to only configure an interface when it recieves a message on it. We assume if we have received a message it's ready as we cannot guess which address to use. So ensure the server address is added before the interface is marked as up! If the interface is marked as down then the interface is deconfigured. This allows dhcpsd to be used in-place of the macOS DHCP server as provided by the UTM framework.
WalkthroughAdds a privilege-separated helper for BPF and interface ops, moves interface discovery to an unprivileged helper, introduces platform-specific link monitors (route/netlink), refactors interface lifecycle/configuration, updates DHCP send paths to use the priv helper, and improves configure/Makefile detection and sanitiser settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as dhcpsd (main)
participant Unpriv as Unpriv Helper
participant Priv as Priv Helper
participant Kernel as Kernel/BPF
Main->>Unpriv: unpriv_getifaddrs(match_if_index)
Unpriv->>Kernel: getifaddrs()
Kernel-->>Unpriv: ifaddrs list
Unpriv-->>Main: packed ifaddrs reply
Main->>Priv: priv_openbpf(if_index/name)
Priv->>Priv: dispatch P_OPENBPF (find/create ifp)
Priv->>Kernel: open BPF (bpf_open / ioctl)
Kernel-->>Priv: bpf fd
Priv-->>Main: reply (fd/result)
Main->>Priv: priv_sendbpf(if_index, iovec)
Priv->>Priv: dispatch P_SENDBPF (locate ifp)
Priv->>Kernel: write/send via bpf fd
Kernel-->>Priv: nbytes/result
Priv-->>Main: reply (nbytes)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 12
🧹 Nitpick comments (5)
src/unpriv.c (1)
88-172: Document that returnedifaddrsmust be freed withfree(), notfreeifaddrs().The
unpriv_getifaddrs()function returns anifaddrsstructure allocated withmalloc()(line 113), not by the system'sgetifaddrs(). Callers must usefree()to release the memory, notfreeifaddrs(). This differs from the standard API convention and could lead to memory corruption if misused.Consider adding a comment documenting this or providing a dedicated free function.
📝 Suggested documentation
+/* + * Retrieve interface addresses via the unprivileged helper. + * Unlike the system getifaddrs(), the returned structure must be + * freed with free(), not freeifaddrs(). + */ int unpriv_getifaddrs(struct srv_ctx *ctx, struct ifaddrs **ifahead, const unsigned int *match_if_index)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/unpriv.c` around lines 88 - 172, The returned ifaddrs list from unpriv_getifaddrs() is allocated with malloc() (buf) and must be released with free(), not freeifaddrs(); update the function to document this difference by adding a clear comment at the top of unpriv_getifaddrs() (or add a helper like unpriv_freeifaddrs() that simply calls free()) so callers know to use free() (or the new unpriv_freeifaddrs()) instead of the system freeifaddrs().src/dhcp.c (1)
41-42: Unusual include path for header in same directory.The include
"src/priv.h"uses a path relative to the project root, but since this file is already insrc/, the conventional include would be"priv.h". This inconsistency may cause issues if include paths change or when building from different directories.♻️ Suggested fix
-#include "src/priv.h" +#include "priv.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dhcp.c` around lines 41 - 42, The include in dhcp.c currently uses a project-root relative path ("src/priv.h") which is inconsistent for a header in the same directory; change the include to the local header form by replacing "src/priv.h" with "priv.h" in src/dhcp.c (look for the include line at the top of the file) so the compiler uses the local header path and avoids build issues when include paths change.src/Makefile (1)
10-16: Inconsistent indentation in OBJS assignments.Lines 10, 13, and 16 use spaces for indentation while the rest of the Makefile uses tabs. While GNU Make is generally tolerant of this in variable assignments, it's best to maintain consistency.
Note: The pipeline failure on
if.ocompilation indicates an issue insrc/if.crather than this Makefile. The build system changes here appear correct.♻️ Suggested fix for consistent indentation
SRCS+= ${LINK_SRC} -OBJS+= ${SRCS:.c=.o} +OBJS+= ${SRCS:.c=.o} PVENDOR_SRCS= ${VENDOR_SRCS:vendor/%=${TOP}/vendor/%} -OBJS+= ${PVENDOR_SRCS:.c=.o} +OBJS+= ${PVENDOR_SRCS:.c=.o} PCOMPAT_SRCS= ${COMPAT_SRCS:compat/%=${TOP}/compat/%} -OBJS+= ${PCOMPAT_SRCS:.c=.o} +OBJS+= ${PCOMPAT_SRCS:.c=.o}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Makefile` around lines 10 - 16, The OBJS-related variable assignment lines use space indentation while the rest of the Makefile uses tabs; update the assignments for OBJS, PVENDOR_SRCS, and PCOMPAT_SRCS so their leading indentation is a tab character (not spaces) to match the file's style—specifically adjust the lines that reference SRCS/.c=.o, VENDOR_SRCS:vendor/%=${TOP}/vendor/%, and COMPAT_SRCS:compat/%=${TOP}/compat/% so they are indented with tabs.configure (1)
594-615: Route detection test may not fail as expected whenRTM_IFINFOis undefined.The test program compiles successfully even when
RTM_IFINFOis not defined because the#ifdef RTM_IFINFOblock simply returns 0, and theelsecase (implicit) falls through to end ofmain()without a return statement, which in C11 is undefined behavior for non-void functions but often compiles without error.Consider making the test fail explicitly when the required macro is missing:
♻️ Proposed fix for reliable detection
cat << EOF >_route.c `#include` <net/route.h> int main(void) { -#ifdef RTM_IFINFO - return 0; -#endif +#ifndef RTM_IFINFO +#error RTM_IFINFO not defined +#endif + return 0; } EOF🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configure` around lines 594 - 615, The test _route.c currently returns 0 regardless of RTM_IFINFO being defined because the `#ifdef` block doesn't enforce a failure when the macro is missing; update the test source generation so that main explicitly returns success (0) only when RTM_IFINFO is defined and returns non‑zero (or triggers a preprocessor `#error`) when RTM_IFINFO is not defined, then keep the existing compilation check that sets LINK_SRC="route.c" on success and reports "no" on failure.src/route.c (1)
87-89: Missingreturnafter logging unexpected event.When an unexpected event type is received, the function logs an error but continues processing. Consider returning early to avoid processing invalid events.
♻️ Proposed fix
if (e != ELE_READ) { logerrx("%s: unexpeced event %d", __func__, e); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/route.c` around lines 87 - 89, The code logs an unexpected event when e != ELE_READ but then keeps executing; modify the branch containing logerrx("%s: unexpeced event %d", __func__, e); to return immediately after logging to avoid processing invalid events—i.e., after the logerrx call, add a return from the current function (or return an appropriate error code if the function has a non-void signature) so that further code does not run on unexpected events.
🤖 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/bpf.c`:
- Around line 400-402: The allocation check for bpf->bpf_buffer must test the
open access mode correctly instead of using flags & (O_RDONLY | O_RDWR)
(O_RDONLY == 0), so modify the allocation logic (in the bpf open/init path that
sets bpf->bpf_buffer and bpf_size) to inspect (flags & O_ACCMODE) and allocate
when the mode is O_RDONLY or O_RDWR (or equivalent read-capable modes); update
the condition that gates buffer allocation to use (flags & O_ACCMODE) ==
O_RDONLY || (flags & O_ACCMODE) == O_RDWR (or test for read-capable flag),
ensure bpf_size > 0 when allocating, and keep bpf_read() usage consistent so
read(bpf->bpf_fd, bpf->bpf_buffer, bpf_size) never happens with a NULL buffer or
zero size.
In `@src/if.c`:
- Around line 186-201: The function if_link_match_index has three issues:
replace the incorrect use of ifa->ifa_addr with the provided sa when handling
AF_PACKET, cast sa to struct sockaddr_ll (use a variable sll) and compare
sll->sll_ifindex (not sll_index) to if_index, keep the AF_LINK branch using sdl
and sdl->sdl_index, and add a default return path (e.g., set errno to ENOSYS or
EINVAL and return -1) for builds where neither AF_LINK nor AF_PACKET is defined
so the function always returns a value.
- Around line 161-184: The function if_sockaddr_active incorrectly references an
undefined variable ifa; replace those references with the function parameters:
use strlcpy(iflr.iflr_name, if_name, sizeof(iflr.iflr_name)), memcpy(&iflr.addr,
sa, MIN(sa->sa_len, sizeof(iflr.addr))) (keeping the existing cast to const
struct sockaddr_dl *sdl for calculating prefix length), set iflr.prefixlen =
(unsigned int)sdl->sdl_alen * NBBY, and then perform the ioctl against
ctx->ctx_pf_link_fd and test IFLR_ACTIVE as written; remove the undefined ifa
usage so the function only uses ctx, if_name, and sa.
- Around line 348-357: The log call uses the incorrect symbol log_infox causing
a compile error; update the call in the block that checks ifp->if_flags &
IF_ACTIVE to use the correct logging function name loginfox, keeping the same
format string and arguments (ifp->if_name, ifp->if_index); verify surrounding
error handling that calls dhcpsd_configure_pools and if_free remains unchanged.
In `@src/plugins/auto.c`:
- Around line 122-123: The buffer returned by unpriv_getifaddrs() is
heap-allocated and must be freed with free(), so replace any calls to
freeifaddrs(ifaddrs) with free(ifaddrs) and ensure the error path also uses
free(ifaddrs) for the ifaddrs variable; keep all frees consistent (use free for
ifaddrs when it came from unpriv_getifaddrs), referencing the ifaddrs variable
and the unpriv_getifaddrs / freeifaddrs symbols to locate the changes.
In `@src/priv.c`:
- Around line 171-176: The code currently dereferences ifp->if_bpf without
ensuring ifp->if_bpf is non-NULL; update the block after the ifp NULL check to
validate ifp->if_bpf before calling ifp->if_output: if ifp->if_bpf is NULL, set
errno (e.g., ENODEV or ENXIO) and goto err, otherwise call ifp->if_output(ifp,
ifp->if_bpf->bpf_fd, iov, ARRAYCOUNT(iov)); this prevents a null-pointer
dereference when BPF was not opened for the interface.
- Around line 112-133: After calloc(1, sizeof(*ifp)) in this allocation branch,
add a NULL check for the returned pointer (ifp) and handle allocation failure
instead of immediately dereferencing it; on failure set errno = ENOMEM (or
appropriate error), jump to the existing err label, and avoid using ifp further.
Update the block around the allocation (the code that sets ifp->if_ctx,
ifp->if_index, calls strlcpy, memcpy, if_update_output, and TAILQ_INSERT_TAIL)
so those operations only run when ifp is non-NULL.
In `@src/route.c`:
- Around line 138-141: The code returns -1 when xsocket(PF_ROUTE, SOCK_RAW |
SOCK_CXNB, AF_UNSPEC) fails but does not free the previously allocated lctx,
causing a memory leak; update the error path in the block where link_fd is set
(the xsocket call) to free the allocated lctx (and any associated resources
initialized earlier) before returning -1 (e.g., call free(lctx) and null out the
pointer or call the existing cleanup helper if one exists) so that lctx is
properly released on socket creation failure.
- Around line 165-172: The function fails to assign the newly allocated link
context to ctx->ctx_link and leaks resources on error; set ctx->ctx_link = lctx
after successful allocation and before registering the event, and if
eloop_event_add(ctx->ctx_eloop, lctx->link_fd, ELE_READ, route_dispatch, lctx)
returns -1, clean up the allocated context and socket (call link_free(lctx) or
otherwise close lctx->link_fd and free lctx) before returning -1 so there is no
leak and link_free() later sees a valid ctx->ctx_link.
- Around line 142-161: The socket option calls are using the wrong context
variable (ctx->link_fd) before ctx->ctx_link is assigned; update all setsockopt
invocations (the SO_RERROR block and the RO_MSGFILTER / ROUTE_MSGFILTER blocks
where setsockopt is called) to use lctx->link_fd instead of ctx->link_fd so they
operate on the local link context; ensure you replace every occurrence in those
blocks (SO_RERROR, RO_MSGFILTER, ROUTE_MSGFILTER) to reference lctx->link_fd
consistently.
In `@src/service.c`:
- Around line 70-75: When recvmsg() returns 0 (EOF) in srv_recvl, synthesize a
hangup so the existing hangup suppression logic works: set the connection's
event mask to include ELE_HANGUP (e.g., sctx->srv_events |= ELE_HANGUP) or
otherwise mark the event as a hangup before jumping to the hangup handling code,
and ensure srv_recvl returns the same path as a real hangup; this ensures the
hangup label/srv handling will call eloop_exit(EXIT_SUCCESS) without later
treating the EOF-as-read case as a failure. Reference srv_recvl, ELE_HANGUP, the
hangup handling block that calls eloop_exit, and sctx->srv_events/sctx->srv_fd
when making the change.
In `@src/unpriv.c`:
- Around line 516-520: The TAILQ_INSERT_TAIL call unconditionally adds ifp to
ctx->ctx_ifaces even when an existing interface (same if_index) was found and
updated, risking duplicate/corrupted list entries; change the logic so
TAILQ_INSERT_TAIL(ctx->ctx_ifaces, ifp, if_next) is executed only when the
interface was newly allocated (i.e., when the code set free_ifp to true /
allocation branch occurred) and skip insertion when the code matched and updated
an existing interface (the branch that locates by if_index and reuses ifp).
---
Nitpick comments:
In `@configure`:
- Around line 594-615: The test _route.c currently returns 0 regardless of
RTM_IFINFO being defined because the `#ifdef` block doesn't enforce a failure when
the macro is missing; update the test source generation so that main explicitly
returns success (0) only when RTM_IFINFO is defined and returns non‑zero (or
triggers a preprocessor `#error`) when RTM_IFINFO is not defined, then keep the
existing compilation check that sets LINK_SRC="route.c" on success and reports
"no" on failure.
In `@src/dhcp.c`:
- Around line 41-42: The include in dhcp.c currently uses a project-root
relative path ("src/priv.h") which is inconsistent for a header in the same
directory; change the include to the local header form by replacing "src/priv.h"
with "priv.h" in src/dhcp.c (look for the include line at the top of the file)
so the compiler uses the local header path and avoids build issues when include
paths change.
In `@src/Makefile`:
- Around line 10-16: The OBJS-related variable assignment lines use space
indentation while the rest of the Makefile uses tabs; update the assignments for
OBJS, PVENDOR_SRCS, and PCOMPAT_SRCS so their leading indentation is a tab
character (not spaces) to match the file's style—specifically adjust the lines
that reference SRCS/.c=.o, VENDOR_SRCS:vendor/%=${TOP}/vendor/%, and
COMPAT_SRCS:compat/%=${TOP}/compat/% so they are indented with tabs.
In `@src/route.c`:
- Around line 87-89: The code logs an unexpected event when e != ELE_READ but
then keeps executing; modify the branch containing logerrx("%s: unexpeced event
%d", __func__, e); to return immediately after logging to avoid processing
invalid events—i.e., after the logerrx call, add a return from the current
function (or return an appropriate error code if the function has a non-void
signature) so that further code does not run on unexpected events.
In `@src/unpriv.c`:
- Around line 88-172: The returned ifaddrs list from unpriv_getifaddrs() is
allocated with malloc() (buf) and must be released with free(), not
freeifaddrs(); update the function to document this difference by adding a clear
comment at the top of unpriv_getifaddrs() (or add a helper like
unpriv_freeifaddrs() that simply calls free()) so callers know to use free() (or
the new unpriv_freeifaddrs()) instead of the system freeifaddrs().
🪄 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: b11aea62-7993-437f-be3c-f9bdf92f2b81
📒 Files selected for processing (20)
configuresrc/Makefilesrc/bpf.csrc/common.csrc/common.hsrc/dhcp.csrc/dhcp.hsrc/dhcpsd.8.insrc/dhcpsd.csrc/dhcpsd.hsrc/if.csrc/if.hsrc/if_ether.csrc/plugins/auto.csrc/priv.csrc/priv.hsrc/route.csrc/service.csrc/unpriv.csrc/unpriv.h
| #if 1 | ||
| int ibuf_len = 0; | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf 'bpf_open call sites and nearby flags:\n\n'
rg -nP -C3 '\bbpf_open\s*\(' src
printf '\nCurrent allocation guard in src/bpf.c:\n\n'
sed -n '505,518p' src/bpf.cRepository: NetworkConfiguration/dhcpsd
Length of output: 1385
🏁 Script executed:
#!/bin/bash
# 1. Check POSIX flag values in standard headers
echo "=== POSIX O_* flag definitions ==="
grep -rn "define O_RDONLY\|define O_WRONLY\|define O_RDWR\|define O_ACCMODE" /usr/include --include="*.h" 2>/dev/null | head -20
echo ""
echo "=== All bpf_open call sites ==="
rg -n '\bbpf_open\s*\(' src
echo ""
echo "=== Context: src/bpf.c lines 395-410 (around line 400) ==="
sed -n '395,410p' src/bpf.c
echo ""
echo "=== Context: src/bpf.c lines 505-520 (allocation guard) ==="
sed -n '505,520p' src/bpf.c
echo ""
echo "=== Check for bpf_read definition and usage ==="
grep -n "^bpf_read" src/bpf.c | head -5
rg -n 'bpf_read\s*\(' src --max-count=10Repository: NetworkConfiguration/dhcpsd
Length of output: 1372
🏁 Script executed:
#!/bin/bash
echo "=== All invocations of bpf_read() ==="
rg -n '\bbpf_read\s*\(' src
echo ""
echo "=== bpf_read function body (lines 534-560) ==="
sed -n '534,560p' src/bpf.c
echo ""
echo "=== Check lines 530-584 context mentioned in review ==="
sed -n '530,545p' src/bpf.c
echo ""
echo "=== Verify POSIX flag semantics via C headers ==="
cat > /tmp/check_flags.c << 'EOF'
`#include` <fcntl.h>
`#include` <stdio.h>
int main() {
printf("O_RDONLY=%d\nO_WRONLY=%d\nO_RDWR=%d\nO_ACCMODE=%d\n",
O_RDONLY, O_WRONLY, O_RDWR, O_ACCMODE);
printf("\nO_RDONLY & (O_RDONLY | O_RDWR) = %d\n", O_RDONLY & (O_RDONLY | O_RDWR));
printf("O_RDWR & (O_RDONLY | O_RDWR) = %d\n", O_RDWR & (O_RDONLY | O_RDWR));
printf("(O_RDONLY & O_ACCMODE) != O_WRONLY: %d\n", (O_RDONLY & O_ACCMODE) != O_WRONLY);
printf("(O_RDWR & O_ACCMODE) != O_WRONLY: %d\n", (O_RDWR & O_ACCMODE) != O_WRONLY);
printf("(O_WRONLY & O_ACCMODE) != O_WRONLY: %d\n", (O_WRONLY & O_ACCMODE) != O_WRONLY);
return 0;
}
EOF
gcc /tmp/check_flags.c -o /tmp/check_flags && /tmp/check_flagsRepository: NetworkConfiguration/dhcpsd
Length of output: 1549
🏁 Script executed:
#!/bin/bash
echo "=== Check if bpf_read is exported or used elsewhere ==="
rg -n 'bpf_read' src --type c --type h
echo ""
echo "=== Verify the flags semantics with known values ==="
python3 << 'EOF'
# POSIX standard values
O_RDONLY = 0
O_WRONLY = 1
O_RDWR = 2
O_ACCMODE = 3
print("POSIX flag values:")
print(f" O_RDONLY = {O_RDONLY}")
print(f" O_WRONLY = {O_WRONLY}")
print(f" O_RDWR = {O_RDWR}")
print(f" O_ACCMODE = {O_ACCMODE}")
print("\nCurrent guard: if (flags & (O_RDONLY | O_WRONLY))")
print(f" With O_RDONLY: {O_RDONLY} & ({O_RDONLY} | {O_RDWR}) = {O_RDONLY & (O_RDONLY | O_RDWR)} (False - FAILS TO ALLOCATE)")
print(f" With O_WRONLY: {O_WRONLY} & ({O_RDONLY} | {O_RDWR}) = {O_WRONLY & (O_RDONLY | O_RDWR)} (False - expected)")
print(f" With O_RDWR: {O_RDWR} & ({O_RDONLY} | {O_RDWR}) = {O_RDWR & (O_RDONLY | O_RDWR)} (True - correct)")
print("\nProposed fix: if ((flags & O_ACCMODE) != O_WRONLY)")
print(f" With O_RDONLY: ({O_RDONLY} & {O_ACCMODE}) != {O_WRONLY} = {(O_RDONLY & O_ACCMODE) != O_WRONLY} (True - correct)")
print(f" With O_WRONLY: ({O_WRONLY} & {O_ACCMODE}) != {O_WRONLY} = {(O_WRONLY & O_ACCMODE) != O_WRONLY} (False - correct)")
print(f" With O_RDWR: ({O_RDWR} & {O_ACCMODE}) != {O_WRONLY} = {(O_RDWR & O_ACCMODE) != O_WRONLY} (True - correct)")
EOF
echo ""
echo "=== Check bpf_read at line 543 for buffer access ==="
sed -n '540,545p' src/bpf.cRepository: NetworkConfiguration/dhcpsd
Length of output: 1023
Allocate the BPF read buffer by access mode, not O_RDONLY | O_RDWR.
Re-enabling bpf_read() makes this path depend on bpf->bpf_buffer, but the allocation check uses flags & (O_RDONLY | O_RDWR). Since O_RDONLY is 0, this condition evaluates to flags & 2, which fails for read-only opens and leaves bpf_buffer as NULL and bpf_size as 0. A read-only open to bpf_read() will then call read(bpf->bpf_fd, NULL, 0) at line 543, causing undefined behavior.
Proposed fix
- if (flags & (O_RDONLY | O_RDWR)) {
+ if ((flags & O_ACCMODE) != O_WRONLY) {
/* Get the required BPF buffer length from the kernel. */
if (ioctl(bpf->bpf_fd, BIOCGBLEN, &ibuf_len) == -1)
goto eexit;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bpf.c` around lines 400 - 402, The allocation check for bpf->bpf_buffer
must test the open access mode correctly instead of using flags & (O_RDONLY |
O_RDWR) (O_RDONLY == 0), so modify the allocation logic (in the bpf open/init
path that sets bpf->bpf_buffer and bpf_size) to inspect (flags & O_ACCMODE) and
allocate when the mode is O_RDONLY or O_RDWR (or equivalent read-capable modes);
update the condition that gates buffer allocation to use (flags & O_ACCMODE) ==
O_RDONLY || (flags & O_ACCMODE) == O_RDWR (or test for read-capable flag),
ensure bpf_size > 0 when allocating, and keep bpf_read() usage consistent so
read(bpf->bpf_fd, bpf->bpf_buffer, bpf_size) never happens with a NULL buffer or
zero size.
| lctx->link_fd = xsocket(PF_ROUTE, SOCK_RAW | SOCK_CXNB, AF_UNSPEC); | ||
| if (lctx->link_fd == -1) | ||
| return -1; | ||
|
|
There was a problem hiding this comment.
Memory leak when socket creation fails.
If xsocket() fails at line 138, the function returns -1 without freeing the lctx that was allocated at line 117. This was correctly flagged by static analysis.
🐛 Proposed fix
lctx->link_fd = xsocket(PF_ROUTE, SOCK_RAW | SOCK_CXNB, AF_UNSPEC);
- if (lctx->link_fd == -1)
+ if (lctx->link_fd == -1) {
+ free(lctx);
return -1;
+ }📝 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.
| lctx->link_fd = xsocket(PF_ROUTE, SOCK_RAW | SOCK_CXNB, AF_UNSPEC); | |
| if (lctx->link_fd == -1) | |
| return -1; | |
| lctx->link_fd = xsocket(PF_ROUTE, SOCK_RAW | SOCK_CXNB, AF_UNSPEC); | |
| if (lctx->link_fd == -1) { | |
| free(lctx); | |
| return -1; | |
| } | |
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 140-140: Memory leak
(memleak)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/route.c` around lines 138 - 141, The code returns -1 when
xsocket(PF_ROUTE, SOCK_RAW | SOCK_CXNB, AF_UNSPEC) fails but does not free the
previously allocated lctx, causing a memory leak; update the error path in the
block where link_fd is set (the xsocket call) to free the allocated lctx (and
any associated resources initialized earlier) before returning -1 (e.g., call
free(lctx) and null out the pointer or call the existing cleanup helper if one
exists) so that lctx is properly released on socket creation failure.
| if (eloop_event_add(ctx->ctx_eloop, lctx->link_fd, ELE_READ, | ||
| route_dispatch, lctx) == -1) { | ||
| logerr("%s: eloop_event_add", __func__); | ||
| return -1; | ||
| } | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
Missing assignment of lctx to ctx->ctx_link and memory leak on failure.
Two issues:
- The function never assigns
ctx->ctx_link = lctx, solink_free()will receiveNULLand the allocated context is leaked. - If
eloop_event_add()fails, the function returns -1 without cleaning uplctxor closing the socket.
🐛 Proposed fix
if (eloop_event_add(ctx->ctx_eloop, lctx->link_fd, ELE_READ,
route_dispatch, lctx) == -1) {
logerr("%s: eloop_event_add", __func__);
+ close(lctx->link_fd);
+ free(lctx);
return -1;
}
+ ctx->ctx_link = lctx;
return 0;
}📝 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.
| if (eloop_event_add(ctx->ctx_eloop, lctx->link_fd, ELE_READ, | |
| route_dispatch, lctx) == -1) { | |
| logerr("%s: eloop_event_add", __func__); | |
| return -1; | |
| } | |
| return 0; | |
| } | |
| if (eloop_event_add(ctx->ctx_eloop, lctx->link_fd, ELE_READ, | |
| route_dispatch, lctx) == -1) { | |
| logerr("%s: eloop_event_add", __func__); | |
| close(lctx->link_fd); | |
| free(lctx); | |
| return -1; | |
| } | |
| ctx->ctx_link = lctx; | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/route.c` around lines 165 - 172, The function fails to assign the newly
allocated link context to ctx->ctx_link and leaks resources on error; set
ctx->ctx_link = lctx after successful allocation and before registering the
event, and if eloop_event_add(ctx->ctx_eloop, lctx->link_fd, ELE_READ,
route_dispatch, lctx) returns -1, clean up the allocated context and socket
(call link_free(lctx) or otherwise close lctx->link_fd and free lctx) before
returning -1 so there is no leak and link_free() later sees a valid
ctx->ctx_link.
| if (sctx->srv_ctx->ctx_options & DHCPSD_MAIN) | ||
| logdebugx("%s(%d): fd %d hungup", __func__, getpid(), | ||
| sctx->srv_fd); | ||
| close(sctx->srv_fd); | ||
| sctx->srv_fd = -1; | ||
| eloop_exit(sctx->srv_ctx->ctx_eloop, EXIT_SUCCESS); |
There was a problem hiding this comment.
Clean EOF is still reported as failure.
recvmsg(...)=0 also lands in hangup:, but srv_recvl() only suppresses failure when the original event mask had ELE_HANGUP. If the peer close arrives as ELE_READ + EOF, this path still exits with EXIT_FAILURE after already requesting EXIT_SUCCESS.
Proposed fix
static void
srv_recvl(void *arg, unsigned short e)
{
struct srv_ctx *sctx = arg;
- if (srv_recv(sctx, e) == -1 && !(e & ELE_HANGUP))
+ if (srv_recv(sctx, e) == -1 && sctx->srv_fd != -1)
eloop_exit(sctx->srv_ctx->ctx_eloop, EXIT_FAILURE);
}Also applies to: 140-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/service.c` around lines 70 - 75, When recvmsg() returns 0 (EOF) in
srv_recvl, synthesize a hangup so the existing hangup suppression logic works:
set the connection's event mask to include ELE_HANGUP (e.g., sctx->srv_events |=
ELE_HANGUP) or otherwise mark the event as a hangup before jumping to the hangup
handling code, and ensure srv_recvl returns the same path as a real hangup; this
ensures the hangup label/srv handling will call eloop_exit(EXIT_SUCCESS) without
later treating the EOF-as-read case as a failure. Reference srv_recvl,
ELE_HANGUP, the hangup handling block that calls eloop_exit, and
sctx->srv_events/sctx->srv_fd when making the change.
as the unpriv process only outputs in debug mode
There was a problem hiding this comment.
Actionable comments posted: 3
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)
186-207:⚠️ Potential issue | 🟠 MajorLua stack imbalance: popping 4 values but 5 were pushed.
After
lua_getfieldforlease_timeat line 186, there are 5 values on the stack (address, netmask, from, to, lease_time). However, line 207 only pops 4, leavinglease_timeon the stack each iteration. This stack leak will eventually corrupt the Lua stack or cause memory issues.🐛 Proposed fix
skip: - lua_pop(L, 4); + lua_pop(L, 5);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/lua.c` around lines 186 - 207, The Lua stack is left with the lease_time value because lua_getfield(L, -5, "lease_time") pushes an extra value but the code calls lua_pop(L, 4); change the pop to remove all five values (e.g., replace lua_pop(L, 4); with lua_pop(L, 5);) or explicitly pop the lease_time after using lua_tonumber to ensure the stack is balanced; update usages around lua_getfield, lua_tonumber, and the skip label so pool parsing no longer leaks lease_time onto the Lua stack.
316-324:⚠️ Potential issue | 🟠 MajorRedundant and always-failing length check logic.
The nested condition is logically flawed:
- Outer
ifat line 318:len < offsetof(struct bootp, vend) + sizeof(uint32_t)- Inner
ifat line 319: Same check||cookie comparisonWhen the outer condition is true (we enter the block), the first part of the inner
||is guaranteed true, so the inner condition always evaluates to true, making thedhcp_cookiecmpcheck unreachable. This means any packet with a truncated vendor area will fail regardless of cookie validity.🐛 Proposed fix - check cookie only when length is sufficient for it
/* Allow for a truncated vendor area if it can at least * contain a DHCP cookie. */ - if (len < offsetof(struct bootp, vend) + sizeof(uint32_t)) { - if (len < offsetof(struct bootp, vend) + sizeof(uint32_t) || - dhcp_cookiecmp(l->l_req) != 0) { - errno = EINVAL; - goto out; - } + if (len < offsetof(struct bootp, vend) + sizeof(uint32_t) || + dhcp_cookiecmp(l->l_req) != 0) { + errno = EINVAL; + goto out; }Or if the intent was to allow packets without vendor area but require valid cookie when present:
/* Allow for a truncated vendor area if it can at least * contain a DHCP cookie. */ - if (len < offsetof(struct bootp, vend) + sizeof(uint32_t)) { - if (len < offsetof(struct bootp, vend) + sizeof(uint32_t) || - dhcp_cookiecmp(l->l_req) != 0) { - errno = EINVAL; - goto out; - } - } + if (len >= offsetof(struct bootp, vend) + sizeof(uint32_t) && + dhcp_cookiecmp(l->l_req) != 0) { + 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 316 - 324, The nested length check is redundant and makes dhcp_cookiecmp(l->l_req) unreachable; update the logic so the cookie comparison only runs when the buffer is large enough to contain the DHCP cookie. Specifically, remove the inner duplicated length test and replace the block around len and offsetof(struct bootp, vend) + sizeof(uint32_t) with: first validate that len is not too small if you intend to require at least a cookie, or otherwise accept truncated vendor area, and then call dhcp_cookiecmp(l->l_req) only when len >= offsetof(struct bootp, vend) + sizeof(uint32_t); on cookie failure set errno = EINVAL and goto out (preserving errno/EINVAL and goto out behavior).
♻️ Duplicate comments (2)
src/bpf.c (1)
400-402:⚠️ Potential issue | 🟠 MajorUse access-mode masking for read-buffer allocation.
Line 509 still uses
flags & (O_RDONLY | O_RDWR). BecauseO_RDONLY == 0, read-only opens won’t allocatebpf->bpf_buffer. Since this path is compiled again (Line 400), switch toO_ACCMODEand guard against non-positiveBIOCGBLEN.Proposed fix
- if (flags & (O_RDONLY | O_RDWR)) { + if ((flags & O_ACCMODE) != O_WRONLY) { /* Get the required BPF buffer length from the kernel. */ if (ioctl(bpf->bpf_fd, BIOCGBLEN, &ibuf_len) == -1) goto eexit; + if (ibuf_len <= 0) { + errno = EINVAL; + goto eexit; + } bpf->bpf_size = (size_t)ibuf_len; bpf->bpf_buffer = malloc(bpf->bpf_size);#!/bin/bash set -euo pipefail echo "=== bpf_open call sites ===" rg -nP '\bbpf_open\s*\(' src echo echo "=== Allocation guard in src/bpf.c ===" sed -n '505,518p' src/bpf.c echo echo "=== Access-mode semantics check ===" python3 - <<'PY' O_RDONLY=0 O_WRONLY=1 O_RDWR=2 O_ACCMODE=3 for name, flags in [("O_RDONLY", O_RDONLY), ("O_WRONLY", O_WRONLY), ("O_RDWR", O_RDWR)]: old = bool(flags & (O_RDONLY | O_RDWR)) new = ((flags & O_ACCMODE) != O_WRONLY) print(f"{name}: old_guard={old}, new_guard={new}") PYAlso applies to: 509-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bpf.c` around lines 400 - 402, Change the read-allocation guards to use access-mode masking and validate buffer size: replace checks like "flags & (O_RDONLY | O_RDWR)" with "((flags & O_ACCMODE) != O_WRONLY)" when deciding to allocate bpf->bpf_buffer in the bpf open path, call BIOCGBLEN and ensure the returned ibuf_len (or the variable used for length) is positive before allocating or assigning bpf->bpf_buffer, and apply the same fix to the other occurrence around the allocation block (the region handling BIOCGBLEN/ibuf_len). Ensure you reference BIOCGBLEN, ibuf_len, bpf->bpf_buffer and the bpf open allocation logic when making the changes.src/plugins/auto.c (1)
122-122:⚠️ Potential issue | 🔴 CriticalUse
free()instead offreeifaddrs()for buffer fromunpriv_getifaddrs().The buffer returned by
unpriv_getifaddrs()is allocated viamalloc()(seesrc/unpriv.cline 113), not by the system'sgetifaddrs(). Callingfreeifaddrs()on this buffer causes undefined behavior becausefreeifaddrs()expects the specific allocation structure from the system call.,
🐛 Proposed fix
if (pool == NULL) { logerr("%s: realloc", __func__); - freeifaddrs(ifaddrs); + free(ifaddrs); return -1; }- freeifaddrs(ifaddrs); + free(ifaddrs); return npools;Also applies to: 156-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/auto.c` at line 122, The code calls freeifaddrs(ifaddrs) for buffers returned by unpriv_getifaddrs(); change those calls to free(ifaddrs) because unpriv_getifaddrs() allocates with malloc() and freeifaddrs() is only valid for system getifaddrs() allocations—locate the usages (e.g., the freeifaddrs call handling the ifaddrs variable in src/plugins/auto.c) and replace them with free to avoid undefined behavior.
🧹 Nitpick comments (1)
src/if.c (1)
40-42: Removesrc/prefix from include paths for consistency.Lines 40-42 use
#include "src/common.h"etc., while other includes in the same file (lines 62-68) and all other files in the codebase use plain filenames like#include "common.h". This inconsistency should be fixed.♻️ Proposed fix
`#include` "queue.h" -#include "src/common.h" -#include "src/priv.h" -#include "src/unpriv.h" +#include "common.h" +#include "priv.h" +#include "unpriv.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/if.c` around lines 40 - 42, The three include directives currently written as `#include` "src/common.h", `#include` "src/priv.h", and `#include` "src/unpriv.h" are inconsistent with the rest of the codebase; remove the "src/" prefix and change them to `#include` "common.h", `#include` "priv.h", and `#include` "unpriv.h" so the include style matches other headers in this file and the project.
🤖 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/if.c`:
- Line 241: The buffer returned by unpriv_getifaddrs() is malloc'd by our code,
so replace the incorrect freeifaddrs(ifaddrs) call with free(ifaddrs) to avoid
undefined behavior; locate the call site in src/if.c (where unpriv_getifaddrs()
is used) and change the deallocation function from freeifaddrs to free, ensuring
any related comments reflect that the buffer is heap-allocated by
unpriv_getifaddrs().
- Around line 138-141: The AF_PACKET branch currently tests the destination
length (ifp->if_hwlen) instead of the source length, causing zero-byte copies
when ifp->if_hwlen is 0; update the condition to check the source sll->sll_halen
(e.g., if (sll->sll_halen <= sizeof(ifp->if_hwaddr))) and then set ifp->if_hwlen
= sll->sll_halen followed by memcpy(ifp->if_hwaddr, sll->sll_addr,
ifp->if_hwlen) so the copy uses the actual source length; adjust the condition
in the block referencing ifp, sll, if_hwlen, sll_halen, and memcpy accordingly.
In `@src/unpriv.c`:
- Around line 122-124: Add a portable fallback for the ALIGN macro in
src/unpriv.c by inserting a conditional definition after the existing includes:
if ALIGN is not already defined, define it to round its argument up to the
machine word boundary using uintptr_t (i.e., add a `#ifndef` ALIGN / `#define`
ALIGN(...) / `#endif` block that computes ((uintptr_t)(x) + (sizeof(long)-1)) &
~(sizeof(long)-1)). Also ensure <stdint.h> is included if uintptr_t is not
available; this fixes uses like ALIGN(sizeof(*ifa)), ALIGN(IFNAMSIZ) and
ALIGN(sizeof(salen) * IFA_NADDRS) without changing the call sites.
---
Outside diff comments:
In `@src/plugins/lua.c`:
- Around line 186-207: The Lua stack is left with the lease_time value because
lua_getfield(L, -5, "lease_time") pushes an extra value but the code calls
lua_pop(L, 4); change the pop to remove all five values (e.g., replace
lua_pop(L, 4); with lua_pop(L, 5);) or explicitly pop the lease_time after using
lua_tonumber to ensure the stack is balanced; update usages around lua_getfield,
lua_tonumber, and the skip label so pool parsing no longer leaks lease_time onto
the Lua stack.
- Around line 316-324: The nested length check is redundant and makes
dhcp_cookiecmp(l->l_req) unreachable; update the logic so the cookie comparison
only runs when the buffer is large enough to contain the DHCP cookie.
Specifically, remove the inner duplicated length test and replace the block
around len and offsetof(struct bootp, vend) + sizeof(uint32_t) with: first
validate that len is not too small if you intend to require at least a cookie,
or otherwise accept truncated vendor area, and then call
dhcp_cookiecmp(l->l_req) only when len >= offsetof(struct bootp, vend) +
sizeof(uint32_t); on cookie failure set errno = EINVAL and goto out (preserving
errno/EINVAL and goto out behavior).
---
Duplicate comments:
In `@src/bpf.c`:
- Around line 400-402: Change the read-allocation guards to use access-mode
masking and validate buffer size: replace checks like "flags & (O_RDONLY |
O_RDWR)" with "((flags & O_ACCMODE) != O_WRONLY)" when deciding to allocate
bpf->bpf_buffer in the bpf open path, call BIOCGBLEN and ensure the returned
ibuf_len (or the variable used for length) is positive before allocating or
assigning bpf->bpf_buffer, and apply the same fix to the other occurrence around
the allocation block (the region handling BIOCGBLEN/ibuf_len). Ensure you
reference BIOCGBLEN, ibuf_len, bpf->bpf_buffer and the bpf open allocation logic
when making the changes.
In `@src/plugins/auto.c`:
- Line 122: The code calls freeifaddrs(ifaddrs) for buffers returned by
unpriv_getifaddrs(); change those calls to free(ifaddrs) because
unpriv_getifaddrs() allocates with malloc() and freeifaddrs() is only valid for
system getifaddrs() allocations—locate the usages (e.g., the freeifaddrs call
handling the ifaddrs variable in src/plugins/auto.c) and replace them with free
to avoid undefined behavior.
---
Nitpick comments:
In `@src/if.c`:
- Around line 40-42: The three include directives currently written as `#include`
"src/common.h", `#include` "src/priv.h", and `#include` "src/unpriv.h" are
inconsistent with the rest of the codebase; remove the "src/" prefix and change
them to `#include` "common.h", `#include` "priv.h", and `#include` "unpriv.h" so the
include style matches other headers in this file and the project.
🪄 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: dec9a201-c27c-4164-b54a-81371d14c334
📒 Files selected for processing (7)
src/bpf.csrc/if.csrc/plugins/auto.csrc/plugins/lua.csrc/route.csrc/service.csrc/unpriv.c
🚧 Files skipped from review as they are similar to previous changes (2)
- src/service.c
- src/route.c
| if (len < ALIGN(sizeof(*ifa)) + ALIGN(IFNAMSIZ) + | ||
| ALIGN(sizeof(salen) * IFA_NADDRS)) | ||
| goto err; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ALIGN is defined in sys/param.h or elsewhere
rg -n '#define\s+ALIGN' --type c --type hRepository: NetworkConfiguration/dhcpsd
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check the includes at the top of src/unpriv.c
head -40 src/unpriv.c | cat -n
echo "---"
# Check lines around 122 where ALIGN is supposedly used
sed -n '120,130p' src/unpriv.c | cat -n
echo "---"
# Check other usage locations
sed -n '363,370p' src/unpriv.c | cat -n
sed -n '414,422p' src/unpriv.c | cat -nRepository: NetworkConfiguration/dhcpsd
Length of output: 2996
🏁 Script executed:
#!/bin/bash
# Search for ALIGN usage across the entire file
rg -n 'ALIGN\(' src/unpriv.cRepository: NetworkConfiguration/dhcpsd
Length of output: 1006
🏁 Script executed:
#!/bin/bash
# Check if sys/param.h is a system header that typically defines ALIGN
# Also check for any other definitions or conditionals
rg -n 'sys/param\.h|define.*ALIGN|ifndef.*ALIGN' src/unpriv.cRepository: NetworkConfiguration/dhcpsd
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Look for build configuration files
fd -t f -E '.git' '(Makefile|CMakeLists\.txt|configure|\.mk|meson\.build|build\.yml|CI\.yml)' | head -20Repository: NetworkConfiguration/dhcpsd
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Check for any platform-specific documentation or README
fd -t f -name 'README*' -o -name 'PLATFORM*' -o -name 'BUILD*' | xargs cat 2>/dev/null | head -100Repository: NetworkConfiguration/dhcpsd
Length of output: 301
🏁 Script executed:
#!/bin/bash
# Look for GitHub Actions or CI configuration
fd -t f '.github/workflows' -o '.gitlab-ci.yml' -o '.travis.yml' | head -20Repository: NetworkConfiguration/dhcpsd
Length of output: 218
🏁 Script executed:
#!/bin/bash
# Check the Makefile
head -50 Makefile | cat -nRepository: NetworkConfiguration/dhcpsd
Length of output: 1623
🏁 Script executed:
#!/bin/bash
# Check configure script for platform handling
grep -n 'ALIGN\|param\.h\|platform\|BSD\|Linux' configure | head -30Repository: NetworkConfiguration/dhcpsd
Length of output: 338
🏁 Script executed:
#!/bin/bash
# Check Makefile.inc for platform configuration
cat Makefile.inc | cat -nRepository: NetworkConfiguration/dhcpsd
Length of output: 733
🏁 Script executed:
#!/bin/bash
# Look for CI/workflow files without using fd syntax
find . -path './.git' -prune -o -type f -name '*.yml' -o -name '*.yaml' | grep -E '(github|gitlab|travis|workflow)' 2>/dev/nullRepository: NetworkConfiguration/dhcpsd
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Check the GitHub Actions workflow
cat .github/workflows/build.yml | head -100Repository: NetworkConfiguration/dhcpsd
Length of output: 2433
🏁 Script executed:
#!/bin/bash
# Look for any ALIGN-related issues or test reports
rg -n 'ALIGN|undeclared' .github/ src/ --type yaml --type txt 2>/dev/null | head -20Repository: NetworkConfiguration/dhcpsd
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about target platforms
grep -r 'target\|platform\|BSD\|Linux' README* CONTRIBUTING* 2>/dev/null | head -20Repository: NetworkConfiguration/dhcpsd
Length of output: 392
🏁 Script executed:
#!/bin/bash
# Check if there are any platform-specific headers or conditionals for ALIGN
rg -n 'ifdef|ifndef|if defined' src/unpriv.c | head -20Repository: NetworkConfiguration/dhcpsd
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Check recent git history to see if this code was recently added or modified
git log --oneline -20 -- src/unpriv.c 2>/dev/null | head -20Repository: NetworkConfiguration/dhcpsd
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Search for any conditional includes or ALIGN handling elsewhere in codebase
rg -n 'ALIGN' --type c --type h | head -30Repository: NetworkConfiguration/dhcpsd
Length of output: 1352
🌐 Web query:
ALIGN macro sys/param.h Linux availability
💡 Result:
The ALIGN macro is not available in sys/param.h on Linux systems. sys/param.h in glibc (used by Linux) is a compatibility header primarily defining BSD-style constants and limits like NBBY, NGROUPS, MAXSYMLINKS, howmany, roundup, powerof2, MIN, and MAX, but does not define ALIGN. ALIGN(p) is defined in some BSD-derived systems (e.g., OpenBSD, FreeBSD) as _ALIGN(p), typically for pointer alignment: #define ALIGN(p) (((u_long)(p) + ALIGNBYTES) & ~ALIGNBYTES), where ALIGNBYTES is architecture-specific (e.g., 7 for 64-bit). Android's Bionic libc defines ALIGN(p) in sys/param.h as (((uintptr_t)(p) + ALIGNBYTES) & ~ALIGNBYTES), but this is Android-specific, not standard Linux/glibc. In the Linux kernel, ALIGN(x, a) is defined in include/linux/kernel.h as ALIGN_KERNEL((x), (a)), which rounds up x to the nearest multiple of a (assuming a is a power of 2). A similar kernel-only round_up(x, y) exists. These are not in userspace sys/param.h. Linux userspace code seeking alignment macros should use standard C11 alignas/aligned_alloc or architecture-specific intrinsics, not sys/param.h ALIGN, as it is not portable to Linux/glibc.
Citations:
- 1: https://codebrowser.dev/gcc/include/x86_64-linux-gnu/sys/param.h.html
- 2: https://codebrowser.dev/glibc/glibc/misc/sys/param.h.html
- 3: https://android.googlesource.com/platform/bionic/+/2aace21/libc/include/sys/param.h
- 4: https://github.com/torvalds/linux/blob/ff2d8b19a3a62559afba1c53360c8577a7697714/include/linux/kernel.h
- 5: https://github.com/torvalds/linux/blob/16f73eb02d7e1765ccab3d2018e0bd98eb93d973/include/linux/kernel.h
- 6: https://github.com/torvalds/linux/blob/master/include/linux/kernel.h
🏁 Script executed:
#!/bin/bash
# Get more detailed history
git log --all --oneline -- src/unpriv.c | head -10Repository: NetworkConfiguration/dhcpsd
Length of output: 324
🏁 Script executed:
#!/bin/bash
# Check file creation date and status
git log --diff-filter=A --find-object -- src/unpriv.cRepository: NetworkConfiguration/dhcpsd
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Check if file exists in current HEAD
test -f src/unpriv.c && echo "File exists" || echo "File does not exist"Repository: NetworkConfiguration/dhcpsd
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Check when src/unpriv.c was added to the repository
git log --all --follow --oneline src/unpriv.c | tail -5Repository: NetworkConfiguration/dhcpsd
Length of output: 324
🏁 Script executed:
#!/bin/bash
# Check the actual commit that added this file
git log --all --diff-filter=A --oneline -- 'src/unpriv.c'Repository: NetworkConfiguration/dhcpsd
Length of output: 252
🏁 Script executed:
#!/bin/bash
# Let's check if there are recent failed builds or if the file was just added
git show HEAD:src/unpriv.c | head -30Repository: NetworkConfiguration/dhcpsd
Length of output: 1570
🏁 Script executed:
#!/bin/bash
# Check if sys/param.h on this system actually has ALIGN
# We can look at what it includes
cat -n /usr/include/sys/param.h 2>/dev/null | head -50 || echo "Not available in sandbox"Repository: NetworkConfiguration/dhcpsd
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check if ALIGN usage is protected by `#ifdef` BSD
sed -n '35,50p' src/unpriv.c | cat -n
sed -n '370,385p' src/unpriv.c | cat -n
sed -n '435,450p' src/unpriv.c | cat -nRepository: NetworkConfiguration/dhcpsd
Length of output: 1518
🏁 Script executed:
#!/antml/bash
# Let's check the exact structure of the functions using ALIGN
# to see if they're BSD-guarded
rg -B5 'ALIGN\(sizeof\(\*ifa\)\)' src/unpriv.c | head -30Repository: NetworkConfiguration/dhcpsd
Length of output: 726
🏁 Script executed:
#!/bin/bash
# Find the function containing lines 122-124 and check if it's guarded
sed -n '100,135p' src/unpriv.c | cat -nRepository: NetworkConfiguration/dhcpsd
Length of output: 1369
🏁 Script executed:
#!/bin/bash
# Check what calls this function and if it's guarded
rg -B10 'unpriv_dispatch_copyout_ifaddrs|ifaddrs' src/unpriv.c | head -40Repository: NetworkConfiguration/dhcpsd
Length of output: 862
🏁 Script executed:
#!/antml/bash
# Check if getifaddrs or the entire ifaddrs code path is BSD-guarded
grep -n 'getifaddrs\|freeifaddrs\|#ifdef\|#endif' src/unpriv.c | head -30Repository: NetworkConfiguration/dhcpsd
Length of output: 584
🏁 Script executed:
#!/bin/bash
# Let me check the exact function signature and if it's platform-guarded
sed -n '89,120p' src/unpriv.c | cat -nRepository: NetworkConfiguration/dhcpsd
Length of output: 1017
🏁 Script executed:
#!/bin/bash
# Check if getifaddrs is used unconditionally
rg -n 'getifaddrs|freeifaddrs' src/unpriv.cRepository: NetworkConfiguration/dhcpsd
Length of output: 434
🏁 Script executed:
#!/bin/bash
# Check if getifaddrs is available on Linux and how it's used
rg -n 'ifdef.*getifaddrs|ifndef.*getifaddrs|getifaddrs' src/unpriv.cRepository: NetworkConfiguration/dhcpsd
Length of output: 352
🏁 Script executed:
#!/bin/bash
# Check if there are any preprocessor guards around the entire getifaddrs functionality
sed -n '320,340p' src/unpriv.c | cat -n
sed -n '338,400p' src/unpriv.c | cat -nRepository: NetworkConfiguration/dhcpsd
Length of output: 2728
🏁 Script executed:
#!/antml/bash
# Let's verify if the code is actually compiled by checking recent builds
# Check if there are any disabled tests or ifdef guards we're missing
grep -n 'ifdef\|ifndef\|if defined' src/unpriv.c | sort -uRepository: NetworkConfiguration/dhcpsd
Length of output: 167
Add fallback definition for ALIGN macro.
The ALIGN macro is not defined in sys/param.h on Linux systems. While it exists on BSD platforms, the code uses it unconditionally without platform guards, causing compilation to fail on Linux (including the active Ubuntu CI builds).
Add fallback definition after includes
`#include` "unpriv.h"
+/* Fallback if ALIGN is not provided by sys/param.h */
+#ifndef ALIGN
+#define ALIGN(x) (((x) + (sizeof(long) - 1)) & ~(sizeof(long) - 1))
+#endif
+
`#define` U_GETIFADDRS 1Affects: lines 122-124, 365-367, 416-421
🧰 Tools
🪛 GitHub Actions: Build
[error] 122-122: clang error: call to undeclared function 'ALIGN' (ISO C99 and later do not support implicit function declarations) [-Wimplicit-function-declaration]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/unpriv.c` around lines 122 - 124, Add a portable fallback for the ALIGN
macro in src/unpriv.c by inserting a conditional definition after the existing
includes: if ALIGN is not already defined, define it to round its argument up to
the machine word boundary using uintptr_t (i.e., add a `#ifndef` ALIGN / `#define`
ALIGN(...) / `#endif` block that computes ((uintptr_t)(x) + (sizeof(long)-1)) &
~(sizeof(long)-1)). Also ensure <stdint.h> is included if uintptr_t is not
available; this fixes uses like ALIGN(sizeof(*ifa)), ALIGN(IFNAMSIZ) and
ALIGN(sizeof(salen) * IFA_NADDRS) without changing the call sites.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dhcp.c (1)
1608-1616:⚠️ Potential issue | 🟡 MinorConsider propagating
dhcp_outputfailures.
dhcp_outputnow returnsssize_t(−1 on realloc/send failure), but the caller still discards it. In particular, ifdhcp_outputfails before committing, the code then callsdhcp_commit_leasefor non-NAK paths as if the reply were sent. The lease gets persisted and the client sees silence. Consider checking the result and skipping commit on −1 so state stays consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dhcp.c` around lines 1608 - 1616, dhcp_output now returns ssize_t and failures must be propagated: in the switch branch handling DHCP_OFFER/DHCP_ACK/DHCP_NAK call dhcp_output(ifp, lease, type, msg, bootp, len) and check its return value (e.g., ssize_t rv); if rv == -1 do not call dhcp_commit_lease(ctx, lease, bootp, len) and handle the error path (log and/or return) so you don't persist a lease when the reply failed to send; ensure this check is applied for both OFFER and ACK paths while NAK still skips commit.
🧹 Nitpick comments (5)
src/dhcp.c (2)
41-41: Include path style inconsistency.Other headers in this file (and
"priv.h"insrc/dhcpsd.cline 56) are included without thesrc/prefix. Prefer#include "priv.h"for consistency, relying on the include path configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dhcp.c` at line 41, The include in src/dhcp.c currently uses a prefixed path ("src/priv.h") which is inconsistent with the other includes and with how priv.h is included in dhcpsd.c; update the include directive in dhcp.c to use the unprefixed form (`#include` "priv.h") so it matches the project's include style and relies on the configured include path; ensure no other occurrences in dhcp.c use the src/ prefix for local headers.
1670-1673: Dead#if 1wrapper.The
#if 1 ...#endif`` is equivalent to unconditional inclusion. Either drop the guard or restore the previous behavior-selecting macro.♻️ Proposed cleanup
-#if 1 logwarnx("dhcp: interface not found 0x%x %s", bootp->xid, inet_ntoa(from.sin_addr)); -#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dhcp.c` around lines 1670 - 1673, Remove the dead preprocessing guard around the log line: delete the surrounding "#if 1" and "#endif" so the call to logwarnx("dhcp: interface not found 0x%x %s", bootp->xid, inet_ntoa(from.sin_addr)); is unconditional (or if you intend conditional logging, replace "#if 1" with the appropriate feature/debug macro instead). This touches the logwarnx call referencing bootp->xid and inet_ntoa(from.sin_addr) in dhcp.c.src/route.c (1)
76-78:logwarnxis noisy for a normal state transition.An interface being brought down is a routine operational event, not a warning-worthy condition. The netlink backend uses
loginfoxfor the equivalent path (src/netlink.c:154). Suggest aligning severity for consistency.♻️ Proposed tweak
- logwarnx("%s: interface is down", ifp->if_name); + loginfox("%s: interface is down", ifp->if_name);Apply the same change to the
IFAN_DEPARTURElog at line 103 if desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/route.c` around lines 76 - 78, Replace the noisy warning-level log when an interface goes down by changing the call site that currently uses logwarnx("%s: interface is down", ifp->if_name) to use loginfox so the event is logged at info level; update the same pattern for the IFAN_DEPARTURE message (the other logwarnx instance near the IFAN_DEPARTURE handling) to maintain consistent severity, keeping the surrounding operations TAILQ_REMOVE(ctx->ctx_ifaces, ifp, if_next) and if_free(ifp) unchanged.src/netlink.c (1)
71-84: Mixingsize_tandssize_tobscures the error path.Assigning
recvmsg()'sssize_treturn into asize_tand then casting back tossize_tfor the-1comparison is error-prone. Usingssize_tthroughout is clearer and avoids UB concerns about lossy conversions.♻️ Proposed cleanup
- size_t len; + ssize_t nread; + size_t len; ... recv_again: - len = (size_t)recvmsg(fd, &msg, flags); - if (len == 0 || (ssize_t)len == -1) - return (int)len; + nread = recvmsg(fd, &msg, flags); + if (nread <= 0) + return (int)nread; + len = (size_t)nread;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/netlink.c` around lines 71 - 84, The recvmsg return value is stored in a size_t (len) which is then cast back to ssize_t—change len to type ssize_t in the recv_again block so recvmsg's signed return is handled natively; update the checks to use len == 0 and len == -1 (or len <= 0 if you prefer a single check), keep the same return semantics (cast the ssize_t return to int where the code returns), and leave the subsequent checks for msg.msg_namelen and nladdr.nl_pid unchanged; specifically modify the local variable declaration for len and the conditional that tests recvmsg(fd, &msg, flags) so no unsigned-to-signed casts are performed.src/dhcpsd.c (1)
365-381: LGTM —-wflag and getopt error handling.Switching
f_flagtobooland adding the?exit path for getopt errors are appropriate. Consider emitting a brief usage line beforegoto exitso an unknown option gives the user actionable feedback rather than a silent exit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dhcpsd.c` around lines 365 - 381, The getopt '?' handler currently jumps to exit silently; update the case '?' in the getopt switch (OPTS handling) to print a short usage message to stderr before the existing goto exit so unknown options give actionable feedback—either call the program's existing usage/print_usage function (if available) or emit a one-line usage via fprintf(stderr, ...) that lists the supported flags (e.g. -d -f -p -w) and then goto exit; keep the change localized to the getopt switch (case '?') near OPTS and f_flag handling.
🤖 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/dhcp.c`:
- Around line 865-874: The calculation of len from ifp->if_mtu can underflow or
produce a buffer smaller than struct bootp causing a subsequent memset overflow;
before using ifp->if_mtu, validate and clamp it: compute min_payload =
sizeof(struct bootp) and required_mtu = min_payload + sizeof(struct udphdr) +
sizeof(struct ip), then if ifp->if_mtu is 0 or less than required_mtu treat it
as required_mtu (or skip realloc and keep existing ctx->dhcp_bootplen if
larger), set len = ifp->if_mtu - sizeof(struct udphdr) - sizeof(struct ip) only
after clamping, and ensure the resulting len >= sizeof(struct bootp) before
calling realloc on ctx->dhcp_bootp and updating ctx->dhcp_bootplen to prevent
heap overflow from the subsequent memset of the bootp structure.
In `@src/dhcpsd.c`:
- Around line 526-539: The variable npools is left uninitialized when
DHCPSD_WAITIF is set because the earlier initialization is skipped; initialize
npools to 0 at its declaration (the size_t npools variable) so it has a defined
value before the TAILQ_FOREACH loop and subsequent check against ctx.ctx_options
& DHCPSD_WAITIF and the npools == 0 test; update the declaration near where
size_t npools is defined (so usages in TAILQ_FOREACH, dhcpsd_configure_pools
handling, and the later conditional remain safe).
In `@src/netlink.c`:
- Line 3: Fix the typo in the file header comment by changing "dhcpsd -
netlink(7) supprt" to "dhcpsd - netlink(7) support" in src/netlink.c (and make
the identical correction in the header comment in src/route.c); locate the
top-of-file comment that contains the exact string "dhcpsd - netlink(7) supprt"
and replace "supprt" with "support".
- Around line 120-121: The re-entry condition in netlink_get is inverted: since
netlink_handle always passes fd = lctx->link_fd, the test (lctx->link_fd != fd)
is always false so multipart NLM_F_MULTI messages aren't drained; fix by
changing the condition to (lctx != NULL && lctx->link_fd == fd) so recv_again is
taken and the sequence is drained in one call, or if single-shot behavior is
intentional, add a clear comment next to the condition explaining why
lctx->link_fd != fd is used to intentionally defer multipart handling to the
next eloop iteration; update the check inside netlink_get where the goto
recv_again is decided to reference lctx and fd accordingly.
- Around line 216-218: The setsockopt call for NETLINK_BROADCAST_ERROR on
lctx->link_fd currently only logs a generic message via logerr("%s:
NETLINK_BROADCAST_ERROR", __func__) and continues; update this to handle
failures explicitly: if setsockopt(...) == -1, capture errno and include it in
the log (use strerror(errno)) and treat unsupported option (e.g.,
ENOPROTOOPT/ENOTSUP) as a non-fatal condition with a clear comment that older
kernels may not support NETLINK_BROADCAST_ERROR, but for any other errno
(unexpected failures) either return an error from the surrounding initialization
function or abort initialization so callers don't silently run with dropped link
events; reference the exact call sites/identifiers: setsockopt, lctx->link_fd,
SOL_NETLINK, NETLINK_BROADCAST_ERROR, logerr, and __func__ when making the
change.
- Line 33: Remove the kernel-internal include by deleting the line that pulls in
<asm/types.h>; instead rely on the userspace-exposed headers already present
(e.g., <linux/netlink.h>, <linux/rtnetlink.h>) which define netlink types like
struct nlmsghdr and struct ifinfomsg, so remove the `#include` <asm/types.h> from
src/netlink.c to avoid using kernel-only headers.
In `@src/route.c`:
- Line 83: The function signature for route_dispatch_ifannounce(struct link_ctx
*lctx, const struct rt_msghdr *rtm) has trailing whitespace after the closing
parenthesis; remove the extra space so the declaration ends immediately after
the closing paren. Update the signature in src/route.c (function
route_dispatch_ifannounce) to eliminate the trailing space and ensure consistent
formatting.
- Around line 116-123: The check for the event uses logerrx("%s: unexpeced event
%d", __func__, e) but does not return, so execution continues into
read(lctx->link_fd, &rtm, sizeof(rtm)); update the branch handling ELE_READ in
src/route.c to either return immediately after logging (add a return; after
logerrx) or replace the log with an assertion-style fatal log to stop execution;
also fix the typo "unexpeced" → "unexpected" in the log message.
---
Outside diff comments:
In `@src/dhcp.c`:
- Around line 1608-1616: dhcp_output now returns ssize_t and failures must be
propagated: in the switch branch handling DHCP_OFFER/DHCP_ACK/DHCP_NAK call
dhcp_output(ifp, lease, type, msg, bootp, len) and check its return value (e.g.,
ssize_t rv); if rv == -1 do not call dhcp_commit_lease(ctx, lease, bootp, len)
and handle the error path (log and/or return) so you don't persist a lease when
the reply failed to send; ensure this check is applied for both OFFER and ACK
paths while NAK still skips commit.
---
Nitpick comments:
In `@src/dhcp.c`:
- Line 41: The include in src/dhcp.c currently uses a prefixed path
("src/priv.h") which is inconsistent with the other includes and with how priv.h
is included in dhcpsd.c; update the include directive in dhcp.c to use the
unprefixed form (`#include` "priv.h") so it matches the project's include style
and relies on the configured include path; ensure no other occurrences in dhcp.c
use the src/ prefix for local headers.
- Around line 1670-1673: Remove the dead preprocessing guard around the log
line: delete the surrounding "#if 1" and "#endif" so the call to logwarnx("dhcp:
interface not found 0x%x %s", bootp->xid, inet_ntoa(from.sin_addr)); is
unconditional (or if you intend conditional logging, replace "#if 1" with the
appropriate feature/debug macro instead). This touches the logwarnx call
referencing bootp->xid and inet_ntoa(from.sin_addr) in dhcp.c.
In `@src/dhcpsd.c`:
- Around line 365-381: The getopt '?' handler currently jumps to exit silently;
update the case '?' in the getopt switch (OPTS handling) to print a short usage
message to stderr before the existing goto exit so unknown options give
actionable feedback—either call the program's existing usage/print_usage
function (if available) or emit a one-line usage via fprintf(stderr, ...) that
lists the supported flags (e.g. -d -f -p -w) and then goto exit; keep the change
localized to the getopt switch (case '?') near OPTS and f_flag handling.
In `@src/netlink.c`:
- Around line 71-84: The recvmsg return value is stored in a size_t (len) which
is then cast back to ssize_t—change len to type ssize_t in the recv_again block
so recvmsg's signed return is handled natively; update the checks to use len ==
0 and len == -1 (or len <= 0 if you prefer a single check), keep the same return
semantics (cast the ssize_t return to int where the code returns), and leave the
subsequent checks for msg.msg_namelen and nladdr.nl_pid unchanged; specifically
modify the local variable declaration for len and the conditional that tests
recvmsg(fd, &msg, flags) so no unsigned-to-signed casts are performed.
In `@src/route.c`:
- Around line 76-78: Replace the noisy warning-level log when an interface goes
down by changing the call site that currently uses logwarnx("%s: interface is
down", ifp->if_name) to use loginfox so the event is logged at info level;
update the same pattern for the IFAN_DEPARTURE message (the other logwarnx
instance near the IFAN_DEPARTURE handling) to maintain consistent severity,
keeping the surrounding operations TAILQ_REMOVE(ctx->ctx_ifaces, ifp, if_next)
and if_free(ifp) 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: 507413c2-998c-4f59-9adf-1a1f2e75ea8d
📒 Files selected for processing (12)
configuresrc/common.csrc/common.hsrc/dhcp.csrc/dhcpsd.csrc/if.csrc/netlink.csrc/plugins/auto.csrc/plugins/lua.csrc/route.csrc/seccomp.csrc/unpriv.c
🚧 Files skipped from review as they are similar to previous changes (5)
- src/plugins/auto.c
- configure
- src/plugins/lua.c
- src/unpriv.c
- src/if.c
| len = (size_t)ifp->if_mtu - sizeof(struct udphdr) - sizeof(struct ip); | ||
| if (len > ctx->dhcp_bootplen) { | ||
| void *n = realloc(ctx->dhcp_bootp, len); | ||
| if (n == NULL) { | ||
| logerr("%s: realloc", __func__); | ||
| return -1; | ||
| } | ||
| ctx->dhcp_bootp = n; | ||
| ctx->dhcp_bootplen = len; | ||
| } |
There was a problem hiding this comment.
Validate MTU to prevent short-buffer overflow.
len = (size_t)ifp->if_mtu - sizeof(struct udphdr) - sizeof(struct ip) is unchecked. For ifp->if_mtu values that are 0 (uninitialized, plausible under -w before the interface is fully configured) or <= 28, the subtraction wraps and produces a huge size_t, causing realloc to fail and the function to error out — recoverable. However, for MTUs in the narrow range of ~29..~327, the realloc succeeds with a buffer smaller than sizeof(struct bootp), and the very next memset(bootp, 0, sizeof(*bootp)) at line 877 overflows the heap. Clamp to a sensible floor.
🐛 Proposed fix
- len = (size_t)ifp->if_mtu - sizeof(struct udphdr) - sizeof(struct ip);
+ if (ifp->if_mtu <= 0 ||
+ (size_t)ifp->if_mtu <= sizeof(struct udphdr) + sizeof(struct ip)) {
+ logerrx("%s: interface mtu too small: %d",
+ ifp->if_name, ifp->if_mtu);
+ return -1;
+ }
+ len = (size_t)ifp->if_mtu - sizeof(struct udphdr) - sizeof(struct ip);
+ if (len < sizeof(struct bootp))
+ len = sizeof(struct bootp);
if (len > ctx->dhcp_bootplen) {📝 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.
| len = (size_t)ifp->if_mtu - sizeof(struct udphdr) - sizeof(struct ip); | |
| if (len > ctx->dhcp_bootplen) { | |
| void *n = realloc(ctx->dhcp_bootp, len); | |
| if (n == NULL) { | |
| logerr("%s: realloc", __func__); | |
| return -1; | |
| } | |
| ctx->dhcp_bootp = n; | |
| ctx->dhcp_bootplen = len; | |
| } | |
| if (ifp->if_mtu <= 0 || | |
| (size_t)ifp->if_mtu <= sizeof(struct udphdr) + sizeof(struct ip)) { | |
| logerrx("%s: interface mtu too small: %d", | |
| ifp->if_name, ifp->if_mtu); | |
| return -1; | |
| } | |
| len = (size_t)ifp->if_mtu - sizeof(struct udphdr) - sizeof(struct ip); | |
| if (len < sizeof(struct bootp)) | |
| len = sizeof(struct bootp); | |
| if (len > ctx->dhcp_bootplen) { | |
| void *n = realloc(ctx->dhcp_bootp, len); | |
| if (n == NULL) { | |
| logerr("%s: realloc", __func__); | |
| return -1; | |
| } | |
| ctx->dhcp_bootp = n; | |
| ctx->dhcp_bootplen = len; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dhcp.c` around lines 865 - 874, The calculation of len from ifp->if_mtu
can underflow or produce a buffer smaller than struct bootp causing a subsequent
memset overflow; before using ifp->if_mtu, validate and clamp it: compute
min_payload = sizeof(struct bootp) and required_mtu = min_payload +
sizeof(struct udphdr) + sizeof(struct ip), then if ifp->if_mtu is 0 or less than
required_mtu treat it as required_mtu (or skip realloc and keep existing
ctx->dhcp_bootplen if larger), set len = ifp->if_mtu - sizeof(struct udphdr) -
sizeof(struct ip) only after clamping, and ensure the resulting len >=
sizeof(struct bootp) before calling realloc on ctx->dhcp_bootp and updating
ctx->dhcp_bootplen to prevent heap overflow from the subsequent memset of the
bootp structure.
| TAILQ_FOREACH(ifp, ctx.ctx_ifaces, if_next) { | ||
| if (!(ifp->if_flags & IF_ACTIVE)) | ||
| continue; | ||
| PLUGIN_FOREACH(&ctx, p) | ||
| { | ||
| if (p->p_configure_pools == NULL) | ||
| continue; | ||
| ssize_t n = p->p_configure_pools(p, ifp); | ||
| if (n == -1 && argc != 0) | ||
| goto exit; | ||
| if (n == -1 || n == 0) | ||
| continue; | ||
| /* XXX When we grow DHCPv6 only open BPF if we configure | ||
| * a DHCPv4 pool. */ | ||
| if (dhcp_openbpf(ifp) == -1) | ||
| goto exit; | ||
| /* First plugin with config wins */ | ||
| npools += (size_t)n; | ||
| break; | ||
| } | ||
| ssize_t n = dhcpsd_configure_pools(ifp); | ||
| if (n == -1 && argc != 0) | ||
| goto exit; | ||
| if (n == -1 || n == 0) | ||
| continue; | ||
| npools += (size_t)n; | ||
| } | ||
| if (npools == 0) { | ||
| if (!(ctx.ctx_options & DHCPSD_WAITIF) && npools == 0) { | ||
| logerrx("no pools, nothing to serve"); | ||
| goto exit; | ||
| } |
There was a problem hiding this comment.
npools is uninitialized when DHCPSD_WAITIF is set.
When -w is in effect, the block at lines 490–514 is skipped, so npools = 0 (assigned at line 492) never executes. Execution reaches the TAILQ_FOREACH at line 526 and eventually the npools == 0 check at line 536, but npools at that point is whatever size_t npools; was declared as at line 351 — uninitialized. Under -w, ctx_ifaces is empty so the TAILQ_FOREACH body doesn't run and npools is never written. The subsequent check short-circuits on !(ctx.ctx_options & DHCPSD_WAITIF), so no undefined-behavior read happens today, but this is fragile: any future edit that reads npools outside the short-circuit (or reorders the condition) will read garbage.
Please initialize npools = 0; at declaration.
🐛 Proposed fix
- size_t npools;
+ size_t npools = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dhcpsd.c` around lines 526 - 539, The variable npools is left
uninitialized when DHCPSD_WAITIF is set because the earlier initialization is
skipped; initialize npools to 0 at its declaration (the size_t npools variable)
so it has a defined value before the TAILQ_FOREACH loop and subsequent check
against ctx.ctx_options & DHCPSD_WAITIF and the npools == 0 test; update the
declaration near where size_t npools is defined (so usages in TAILQ_FOREACH,
dhcpsd_configure_pools handling, and the later conditional remain safe).
| @@ -0,0 +1,240 @@ | |||
| /* SPDX-License-Identifier: BSD-2-Clause */ | |||
| /* | |||
| * dhcpsd - netlink(7) supprt | |||
There was a problem hiding this comment.
Typo in header: "supprt" → "support". Same typo exists in src/route.c header.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/netlink.c` at line 3, Fix the typo in the file header comment by changing
"dhcpsd - netlink(7) supprt" to "dhcpsd - netlink(7) support" in src/netlink.c
(and make the identical correction in the header comment in src/route.c); locate
the top-of-file comment that contains the exact string "dhcpsd - netlink(7)
supprt" and replace "supprt" with "support".
| if (setsockopt(lctx->link_fd, SOL_NETLINK, NETLINK_BROADCAST_ERROR, &on, | ||
| sizeof(on)) == -1) | ||
| logerr("%s: NETLINK_BROADCAST_ERROR", __func__); |
There was a problem hiding this comment.
NETLINK_BROADCAST_ERROR failure swallowed.
Setting this sockopt is important for detecting kernel-side broadcast delivery failures (e.g., ENOBUFS). The code logs and continues on failure. If that's intentional (older kernels may not support it), a comment would help; otherwise consider failing or at least documenting that dropped link events can result in stale interface state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/netlink.c` around lines 216 - 218, The setsockopt call for
NETLINK_BROADCAST_ERROR on lctx->link_fd currently only logs a generic message
via logerr("%s: NETLINK_BROADCAST_ERROR", __func__) and continues; update this
to handle failures explicitly: if setsockopt(...) == -1, capture errno and
include it in the log (use strerror(errno)) and treat unsupported option (e.g.,
ENOPROTOOPT/ENOTSUP) as a non-fatal condition with a clear comment that older
kernels may not support NETLINK_BROADCAST_ERROR, but for any other errno
(unexpected failures) either return an error from the surrounding initialization
function or abort initialization so callers don't silently run with dropped link
events; reference the exact call sites/identifiers: setsockopt, lctx->link_fd,
SOL_NETLINK, NETLINK_BROADCAST_ERROR, logerr, and __func__ when making the
change.
| if (e != ELE_READ) { | ||
| logerrx("%s: unexpeced event %d", __func__, e); | ||
| } | ||
| len = read(lctx->link_fd, &rtm, sizeof(rtm)); | ||
| if (len == -1) { | ||
| logerr("%s: read", __func__); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing early return after unexpected event.
logerrx logs but does not exit, so execution falls through to read(lctx->link_fd, ...) even though the event indicated something other than ELE_READ. In most configurations this is harmless (read will EAGAIN or behave normally), but the intent is clearly to bail. Either add a return; or make the check an assertion-style log without the fallthrough.
🐛 Proposed fix
if (e != ELE_READs) {
- logerrx("%s: unexpeced event %d", __func__, e);
+ logerrx("%s: unexpected event %d", __func__, e);
+ return;
}Also fixes the "unexpeced" → "unexpected" typo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/route.c` around lines 116 - 123, The check for the event uses
logerrx("%s: unexpeced event %d", __func__, e) but does not return, so execution
continues into read(lctx->link_fd, &rtm, sizeof(rtm)); update the branch
handling ELE_READ in src/route.c to either return immediately after logging (add
a return; after logerrx) or replace the log with an assertion-style fatal log to
stop execution; also fix the typo "unexpeced" → "unexpected" in the log message.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dhcp.c (1)
1604-1615:⚠️ Potential issue | 🟠 Major
dhcp_outputreturn value is discarded; lease is committed even if the reply was never sent.
dhcp_outputnow returnsssize_tand can fail onrealloc,dhcp_addoptions,sendto, orpriv_sendbpf. The caller ignores it, so on BPF/UDP send failure we still fall through todhcp_commit_lease, persisting a lease the client never acknowledged (and possibly never received). At minimum skip the commit when the response could not be sent.🐛 Proposed fix
case DHCP_NAK: - dhcp_output(ifp, lease, type, msg, bootp, len); - /* Commit the lease after sending it as it might - * be a slow operation depending on what the plugins do. */ - if (type != DHCP_NAK) + if (dhcp_output(ifp, lease, type, msg, bootp, len) == -1) + break; + /* Commit the lease after sending it as it might + * be a slow operation depending on what the plugins do. */ + if (type != DHCP_NAK) dhcp_commit_lease(ctx, lease, bootp, len); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dhcp.c` around lines 1604 - 1615, The code currently ignores dhcp_output's ssize_t return value and always calls dhcp_commit_lease, which can persist a lease even if dhcp_output failed (realloc/dhcp_addoptions/sendto/priv_sendbpf); change the switch handling so you capture the result of dhcp_output(…) into a ssize_t (e.g. rc) and only call dhcp_commit_lease(ctx, lease, bootp, len) when rc >= 0 (or otherwise indicates success), skipping the commit and handling/logging the error when dhcp_output fails; adjust the switch case around DHCP_BOOTREQUEST/DHCP_OFFER/DHCP_ACK/DHCP_NAK accordingly to use the return value.
🧹 Nitpick comments (4)
src/unpriv.c (1)
190-214: Serializing the entirestruct interfaceacross IPC leaks pointers and couples layouts.
unpriv_dispatch_learnifsendsbuf = ifp; len = sizeof(*ifp)and the client validatesrdata_len != sizeof(*ifp). However,struct interface(seesrc/if.h:53-70) contains aTAILQ_ENTRY(internal linked-list pointers), several pointer fields (if_ctx,if_pools,if_bpf), and a function pointer (if_output). Transmitting these across the privilege boundary leaks privileged-side addresses (weakening ASLR as a mitigation) and tightly couples the wire format to struct layout — any future field added/reordered silently breaks the protocol despite thesizeofcheck passing on both ends.
src/priv.c:110-140already demonstrates the correct pattern: serialize only the meaningful fields (if_hwtype,if_hwlen,if_hwaddr,if_mtu,if_name). Consider mirroring that — e.g., define a smallstruct unpriv_interfacewire type with just name/hwtype/hwlen/hwaddr/mtu, populate it on the server, and parse it on the client. It also makes thesizeofequality check a precise contract rather than an accidental one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/unpriv.c` around lines 190 - 214, The code currently transmits a full struct interface blob in unpriv_dispatch_learnif/unpriv.c which leaks internal pointers and couples the wire format to in-memory layout; replace that with a dedicated wire-format struct (e.g., struct unpriv_interface) containing only the serialized fields (if_name, if_hwtype, if_hwlen, fixed-size if_hwaddr array, if_mtu), have the privileged side populate and send that struct (see priv.c:110-140 for the pattern), update the client-side validation to check rdata_len == sizeof(struct unpriv_interface) and memcpy/strlcpy only those fields into struct interface (copy hwaddr using the provided hwlen, validate bounds), and stop sending the full struct interface across the IPC boundary to avoid leaking pointers/function pointers and layout coupling.src/netlink.c (2)
217-223: Comment references "Routing socket"; this is a netlink socket.Looks like a copy-paste from
src/route.c. Minor, but worth tightening to avoid confusion:📝 Proposed fix
- /* Routing socket can overflow if the kernel sends too many messages. - * We need to reliably track state and if we can't we need to know. */ + /* The netlink socket can overflow if the kernel sends too many + * messages. We need to reliably track state, and if we can't we + * need to know. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/netlink.c` around lines 217 - 223, The comment incorrectly calls this a "Routing socket"—change it to "Netlink socket" to avoid confusion; update the comment above the NETLINK_BROADCAST_ERROR setsockopt block to reference "Netlink socket can overflow" (keeping the rest of the logic intact), and ensure the surrounding identifiers (lctx->link_fd, NETLINK_BROADCAST_ERROR, logerr, __func__) remain unchanged.
114-117:cberrors are silently overwritten by later messages in the same batch.
r = cb(lctx, nlm);stores only the last callback's return. If one message fails (e.g.EBADMSGreturned bynetlink_link) and a subsequent message in the samerecvmsgbatch succeeds, the error is lost. Consider breaking out on-1, or at minimum preserving a sticky failure:♻️ Proposed fix
- if (cb == NULL) - continue; - r = cb(lctx, nlm); + if (cb == NULL) + continue; + if (cb(lctx, nlm) == -1) + r = -1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/netlink.c` around lines 114 - 117, The callback result from cb(lctx, nlm) is currently overwritten by later callbacks so earlier errors (e.g. from netlink_link) are lost; change the loop that invokes cb to preserve the first negative error or break immediately on error: after calling r = cb(lctx, nlm), if r < 0 either break out of the loop or store it into a sticky variable (e.g. err = err ?: r) and avoid overwriting err with subsequent non-negative returns, then return that preserved error after the recvmsg/batch processing completes.src/dhcpsd.c (1)
305-334: Redundantargc/ctx_argccheck between helper and caller.
dhcpsd_configure_poolsalready returns-1only whenctx->ctx_argc != 0(line 320), yet the caller at lines 529–531 re-testsn == -1 && argc != 0. Either drop the inner guard and let the caller decide, or drop the caller guard — splitting the policy across both sites makes future maintenance error-prone. Prefer keeping the policy in one place (typically the caller, so the helper just propagates-1).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dhcpsd.c` around lines 305 - 334, dhcpsd_configure_pools currently tests ctx->ctx_argc before returning -1 which duplicates the caller's policy; change the helper to simply propagate errors by replacing the guarded check (if (n == -1 && ctx->ctx_argc != 0) return -1;) with an unconditional error propagation (if (n == -1) return -1;) in dhcpsd_configure_pools so the caller retains responsibility for inspecting argc/ctx_argc and policy stays in one place.
🤖 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/dhcpsd.c`:
- Around line 490-513: When DHCPSD_WAITIF is set the validation block is skipped
causing unknown interface names to be accepted; modify the logic around
ctx.ctx_options & DHCPSD_WAITIF so that requested argv names are recorded and
validated later: capture the requested names into a pending list when parsing
argv (use a new pending structure or mark entries in argv), call
if_learnifaces(&ctx) as now, but also validate pending names against
ctx.ctx_ifaces when interfaces are learned (e.g. on initial learn in
if_learnifaces and inside the netlink_link / RTM_NEWLINK handler), and log or
error for names that never appear (use logerrx or a debug log listing pending
names) rather than silently ignoring them. Ensure references: DHCPSD_WAITIF
check, if_learnifaces, ctx.ctx_ifaces, netlink_link/RTM_NEWLINK handlers are
updated to consult the pending list.
In `@src/unpriv.c`:
- Around line 511-515: The truncation check after calling strlcpy is off-by-one:
change the condition that validates nlen so it treats values >=
sizeof(ifp->if_name) as truncation. Specifically, in the block using nlen =
strlcpy(ifp->if_name, ifa->ifa_name, sizeof(ifp->if_name)) replace the current
`nlen > sizeof(ifp->if_name)` test with `nlen >= sizeof(ifp->if_name)` and keep
the same errno = EINVAL; goto err; handling to match the correct check used
elsewhere (see the similar check at the earlier reference).
---
Outside diff comments:
In `@src/dhcp.c`:
- Around line 1604-1615: The code currently ignores dhcp_output's ssize_t return
value and always calls dhcp_commit_lease, which can persist a lease even if
dhcp_output failed (realloc/dhcp_addoptions/sendto/priv_sendbpf); change the
switch handling so you capture the result of dhcp_output(…) into a ssize_t (e.g.
rc) and only call dhcp_commit_lease(ctx, lease, bootp, len) when rc >= 0 (or
otherwise indicates success), skipping the commit and handling/logging the error
when dhcp_output fails; adjust the switch case around
DHCP_BOOTREQUEST/DHCP_OFFER/DHCP_ACK/DHCP_NAK accordingly to use the return
value.
---
Nitpick comments:
In `@src/dhcpsd.c`:
- Around line 305-334: dhcpsd_configure_pools currently tests ctx->ctx_argc
before returning -1 which duplicates the caller's policy; change the helper to
simply propagate errors by replacing the guarded check (if (n == -1 &&
ctx->ctx_argc != 0) return -1;) with an unconditional error propagation (if (n
== -1) return -1;) in dhcpsd_configure_pools so the caller retains
responsibility for inspecting argc/ctx_argc and policy stays in one place.
In `@src/netlink.c`:
- Around line 217-223: The comment incorrectly calls this a "Routing
socket"—change it to "Netlink socket" to avoid confusion; update the comment
above the NETLINK_BROADCAST_ERROR setsockopt block to reference "Netlink socket
can overflow" (keeping the rest of the logic intact), and ensure the surrounding
identifiers (lctx->link_fd, NETLINK_BROADCAST_ERROR, logerr, __func__) remain
unchanged.
- Around line 114-117: The callback result from cb(lctx, nlm) is currently
overwritten by later callbacks so earlier errors (e.g. from netlink_link) are
lost; change the loop that invokes cb to preserve the first negative error or
break immediately on error: after calling r = cb(lctx, nlm), if r < 0 either
break out of the loop or store it into a sticky variable (e.g. err = err ?: r)
and avoid overwriting err with subsequent non-negative returns, then return that
preserved error after the recvmsg/batch processing completes.
In `@src/unpriv.c`:
- Around line 190-214: The code currently transmits a full struct interface blob
in unpriv_dispatch_learnif/unpriv.c which leaks internal pointers and couples
the wire format to in-memory layout; replace that with a dedicated wire-format
struct (e.g., struct unpriv_interface) containing only the serialized fields
(if_name, if_hwtype, if_hwlen, fixed-size if_hwaddr array, if_mtu), have the
privileged side populate and send that struct (see priv.c:110-140 for the
pattern), update the client-side validation to check rdata_len == sizeof(struct
unpriv_interface) and memcpy/strlcpy only those fields into struct interface
(copy hwaddr using the provided hwlen, validate bounds), and stop sending the
full struct interface across the IPC boundary to avoid leaking pointers/function
pointers and layout coupling.
🪄 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: 0c71ebaa-45d0-429e-9f72-0ef01b1c62aa
📒 Files selected for processing (5)
src/dhcp.csrc/dhcpsd.csrc/netlink.csrc/route.csrc/unpriv.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/route.c
| if (!(ctx.ctx_options & DHCPSD_WAITIF)) { | ||
| if_learnifaces(&ctx); | ||
| TAILQ_FOREACH(ifp, ctx.ctx_ifaces, if_next) { | ||
| if (argc == 0) | ||
| ifp->if_flags |= IF_ACTIVE; | ||
| else { | ||
| for (ch = 0; ch < argc; ch++) { | ||
| if (strcmp(argv[ch], ifp->if_name) == | ||
| 0) { | ||
| ifp->if_flags |= IF_ACTIVE; | ||
| argv[ch][0] = '\0'; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (ch = 0; ch < argc; ch++) { | ||
| if (argv[ch][0] != '\0') { | ||
| logerrx("%s: no such interface", argv[ch]); | ||
| goto exit; | ||
| for (ch = 0; ch < argc; ch++) { | ||
| if (argv[ch][0] != '\0') { | ||
| logerrx("%s: no such interface", argv[ch]); | ||
| goto exit; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Under -w, interface-name arguments are never validated.
When DHCPSD_WAITIF is set, the entire block that matches argv[ch] against ctx_ifaces and reports "no such interface" is skipped. A user typo like dhcpsd -w eth0 bda0 (intending bdg0) will be silently accepted, and the daemon will wait forever for an interface that can never appear. Consider stashing the requested names and validating (or at least logging) them as interfaces arrive via netlink_link / RTM_NEWLINK, or printing a debug line listing the pending names so misconfigurations are diagnosable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dhcpsd.c` around lines 490 - 513, When DHCPSD_WAITIF is set the
validation block is skipped causing unknown interface names to be accepted;
modify the logic around ctx.ctx_options & DHCPSD_WAITIF so that requested argv
names are recorded and validated later: capture the requested names into a
pending list when parsing argv (use a new pending structure or mark entries in
argv), call if_learnifaces(&ctx) as now, but also validate pending names against
ctx.ctx_ifaces when interfaces are learned (e.g. on initial learn in
if_learnifaces and inside the netlink_link / RTM_NEWLINK handler), and log or
error for names that never appear (use logerrx or a debug log listing pending
names) rather than silently ignoring them. Ensure references: DHCPSD_WAITIF
check, if_learnifaces, ctx.ctx_ifaces, netlink_link/RTM_NEWLINK handlers are
updated to consult the pending list.
| if (cb == NULL) | ||
| continue; | ||
| r = cb(lctx, nlm); | ||
| } | ||
|
|
||
| if (again || !terminated) | ||
| goto recv_again; | ||
|
|
||
| return r; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the netlink.c file
find . -name "netlink.c" -type fRepository: NetworkConfiguration/dhcpsd
Length of output: 86
🏁 Script executed:
# Check the size of the netlink.c file
wc -l src/netlink.c 2>/dev/null || echo "File not found at standard path"Repository: NetworkConfiguration/dhcpsd
Length of output: 88
🏁 Script executed:
# Read the netlink_get function and surrounding code, focusing on the recv_again logic
sed -n '70,230p' src/netlink.cRepository: NetworkConfiguration/dhcpsd
Length of output: 3606
🏁 Script executed:
# Check if there are other places where MSG_DONTWAIT is used or comments about broadcast behavior
rg -n "MSG_DONTWAIT|NLM_F_MULTI|RTM_NEWLINK|RTM_DELLINK" src/netlink.cRepository: NetworkConfiguration/dhcpsd
Length of output: 334
🏁 Script executed:
# Look for any tests or documentation about netlink broadcast handling
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*test*" -o -name "*example*" \) | head -20Repository: NetworkConfiguration/dhcpsd
Length of output: 120
🏁 Script executed:
# Check git history or comments around the netlink_get function for context on the design intent
head -80 src/netlink.cRepository: NetworkConfiguration/dhcpsd
Length of output: 2475
Broadcast link events trigger spurious error logs due to unconditional recv_again loop with MSG_DONTWAIT.
For broadcast messages (RTM_NEWLINK, RTM_DELLINK), which are single, non-multipart messages:
again = (nlm->nlmsg_flags & NLM_F_MULTI)→0(assignment, not OR, overwrites any prior value each iteration)terminatedremainsfalse(only set onNLMSG_DONEorNLMSG_ERROR, neither sent for broadcasts)- Condition
if (again || !terminated)evaluates totrue→ unconditionalgoto recv_again - Next
recvmsg(fd, &msg, MSG_DONTWAIT)finds no waiting data → returns-1witherrno=EAGAIN netlink_getreturns-1netlink_handlelogs"netlink_get: Resource temporarily unavailable"for every legitimate event
The messages are processed correctly, but the spurious error logs mislead operators into thinking failures occurred. Additionally, each link event incurs an extra system call guaranteed to fail.
Fix:
- Distinguish between "no more data in buffer" (success after
MSG_DONTWAIT) and actual errors - Use bitwise OR (
|=) for theagainflag to preserve multipart state across iterations
Proposed fix
recv_again:
len = recvmsg(fd, &msg, flags);
- if (len == 0 || len == -1)
+ if (len == 0)
return (int)len;
+ if (len == -1) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK)
+ return 0;
+ return -1;
+ }
@@
- again = (nlm->nlmsg_flags & NLM_F_MULTI);
+ again |= (nlm->nlmsg_flags & NLM_F_MULTI);
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/netlink.c (2)
217-223:⚠️ Potential issue | 🟡 Minor
NETLINK_BROADCAST_ERRORsetsockopt failure is still silently swallowed.The preceding comment explicitly notes the reliability concern ("if we can't we need to know"), yet an error here only produces a log line and lets initialization proceed. Either fail
link_openon unexpected errnos, or treat onlyENOPROTOOPT/EOPNOTSUPPas acceptable (older kernels) and log errno explicitly so operators can distinguish "unsupported" from real misconfigurations.Proposed fix
`#ifdef` NETLINK_BROADCAST_ERROR if (setsockopt(lctx->link_fd, SOL_NETLINK, NETLINK_BROADCAST_ERROR, &on, - sizeof(on)) == -1) - logerr("%s: NETLINK_BROADCAST_ERROR", __func__); + sizeof(on)) == -1) { + if (errno != ENOPROTOOPT && errno != EOPNOTSUPP) { + logerr("%s: NETLINK_BROADCAST_ERROR", __func__); + return -1; + } + /* Older kernels may not support this option. */ + } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/netlink.c` around lines 217 - 223, link_open currently ignores setsockopt failures for NETLINK_BROADCAST_ERROR by only logging a generic message; instead, treat ENOPROTOOPT and EOPNOTSUPP as ok (older kernels) but for any other errno log the errno (use strerror(errno) or %d) and fail link_open (return an error/cleanup) so initialization does not silently proceed; update the setsockopt handling around NETLINK_BROADCAST_ERROR (referencing lctx->link_fd, setsockopt, NETLINK_BROADCAST_ERROR, and link_open) to check errno and act accordingly.
70-120:⚠️ Potential issue | 🟠 MajorSpurious
netlink_get: EAGAINerror log on every broadcast link event (still unresolved).For single, non-multipart broadcasts (
RTM_NEWLINK/RTM_DELLINK) delivered throughMSG_DONTWAIT:
- Line 89 uses assignment, so
againends up0after the loop (noNLM_F_MULTIflag on broadcast notifications).terminatedstaysfalse(neitherNLMSG_DONEnorNLMSG_ERRORis sent for broadcasts).- Line 119:
again || !terminatedistrue→ unconditionalgoto recv_again.- Line 71: next
recvmsg(…, MSG_DONTWAIT)returns-1/EAGAIN.- Line 72-73 returns
-1without distinguishing EAGAIN, sonetlink_handlelogslogerr("%s: netlink_get", __func__)for every legitimate event, plus incurs an extra syscall guaranteed to fail.Proposed fix
recv_again: len = recvmsg(fd, &msg, flags); - if (len == 0 || len == -1) + if (len == 0) return (int)len; + if (len == -1) { + if (errno == EAGAIN || errno == EWOULDBLOCK) + return 0; + return -1; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/netlink.c` around lines 70 - 120, netlink_get currently treats any lack of NLMSG_DONE/NLMSG_ERROR as "more to read" and loops back, causing a subsequent recvmsg with MSG_DONTWAIT to return -1/EAGAIN which is propagated as error; change behavior so a non-multipart message that leaves terminated == false does not force another recv: in netlink_get (label recv_again) after the message-processing loop, only loop again when again (NLM_F_MULTI) is set (keep the NLMSG_ERROR/NLMSG_DONE handling), otherwise return 0 to indicate success; additionally, when recvmsg returns -1, check errno and return 0 instead of -1 for EAGAIN (but preserve other errno cases), so callers like netlink_handle won't log spurious EAGAIN errors.
🧹 Nitpick comments (1)
src/netlink.c (1)
150-158: Deadelsebranch in state logging.
netlink_link()is only reached forRTM_DELLINK/RTM_NEWLINK(seenetlink_dispatch), and line 140 already returns for anyRTM_NEWLINKwithIFF_UPset. Therefore theelseat line 154-155 ("interface going away for no reason") is unreachable — it would requireRTM_NEWLINKwithIFF_UPset, which was filtered out above. Consider collapsing to a straightforwardif/elseto make the intent clearer and avoid the misleading log message.Proposed fix
- if (nlm->nlmsg_type == RTM_DELLINK) - loginfox("%s: interface has departed", ifp->if_name); - else if (!(ifi->ifi_flags & IFF_UP)) - loginfox("%s: interface is down", ifp->if_name); - else - logerrx("%s: interface going away for no reason", ifp->if_name); + loginfox("%s: interface %s", ifp->if_name, + nlm->nlmsg_type == RTM_DELLINK ? "has departed" : "is down");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/netlink.c` around lines 150 - 158, In netlink_link(), remove the unreachable third branch and collapse the logging to a two-way decision: in the netlink_link function, check nlm->nlmsg_type == RTM_DELLINK and call loginfox("%s: interface has departed", ifp->if_name); otherwise (handle the non-RTM_DELLINK case) call loginfox("%s: interface is down", ifp->if_name) based on the existing IFF_UP logic — delete the dead logerrx(...) branch. Keep the surrounding TAILQ_REMOVE(ctx->ctx_ifaces, ifp, if_next); if_free(ifp); and return 0; unchanged; references: netlink_link, netlink_dispatch, RTM_DELLINK, RTM_NEWLINK, IFF_UP, loginfox, logerrx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/netlink.c`:
- Around line 217-223: link_open currently ignores setsockopt failures for
NETLINK_BROADCAST_ERROR by only logging a generic message; instead, treat
ENOPROTOOPT and EOPNOTSUPP as ok (older kernels) but for any other errno log the
errno (use strerror(errno) or %d) and fail link_open (return an error/cleanup)
so initialization does not silently proceed; update the setsockopt handling
around NETLINK_BROADCAST_ERROR (referencing lctx->link_fd, setsockopt,
NETLINK_BROADCAST_ERROR, and link_open) to check errno and act accordingly.
- Around line 70-120: netlink_get currently treats any lack of
NLMSG_DONE/NLMSG_ERROR as "more to read" and loops back, causing a subsequent
recvmsg with MSG_DONTWAIT to return -1/EAGAIN which is propagated as error;
change behavior so a non-multipart message that leaves terminated == false does
not force another recv: in netlink_get (label recv_again) after the
message-processing loop, only loop again when again (NLM_F_MULTI) is set (keep
the NLMSG_ERROR/NLMSG_DONE handling), otherwise return 0 to indicate success;
additionally, when recvmsg returns -1, check errno and return 0 instead of -1
for EAGAIN (but preserve other errno cases), so callers like netlink_handle
won't log spurious EAGAIN errors.
---
Nitpick comments:
In `@src/netlink.c`:
- Around line 150-158: In netlink_link(), remove the unreachable third branch
and collapse the logging to a two-way decision: in the netlink_link function,
check nlm->nlmsg_type == RTM_DELLINK and call loginfox("%s: interface has
departed", ifp->if_name); otherwise (handle the non-RTM_DELLINK case) call
loginfox("%s: interface is down", ifp->if_name) based on the existing IFF_UP
logic — delete the dead logerrx(...) branch. Keep the surrounding
TAILQ_REMOVE(ctx->ctx_ifaces, ifp, if_next); if_free(ifp); and return 0;
unchanged; references: netlink_link, netlink_dispatch, RTM_DELLINK, RTM_NEWLINK,
IFF_UP, loginfox, logerrx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 593b9f7d-e7dd-4c07-a6e6-1e27c39dd556
📒 Files selected for processing (2)
src/netlink.csrc/unpriv.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/unpriv.c
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/unpriv.c (1)
586-593:⚠️ Potential issue | 🟠 Major
servnameis assigned fromu_gai_hostname(typo).When a non-empty
u_gai_servnameis supplied, the helper passes the hostname string togetaddrinfo()as the service name. This looks like a pre-existing bug that is now encapsulated in the extracted static dispatcher — worth fixing while the area is being refactored.🐛 Proposed fix
if (u_gai->u_gai_servname[0] != '\0') - servname = u_gai->u_gai_hostname; + servname = u_gai->u_gai_servname; else servname = NULL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/unpriv.c` around lines 586 - 593, The code mistakenly assigns servname from u_gai->u_gai_hostname; update the servname assignment so that when u_gai->u_gai_servname[0] != '\0' you set servname = u_gai->u_gai_servname (otherwise servname = NULL), leaving the hostname logic unchanged (hostname = u_gai->u_gai_hostname when non-empty); also scan the surrounding code in src/unpriv.c (the getaddrinfo helper) for any other hostname/servname mix-ups to ensure getaddrinfo() receives the correct service string.
♻️ Duplicate comments (2)
src/netlink.c (2)
217-223:⚠️ Potential issue | 🟡 Minor
NETLINK_BROADCAST_ERRORsetsockopt failure is silently swallowed.Re-iterating the earlier observation: this option is what lets the daemon notice dropped broadcasts (e.g.,
ENOBUFS) and re-sync interface state. If the intent is to tolerate older kernels that lack the option, handle that case explicitly (e.g.,errno == ENOPROTOOPT) and treat other failures as setup errors; otherwise stale interface state can accumulate silently.Suggested shape
`#ifdef` NETLINK_BROADCAST_ERROR if (setsockopt(lctx->link_fd, SOL_NETLINK, NETLINK_BROADCAST_ERROR, &on, - sizeof(on)) == -1) - logerr("%s: NETLINK_BROADCAST_ERROR", __func__); + sizeof(on)) == -1 && errno != ENOPROTOOPT) { + logerr("%s: NETLINK_BROADCAST_ERROR", __func__); + return -1; + } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/netlink.c` around lines 217 - 223, The NETLINK_BROADCAST_ERROR setsockopt failure is currently only logged and can be silently ignored; change the setsockopt error handling around the call on lctx->link_fd (the NETLINK_BROADCAST_ERROR branch) to inspect errno: if errno == ENOPROTOOPT treat it as an acceptable older-kernel case and continue, otherwise treat it as a real setup failure—call logerr with a clear message including __func__ and errno (or strerror(errno)) and propagate/fail initialization (e.g., return error or exit) instead of swallowing the error. Ensure you update the error path where setsockopt(...) == -1 to implement this conditional handling.
70-123:⚠️ Potential issue | 🟠 MajorBroadcast link events still trigger spurious
EAGAINerror logs.Re-checking the current implementation, the issue flagged previously still manifests:
- For single (non-multipart) broadcast events like
RTM_NEWLINK/RTM_DELLINK, after parsing the bufferagain = (nlm->nlmsg_flags & NLM_F_MULTI)is0andterminatedremainsfalse(neitherNLMSG_DONEnorNLMSG_ERRORis emitted for broadcasts).- The condition at line 119
if (again || !terminated)is thereforetrue, so we always re-enterrecv_again.- The follow-up
recvmsg(fd, &msg, MSG_DONTWAIT)finds no pending data and returns-1witherrno=EAGAIN, which is returned verbatim bynetlink_getand causesnetlink_handleto log"netlink_get: Resource temporarily unavailable"for every legitimate link event.Additionally, line 89 uses assignment (
again = ...) rather than OR (again |= ...), so within a single buffer a later non-multipart header can clear the multipart state set by an earlier one.Suggested fix
recv_again: len = recvmsg(fd, &msg, flags); - if (len == 0 || len == -1) - return (int)len; + if (len == 0) + return 0; + if (len == -1) { + if (errno == EAGAIN || errno == EWOULDBLOCK) + return 0; + return -1; + } @@ - again = (nlm->nlmsg_flags & NLM_F_MULTI); + again |= (nlm->nlmsg_flags & NLM_F_MULTI);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/netlink.c` around lines 70 - 123, The loop incorrectly resets multipart state and treats single non-multipart broadcast messages as unterminated, causing a spurious retry and an EAGAIN propagated to netlink_get/netlink_handle; fix by changing the multipart assignment to accumulate flags (use again |= (nlm->nlmsg_flags & NLM_F_MULTI) instead of again = ...) and ensure that if no multipart flag is set for any message in the received buffer you mark the batch as terminated (set terminated = true when again == 0 after processing the NLMSG loop) so the code does not goto recv_again for single-part broadcasts.
🧹 Nitpick comments (4)
configure (1)
313-326: Sanitizer probe tests onlyaddressbut enablesaddress,undefined.The feature test on line 320 compiles with
-fsanitize=address, but on success the finalCFLAGS/LDFLAGSget-fsanitize=address,undefined. A compiler/toolchain that supports ASan but not UBSan will pass the probe and then fail the actual build. Either test both sanitizers together, or separate them so UBSan is only enabled when its own probe succeeds.Suggested diff
- if $CC -fsanitize=address _test.c -o _test 2>&3; then + if $CC -fsanitize=address,undefined _test.c -o _test 2>&3; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configure` around lines 313 - 326, The probe currently controlled by SANITIZEADDRESS compiles with "-fsanitize=address" but then the config writes "-fsanitize=address,undefined" into CONFIG_MK; update the probe so the toolchain is actually tested for what will be enabled: either (A) change the compile test that uses CC and _test.c to try "-fsanitize=address,undefined" so both ASan+UBSan are verified before writing CPPFLAGS/CFLAGS/LDFLAGS, or (B) split the logic into two probes (e.g., SANITIZEADDRESS and a new SANITIZEUNDEFINED) that compile separately and only append "-fsanitize=undefined" into CFLAGS/LDFLAGS when the UBSan probe succeeds; make sure to reference and update the same CONFIG_MK write paths that currently emit CPPFLAGS/CFLAGS/LDFLAGS so the flags recorded match the successful probe results.src/unpriv.c (3)
101-103: Nit: passNULLinstead of0for the plugin pointer.The other
srv_runcall sites in this file (lines 185, 223, 261) passNULL;0here is inconsistent and relies on implicit conversion.- err = srv_run(ctx, 0, U_GETIFADDRS, match_if_index, + err = srv_run(ctx, NULL, U_GETIFADDRS, match_if_index, match_if_index != NULL ? sizeof(*match_if_index) : 0, &result, &rdata, &rdata_len);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/unpriv.c` around lines 101 - 103, The srv_run call in unpriv.c uses an integer literal 0 for the plugin pointer which is inconsistent with other call sites; change the third argument from 0 to NULL in the err = srv_run(ctx, 0, U_GETIFADDRS, match_if_index, ...) invocation so it matches the other calls (see srv_run, U_GETIFADDRS, match_if_index, result, rdata, rdata_len) and avoids relying on implicit conversion.
351-365: Size-calculation pass doesn't apply theif_sockaddr_activefilter.The first pass (lines 351-384) accounts for every entry returned by
getifaddrs, while the second pass (line 412-414) skips entries failingif_sockaddr_active. Net effect:callocmay over-allocate. Not a correctness bug —lenis recomputed frombpat line 453 beforesrv_send— but worth aligning the two passes to avoid wasted allocation and keep future maintainers from assuming both loops are in lock-step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/unpriv.c` around lines 351 - 365, In the first pass over the ifaddrs list (the loop iterating ifa from ifaddrs that uses match_index and calls if_link_match_index), add the same if_sockaddr_active(ifa->ifa_addr) check used in the second pass so entries with inactive sockaddr are skipped when computing len/allocation; specifically, after the match_index switch (or before counting/size accumulation) call if_sockaddr_active(ifa->ifa_addr) and continue if it returns false so the size-calculation loop mirrors the second pass (bp/len logic) and avoids over-allocation.
521-528: Fullstruct interface(including in-process pointers) sent across IPC.
buf = ifp; len = sizeof(*ifp);ships the entire record — includingTAILQ_ENTRY,if_ctx,if_pools,if_bpf, andif_output— to the main process. The current client (unpriv_learnif, lines 195-211) only reads name/hwtype/hwlen/hwaddr/mtu, so it works today, but it is fragile: any future client reader of the other fields would dereference pointers from the helper's address space, and uninitialised struct padding is exposed on the wire. Consider serialising only the intended fields (e.g., into a small dedicated reply struct) — same pattern asstruct unpriv_addrinfoforgetaddrinfo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/unpriv.c` around lines 521 - 528, The code currently sends the entire struct interface (buf = ifp; len = sizeof(*ifp)) across IPC which leaks in-process pointers and padding; instead define a small dedicated reply struct (like struct unpriv_addrinfo used for getaddrinfo) containing only the fields the client reads (name, hwtype, hwlen, hwaddr, mtu), populate that struct from ifp in unpriv_learnif path, and change buf and len to point to and size of that reply struct; ensure no TAILQ_ENTRY or pointer fields (if_ctx, if_pools, if_bpf, if_output) are copied or transmitted and update any receiver code expecting the compact reply accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configure`:
- Around line 614-632: The configure netlink feature probe is including the
kernel-internal header <asm/types.h>, which causes false negatives; update the
test stub (_netlink.c used when LINK_SRC is unset and compiled with XCC) to
remove the line that includes <asm/types.h> so it only includes the same headers
used by the real source (e.g., <linux/netlink.h>, <linux/rtnetlink.h>,
<sys/socket.h>), ensuring the probe matches src/netlink.c and will succeed on
systems where <asm/types.h> is not available.
In `@src/netlink.c`:
- Around line 140-155: The final else branch with logerrx is unreachable because
the early return at the top (if nlm->nlmsg_type != RTM_DELLINK && ifi->ifi_flags
& IFF_UP) guarantees only two states reach the later switch: RTM_DELLINK or
RTM_NEWLINK with IFF_UP cleared; remove the dead else (logerrx) or, if you
intended defensive handling, move the IFF_UP check out of the early-return and
instead branch on nlm->nlmsg_type inside the block (use nlm->nlmsg_type,
RTM_DELLINK, RTM_NEWLINK and ifi->ifi_flags / IFF_UP to explicitly handle the
three cases) so the code in netlink.c that uses loginfox/logerrx and the iface
lookup (ctx->ctx_ifaces / ifp) only executes reachable branches.
In `@src/unpriv.c`:
- Around line 101-111: The client currently ignores the server-side return code
and treats an empty reply as success; update the post-srv_run handling in the
block using srv_run, result, rdata_len, rdata and ifahead so that you check both
err and result (i.e., if err == -1 || result == -1) and return -1 on failure
(and ensure any allocated rdata is handled appropriately), mirroring the
behavior in unpriv_learnif and acknowledging failures signaled by
unpriv_dispatch_getifaddrs.
---
Outside diff comments:
In `@src/unpriv.c`:
- Around line 586-593: The code mistakenly assigns servname from
u_gai->u_gai_hostname; update the servname assignment so that when
u_gai->u_gai_servname[0] != '\0' you set servname = u_gai->u_gai_servname
(otherwise servname = NULL), leaving the hostname logic unchanged (hostname =
u_gai->u_gai_hostname when non-empty); also scan the surrounding code in
src/unpriv.c (the getaddrinfo helper) for any other hostname/servname mix-ups to
ensure getaddrinfo() receives the correct service string.
---
Duplicate comments:
In `@src/netlink.c`:
- Around line 217-223: The NETLINK_BROADCAST_ERROR setsockopt failure is
currently only logged and can be silently ignored; change the setsockopt error
handling around the call on lctx->link_fd (the NETLINK_BROADCAST_ERROR branch)
to inspect errno: if errno == ENOPROTOOPT treat it as an acceptable older-kernel
case and continue, otherwise treat it as a real setup failure—call logerr with a
clear message including __func__ and errno (or strerror(errno)) and
propagate/fail initialization (e.g., return error or exit) instead of swallowing
the error. Ensure you update the error path where setsockopt(...) == -1 to
implement this conditional handling.
- Around line 70-123: The loop incorrectly resets multipart state and treats
single non-multipart broadcast messages as unterminated, causing a spurious
retry and an EAGAIN propagated to netlink_get/netlink_handle; fix by changing
the multipart assignment to accumulate flags (use again |= (nlm->nlmsg_flags &
NLM_F_MULTI) instead of again = ...) and ensure that if no multipart flag is set
for any message in the received buffer you mark the batch as terminated (set
terminated = true when again == 0 after processing the NLMSG loop) so the code
does not goto recv_again for single-part broadcasts.
---
Nitpick comments:
In `@configure`:
- Around line 313-326: The probe currently controlled by SANITIZEADDRESS
compiles with "-fsanitize=address" but then the config writes
"-fsanitize=address,undefined" into CONFIG_MK; update the probe so the toolchain
is actually tested for what will be enabled: either (A) change the compile test
that uses CC and _test.c to try "-fsanitize=address,undefined" so both
ASan+UBSan are verified before writing CPPFLAGS/CFLAGS/LDFLAGS, or (B) split the
logic into two probes (e.g., SANITIZEADDRESS and a new SANITIZEUNDEFINED) that
compile separately and only append "-fsanitize=undefined" into CFLAGS/LDFLAGS
when the UBSan probe succeeds; make sure to reference and update the same
CONFIG_MK write paths that currently emit CPPFLAGS/CFLAGS/LDFLAGS so the flags
recorded match the successful probe results.
In `@src/unpriv.c`:
- Around line 101-103: The srv_run call in unpriv.c uses an integer literal 0
for the plugin pointer which is inconsistent with other call sites; change the
third argument from 0 to NULL in the err = srv_run(ctx, 0, U_GETIFADDRS,
match_if_index, ...) invocation so it matches the other calls (see srv_run,
U_GETIFADDRS, match_if_index, result, rdata, rdata_len) and avoids relying on
implicit conversion.
- Around line 351-365: In the first pass over the ifaddrs list (the loop
iterating ifa from ifaddrs that uses match_index and calls if_link_match_index),
add the same if_sockaddr_active(ifa->ifa_addr) check used in the second pass so
entries with inactive sockaddr are skipped when computing len/allocation;
specifically, after the match_index switch (or before counting/size
accumulation) call if_sockaddr_active(ifa->ifa_addr) and continue if it returns
false so the size-calculation loop mirrors the second pass (bp/len logic) and
avoids over-allocation.
- Around line 521-528: The code currently sends the entire struct interface (buf
= ifp; len = sizeof(*ifp)) across IPC which leaks in-process pointers and
padding; instead define a small dedicated reply struct (like struct
unpriv_addrinfo used for getaddrinfo) containing only the fields the client
reads (name, hwtype, hwlen, hwaddr, mtu), populate that struct from ifp in
unpriv_learnif path, and change buf and len to point to and size of that reply
struct; ensure no TAILQ_ENTRY or pointer fields (if_ctx, if_pools, if_bpf,
if_output) are copied or transmitted and update any receiver code expecting the
compact reply accordingly.
🪄 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: 50fba8d2-ee58-4d7f-a9ba-e4a8cc0a35d0
📒 Files selected for processing (5)
configuresrc/dhcpsd.csrc/if.csrc/netlink.csrc/unpriv.c
🚧 Files skipped from review as they are similar to previous changes (2)
- src/dhcpsd.c
- src/if.c
| if [ -z "$LINK_SRC" ]; then | ||
| printf "Testing for netlink ... " | ||
| cat << EOF >_netlink.c | ||
| #include <asm/types.h> | ||
| #include <linux/netlink.h> | ||
| #include <linux/rtnetlink.h> | ||
| #include <sys/socket.h> | ||
| int main(void) { | ||
| return 0; | ||
| } | ||
| EOF | ||
| if $XCC _netlink.c -o _netlink 2>&3; then | ||
| LINK_SRC="netlink.c" | ||
| echo "yes" | ||
| else | ||
| echo "no" | ||
| fi | ||
| rm -rf _netlink.* _netlink | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine src/netlink.c
find . -name "netlink.c" -type fRepository: NetworkConfiguration/dhcpsd
Length of output: 86
🏁 Script executed:
# Also check the configure script at the specified lines
sed -n '614,632p' configureRepository: NetworkConfiguration/dhcpsd
Length of output: 426
🏁 Script executed:
# Let's search for asm/types.h usage in the codebase
rg "asm/types\.h" --type-list | head -20Repository: NetworkConfiguration/dhcpsd
Length of output: 460
🏁 Script executed:
# Check for asm/types.h in the entire repository
rg "asm/types" -A 2 -B 2Repository: NetworkConfiguration/dhcpsd
Length of output: 258
🏁 Script executed:
# Examine src/netlink.c to see its current includes
head -30 ./src/netlink.cRepository: NetworkConfiguration/dhcpsd
Length of output: 1539
🏁 Script executed:
# Also check if asm/types.h appears anywhere in src/netlink.c
rg "asm/types" ./src/netlink.cRepository: NetworkConfiguration/dhcpsd
Length of output: 53
🏁 Script executed:
# Get the first 50 lines to see all includes in src/netlink.c
sed -n '1,50p' ./src/netlink.cRepository: NetworkConfiguration/dhcpsd
Length of output: 1887
Feature probe includes kernel-internal <asm/types.h> — inconsistent with src/netlink.c.
The source file src/netlink.c does not include <asm/types.h>, relying on <linux/types.h> via <linux/netlink.h> and <linux/rtnetlink.h>. The feature probe in configure (lines 617) still includes <asm/types.h>. On systems where <asm/types.h> is not exposed to userspace (musl, sanitized header setups), the probe will incorrectly fail even though the actual netlink.c would compile fine.
Remove <asm/types.h> from the probe to match the source file.
Suggested diff
cat << EOF >_netlink.c
-#include <asm/types.h>
`#include` <linux/netlink.h>
`#include` <linux/rtnetlink.h>
`#include` <sys/socket.h>
int main(void) {
return 0;
}
EOF📝 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.
| if [ -z "$LINK_SRC" ]; then | |
| printf "Testing for netlink ... " | |
| cat << EOF >_netlink.c | |
| #include <asm/types.h> | |
| #include <linux/netlink.h> | |
| #include <linux/rtnetlink.h> | |
| #include <sys/socket.h> | |
| int main(void) { | |
| return 0; | |
| } | |
| EOF | |
| if $XCC _netlink.c -o _netlink 2>&3; then | |
| LINK_SRC="netlink.c" | |
| echo "yes" | |
| else | |
| echo "no" | |
| fi | |
| rm -rf _netlink.* _netlink | |
| fi | |
| if [ -z "$LINK_SRC" ]; then | |
| printf "Testing for netlink ... " | |
| cat << EOF >_netlink.c | |
| `#include` <linux/netlink.h> | |
| `#include` <linux/rtnetlink.h> | |
| `#include` <sys/socket.h> | |
| int main(void) { | |
| return 0; | |
| } | |
| EOF | |
| if $XCC _netlink.c -o _netlink 2>&3; then | |
| LINK_SRC="netlink.c" | |
| echo "yes" | |
| else | |
| echo "no" | |
| fi | |
| rm -rf _netlink.* _netlink | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@configure` around lines 614 - 632, The configure netlink feature probe is
including the kernel-internal header <asm/types.h>, which causes false
negatives; update the test stub (_netlink.c used when LINK_SRC is unset and
compiled with XCC) to remove the line that includes <asm/types.h> so it only
includes the same headers used by the real source (e.g., <linux/netlink.h>,
<linux/rtnetlink.h>, <sys/socket.h>), ensuring the probe matches src/netlink.c
and will succeed on systems where <asm/types.h> is not available.
| if (nlm->nlmsg_type != RTM_DELLINK && ifi->ifi_flags & IFF_UP) | ||
| return 0; | ||
|
|
||
| TAILQ_FOREACH(ifp, ctx->ctx_ifaces, if_next) { | ||
| if (ifp->if_index == (unsigned int)ifi->ifi_index) | ||
| break; | ||
| } | ||
| if (ifp == NULL) | ||
| return 0; | ||
|
|
||
| if (nlm->nlmsg_type == RTM_DELLINK) | ||
| loginfox("%s: interface has departed", ifp->if_name); | ||
| else if (!(ifi->ifi_flags & IFF_UP)) | ||
| loginfox("%s: interface is down", ifp->if_name); | ||
| else | ||
| logerrx("%s: interface going away for no reason", ifp->if_name); |
There was a problem hiding this comment.
Unreachable else branch — the "going away for no reason" logerrx is dead code.
Line 140 already short-circuits with return 0 whenever nlm->nlmsg_type != RTM_DELLINK && (ifi->ifi_flags & IFF_UP). By the time execution reaches line 150, we are guaranteed to be in one of two states:
nlmsg_type == RTM_DELLINK→ first branch.nlmsg_type == RTM_NEWLINKwithIFF_UPcleared → second branch.
The else at line 154 (RTM_NEWLINK with IFF_UP set) is unreachable, so the logerrx never fires and the message is misleading to anyone reading the code. Drop the dead branch (or, if it is defensive, restructure so the IFF_UP filter is only applied inside this block rather than as an early-return guard).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/netlink.c` around lines 140 - 155, The final else branch with logerrx is
unreachable because the early return at the top (if nlm->nlmsg_type !=
RTM_DELLINK && ifi->ifi_flags & IFF_UP) guarantees only two states reach the
later switch: RTM_DELLINK or RTM_NEWLINK with IFF_UP cleared; remove the dead
else (logerrx) or, if you intended defensive handling, move the IFF_UP check out
of the early-return and instead branch on nlm->nlmsg_type inside the block (use
nlm->nlmsg_type, RTM_DELLINK, RTM_NEWLINK and ifi->ifi_flags / IFF_UP to
explicitly handle the three cases) so the code in netlink.c that uses
loginfox/logerrx and the iface lookup (ctx->ctx_ifaces / ifp) only executes
reachable branches.
| err = srv_run(ctx, 0, U_GETIFADDRS, match_if_index, | ||
| match_if_index != NULL ? sizeof(*match_if_index) : 0, &result, | ||
| &rdata, &rdata_len); | ||
| if (err == -1) | ||
| return -1; | ||
|
|
||
| /* Should be impossible - lo0 will always exist. */ | ||
| if (rdata_len == 0) { | ||
| *ifahead = NULL; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Server-side errors are silently swallowed.
srv_run captures the server's result code into result, but the client never inspects it. On the server, unpriv_dispatch_getifaddrs reports failures (e.g., getifaddrs() returning -1 at line 339) by replying with err = -1, buf = NULL, len = 0. Here at line 108 the client sees rdata_len == 0, sets *ifahead = NULL, and returns 0 — reporting success for a failed call. Compare with unpriv_learnif (line 187) which correctly checks both err == -1 || result == -1.
🐛 Proposed fix
err = srv_run(ctx, 0, U_GETIFADDRS, match_if_index,
match_if_index != NULL ? sizeof(*match_if_index) : 0, &result,
&rdata, &rdata_len);
- if (err == -1)
+ if (err == -1 || result == -1)
return -1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/unpriv.c` around lines 101 - 111, The client currently ignores the
server-side return code and treats an empty reply as success; update the
post-srv_run handling in the block using srv_run, result, rdata_len, rdata and
ifahead so that you check both err and result (i.e., if err == -1 || result ==
-1) and return -1 on failure (and ensure any allocated rdata is handled
appropriately), mirroring the behavior in unpriv_learnif and acknowledging
failures signaled by unpriv_dispatch_getifaddrs.
1 participant
The interface you may want to configure might not
exist when dhcpsd first starts.
The -w option will cause dhcpcd to only configure
an interface when it recieves a message on it.
We assume if we have received a message it's
ready as we cannot guess which address to use.
So ensure the server address is added before the
interface is marked as up!
If the interface is marked as down then the
interface is deconfigured.
This allows dhcpsd to be used in-place of the
macOS DHCP server as provided by the UTM
framework.