manager: Fix loosing iface options on CARRIER #548
Conversation
WalkthroughAdds per-interface Changes
Sequence Diagram(s)sequenceDiagram
participant Control as Control process
participant Reconf as reconf_reboot
participant Iface as Interface (ifp)
participant Init as preinit/start
Note over Control,Reconf: Control command invokes reconf with argc/argv
Control->>Reconf: reconf_reboot(reboot_else_applyaddr, argc, argv, first_iface_arg)
alt ifp already has per-interface argv
Reconf->>Iface: use existing ifp->argc/ifp->argv
Reconf->>Init: call preinit/start using ifp->argv
else interface referenced but not active
Reconf->>Iface: copy_argv(argc, argv)
Iface-->>Reconf: store copied argv in ifp->argv/ifp->argc
Reconf->>Init: activate interface -> preinit/start using ifp->argv
else interface active but action is apply-address
Reconf->>Init: apply IPv4 address (no reboot) using context/main argv as appropriate
end
Init->>Iface: bring up/apply addresses
Note right of Iface: on teardown -> free_argv_copy(ifp->argv)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/dhcpcd.c(5 hunks)src/dhcpcd.h(1 hunks)src/if-options.c(2 hunks)src/if-options.h(1 hunks)src/if.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T19:24:39.833Z
Learnt from: rsmarples
Repo: NetworkConfiguration/dhcpcd PR: 536
File: src/eloop.c:53-59
Timestamp: 2025-11-03T19:24:39.833Z
Learning: In the dhcpcd codebase (NetworkConfiguration/dhcpcd), compile-time feature detection based on header versions (e.g., LINUX_VERSION_CODE for epoll_pwait2) does not require runtime fallback handling for syscall unavailability (ENOSYS). Users are responsible for ensuring headers match their target runtime kernel.
Applied to files:
src/dhcpcd.hsrc/dhcpcd.c
🧬 Code graph analysis (4)
src/if-options.h (1)
src/if-options.c (2)
copy_argv(2993-3034)free_argv_copy(3036-3040)
src/if.c (1)
src/if-options.c (1)
free_argv_copy(3036-3040)
src/dhcpcd.c (2)
src/ipv4.c (1)
ipv4_applyaddr(746-833)src/if-options.c (2)
free_argv_copy(3036-3040)copy_argv(2993-3034)
src/if-options.c (1)
compat/strlcpy.c (1)
strlcpy(28-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: netbsd (--disable-ipv6)
- GitHub Check: netbsd (--disable-arp, -DSMALL)
- GitHub Check: netbsd (--disable-dhcp6, -DSMALL)
- GitHub Check: netbsd (--disable-ipv6, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4ll, -DSMALL)
- GitHub Check: netbsd (--disable-dhcp6)
- GitHub Check: netbsd (--disable-arp)
- GitHub Check: netbsd (--disable-ipv4)
- GitHub Check: netbsd (-DSMALL)
- GitHub Check: netbsd (--disable-ipv4ll)
- GitHub Check: netbsd
- GitHub Check: netbsd (--disable-ipv4, -DSMALL)
- GitHub Check: freebsd
- GitHub Check: openbsd
DanielG
force-pushed
the
fix-carrier-reset
branch
3 times, most recently
from
aba6a30 to
0ee7320
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/if-options.c (1)
2993-3036: Approve with optional style refinement.The argv copy implementation is correct with good defensive checks for truncation and buffer overruns. The NULL check at lines 3005-3006 protects the dereference at line 3008, making the code safe.
Optional style improvement: For consistency with defensive programming best practices, consider checking malloc's result before any assignments:
void *buf = malloc(ptrslen + strslen); +if (buf == NULL) + return NULL; char **ptrs = buf; -if (!buf) - return NULL;While the current code is safe (no dereference before NULL check), this ordering is more conventional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/dhcpcd.c(5 hunks)src/dhcpcd.h(1 hunks)src/if-options.c(2 hunks)src/if-options.h(1 hunks)src/if.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/if-options.h
- src/dhcpcd.h
- src/dhcpcd.c
🔇 Additional comments (4)
src/if.c (1)
103-104: LGTM! Per-interface argv cleanup properly integrated.The cleanup of per-interface argv is correctly placed in the interface teardown path with an appropriate NULL check, consistent with other resource cleanup in this function.
src/if-options.c (3)
47-47: LGTM! Include necessary for defensive assertions.The
assert.hinclude supports the sentinel validation and bounds checking in the new argv copy functions.
2990-2992: LGTM! Sentinel magic number properly documented.The sentinel value provides runtime validation that
free_argv_copy()receives a pointer fromcopy_argv(). The intentional truncation on 32-bit architectures is documented.
3038-3042: LGTM! Sentinel validation enforces API contract.The assertion at line 3040 validates that
argvoriginated fromcopy_argv()by checking the magic sentinel. This catches API misuse in debug builds. In release builds, the function assumes correct usage per the API contract.
DanielG
force-pushed
the
fix-carrier-reset
branch
from
0ee7320 to
5fc8cdb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/if-options.c (1)
2997-3003: Consider guarding against integer overflow in size calculations.The size calculations could theoretically overflow if presented with a large number of very long strings:
- Line 2998:
strslen += strlen(argv[i]) + 1- Line 3003:
malloc(ptrslen + strslen)While unlikely with typical command-line arguments, this could be exploitable with malicious inputs. Consider adding overflow checks or using safer arithmetic functions.
For example:
size_t strslen = 0; for (i = 0; i < argc; i++) { - strslen += strlen(argv[i]) + 1; + size_t len = strlen(argv[i]); + if (strslen > SIZE_MAX - len - 1) + return NULL; /* Overflow would occur */ + strslen += len + 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/dhcpcd.c(5 hunks)src/dhcpcd.h(1 hunks)src/if-options.c(2 hunks)src/if-options.h(1 hunks)src/if.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/if-options.h
- src/dhcpcd.c
- src/dhcpcd.h
- src/if.c
🧰 Additional context used
🧬 Code graph analysis (1)
src/if-options.c (1)
compat/strlcpy.c (1)
strlcpy(28-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: netbsd (--disable-dhcp6, -DSMALL)
- GitHub Check: netbsd (--disable-arp, -DSMALL)
- GitHub Check: netbsd (--disable-dhcp6)
- GitHub Check: netbsd (--disable-ipv6, -DSMALL)
- GitHub Check: netbsd (--disable-ipv6)
- GitHub Check: netbsd (--disable-arp)
- GitHub Check: netbsd (--disable-ipv4ll, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4)
- GitHub Check: netbsd (--disable-ipv4ll)
- GitHub Check: netbsd
- GitHub Check: netbsd (-DSMALL)
- GitHub Check: openbsd
- GitHub Check: freebsd
🔇 Additional comments (1)
src/if-options.c (1)
47-47: LGTM!The
assert.hinclude is required for the assertions used in the newcopy_argvandfree_argv_copyfunctions.
DanielG
force-pushed
the
fix-carrier-reset
branch
from
5fc8cdb to
112cc12
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/if-options.h (1)
325-327: LGTM! Clean API for per-interface argv management.The function declarations are correct and match the implementations in
src/if-options.c. The copy/free pair with magic value validation is a sound design pattern for this use case.Optionally, consider adding brief documentation comments to clarify:
copy_argvreturns NULL on allocation failure (callers should check)free_argv_copymust be used on pointers returned bycopy_argv(not regularfree)This would help future maintainers understand the ownership semantics, though it's consistent with the existing style in this header to omit such comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/dhcpcd.c(5 hunks)src/dhcpcd.h(1 hunks)src/if-options.c(2 hunks)src/if-options.h(1 hunks)src/if.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/dhcpcd.h
- src/if.c
- src/dhcpcd.c
- src/if-options.c
🧰 Additional context used
🧬 Code graph analysis (1)
src/if-options.h (1)
src/if-options.c (2)
copy_argv(2993-3038)free_argv_copy(3040-3047)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: netbsd (--disable-dhcp6, -DSMALL)
- GitHub Check: netbsd (--disable-dhcp6)
- GitHub Check: netbsd (--disable-arp, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4ll, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4)
- GitHub Check: netbsd (--disable-ipv6)
- GitHub Check: netbsd (--disable-ipv4, -DSMALL)
- GitHub Check: netbsd (--disable-ipv6, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4ll)
- GitHub Check: netbsd (-DSMALL)
- GitHub Check: netbsd
- GitHub Check: netbsd (--disable-arp)
- GitHub Check: freebsd
- GitHub Check: openbsd
|
@DanielG Just to double-check, is this what you attached to this Debian bug? |
Yup. |
| } | ||
|
|
||
| #define ARGV_COPY_MAGIC ((char *)0x5a54292d273f3d34) | ||
| /*^ intentional truncation on 32bit arches */ |
There was a problem hiding this comment.
This comment is scary.
Why do we need a sentinel here? The scope of the variable isn't going outside any process so we can just malloc and free which makes the below code easier.
There was a problem hiding this comment.
The sentinel is for memory safety. Think: double free or free or corrupted/wrong pointer being passed in. Always good manners for allocators IMO. Not sure what you mean by "going outside any process".
The sentinel could just as well be 32bit if you like, I just like to use the extra entropy modern machines can offer. The explicit cast should supress truncation warnings but I'm not 100% on that. Was hoping CI would show whether it raises a warning on on 32bit, but it seems there is no coverage.
| strslen -= len + 1; | ||
| } | ||
|
|
||
| assert(strslen == 0); |
There was a problem hiding this comment.
I find it interesting that you assert this error but allow impossible errors to pass with a failure in the loop.
Please pick one and be consistent.
There was a problem hiding this comment.
What "impossible errors" are you talking about?
When an interface (re-)gains carrier dhcpcd_handlecarrier() runs
dhcpcd_initstate() to kick off profile re-selection. Previously this used
args originally passed when starting the manager (ctx->argv).
However interfaces started via the manager control
interface (dhcpcd_initstate1() in dhcpcd_handleargs()) may be started with
different args.
For example if we start a manager with
dhcpcd -M --inactive
and then start only IPv4 on an interface with
dhcpcd -4 iface0
a subsequent CARRIER event will reset the interface to what amounts to
"default config + `-M --inactive`" which in this case will enable ipv6
also!
To fix this we keep a copy of the arguments used to start an interface in
the manager (dhcpcd_handleargs()) code path around around (ifp->argv).
In the current implementation args passed for renew following the initial
interface start will not be persisted. This causes the interface to reset
to a state of "defaults + config + profile + start-cmdline".
For example (continuing the scenario above) after enabling ipv6 with -n:
$ dhcpcd -6 -n iface0
A subsequent CARRIER event will disable ipv6 again as the effective
arguments remain `-4 iface0` as passed during interface start.
Note the per-interface daemon code path wasn't affected as ctx->args
already contains the interface start args.
DanielG
force-pushed
the
fix-carrier-reset
branch
from
112cc12 to
2661f5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/dhcpcd.c (1)
1425-1425: Address previous review feedback: remove or clarify misleading comment.This comment was flagged in a previous review by rsmarples as misleading. The interface is not "new"—it already exists in ctx->ifaces and is already allocated. It's simply not active as IF_ACTIVE_USER. Per the previous review request, please remove this comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/dhcpcd.c(5 hunks)src/dhcpcd.h(1 hunks)src/if-options.c(2 hunks)src/if-options.h(1 hunks)src/if.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/if.c
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T19:24:39.851Z
Learnt from: rsmarples
Repo: NetworkConfiguration/dhcpcd PR: 536
File: src/eloop.c:53-59
Timestamp: 2025-11-03T19:24:39.851Z
Learning: In the dhcpcd codebase (NetworkConfiguration/dhcpcd), compile-time feature detection based on header versions (e.g., LINUX_VERSION_CODE for epoll_pwait2) does not require runtime fallback handling for syscall unavailability (ENOSYS). Users are responsible for ensuring headers match their target runtime kernel.
Applied to files:
src/dhcpcd.c
🧬 Code graph analysis (2)
src/if-options.h (1)
src/if-options.c (2)
copy_argv(2993-3041)free_argv_copy(3043-3050)
src/dhcpcd.c (1)
src/if-options.c (2)
free_argv_copy(3043-3050)copy_argv(2993-3041)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: netbsd (--disable-dhcp6, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4)
- GitHub Check: netbsd (--disable-arp, -DSMALL)
- GitHub Check: netbsd (--disable-ipv6)
- GitHub Check: netbsd (--disable-ipv6, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4ll, -DSMALL)
- GitHub Check: netbsd (--disable-ipv4ll)
- GitHub Check: netbsd (--disable-arp)
- GitHub Check: netbsd (--disable-dhcp6)
- GitHub Check: netbsd
- GitHub Check: freebsd
- GitHub Check: netbsd (-DSMALL)
- GitHub Check: openbsd
🔇 Additional comments (9)
src/dhcpcd.h (1)
88-90: LGTM! Per-interface argc/argv fields added correctly.The new fields enable per-interface argument storage, distinguishing control command options from manager-global options. Ensure these fields are zero-initialized when interfaces are allocated (typically via calloc in if.c).
src/if-options.h (1)
325-327: LGTM! Function declarations are clear.The new argv copy/free API is properly declared. The implementations in if-options.c include safety checks (ARGV_COPY_MAGIC sentinel, argc validation) that were addressed in previous review cycles.
src/if-options.c (4)
47-47: LGTM! Assert header appropriately added.The assert.h include supports safety checks in copy_argv and free_argv_copy, following established project conventions.
2990-2991: LGTM! Sentinel provides memory safety.ARGV_COPY_MAGIC validates that argv was allocated by copy_argv before freeing, protecting against double-free and corruption. The truncation on 32-bit architectures is acceptable for sentinel purposes.
2993-3041: LGTM! Implementation is robust.The copy_argv implementation properly handles edge cases (argc ≤ 0, allocation failure, truncation) and follows memory safety best practices with a single-buffer allocation. Previous review concerns about null-checks and validation were addressed in commits 8ed586b and 112cc12.
3043-3050: LGTM! Safe deallocation with validation.The free_argv_copy implementation validates the magic marker both at compile-time (assert) and runtime (logerrx), providing defense-in-depth against invalid frees. The runtime check was added per previous review feedback in commit 112cc12.
src/dhcpcd.c (3)
1370-1370: LGTM! Comment accurately describes argv source.The comment correctly notes that argc/argv may come from either control commands or the main process arguments, depending on the caller.
1787-1787: LGTM! Comment accurately describes the context.The comment correctly emphasizes that this code path processes control command options from the dhcpcd control socket.
2694-2694: LGTM! Comment accurately describes argv source.The comment correctly notes that this initialization uses the main process command-line arguments (from program startup), distinguishing it from control command arguments.
DanielG
force-pushed
the
fix-carrier-reset
branch
from
e3935ab to
fde51c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/dhcpcd.c (1)
1415-1425: Consider using/* */comment style for consistency.The codebase predominantly uses
/* */style comments. The//style comments here are valid C99+ but create a minor style inconsistency.- // Find ifp in iface args - for (i = first_iface_arg; i < argc; i++) { + /* Find ifp in iface args */ + for (i = first_iface_arg; i < argc; i++) { if (strcmp(ifp->name, argv[i]) == 0) break; } - bool iface_found = i != argc; + bool iface_found = i != argc; - if (!args_empty && !iface_found) - continue; // try to find other ifaces - // Note: if args_empty affect all interfaces + if (!args_empty && !iface_found) + continue; /* try to find other ifaces */ + /* Note: if args_empty affect all interfaces */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/dhcpcd.c(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T19:24:39.851Z
Learnt from: rsmarples
Repo: NetworkConfiguration/dhcpcd PR: 536
File: src/eloop.c:53-59
Timestamp: 2025-11-03T19:24:39.851Z
Learning: In the dhcpcd codebase (NetworkConfiguration/dhcpcd), compile-time feature detection based on header versions (e.g., LINUX_VERSION_CODE for epoll_pwait2) does not require runtime fallback handling for syscall unavailability (ENOSYS). Users are responsible for ensuring headers match their target runtime kernel.
Applied to files:
src/dhcpcd.c
🧬 Code graph analysis (1)
src/dhcpcd.c (1)
src/if-options.c (2)
free_argv_copy(3043-3050)copy_argv(2993-3041)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: netbsd (--disable-dhcp6, -DSMALL)
- GitHub Check: netbsd (--disable-ipv6, -DSMALL)
- GitHub Check: netbsd (-DSMALL)
- GitHub Check: netbsd (--disable-ipv4ll, -DSMALL)
- GitHub Check: netbsd (--disable-ipv6)
- GitHub Check: netbsd (--disable-dhcp6)
- GitHub Check: netbsd (--disable-arp, -DSMALL)
- GitHub Check: netbsd (--disable-arp)
- GitHub Check: netbsd (--disable-ipv4ll)
- GitHub Check: netbsd (--disable-ipv4)
- GitHub Check: netbsd (--disable-ipv4, -DSMALL)
- GitHub Check: netbsd
- GitHub Check: freebsd
- GitHub Check: openbsd
🔇 Additional comments (5)
src/dhcpcd.c (5)
732-735: LGTM - per-interface argv propagation correctly implemented.Using
ifp->argvas the discriminator for both argc and argv ensures they stay consistent. This correctly addresses the PR objective of preserving per-interface options across CARRIER events.
1438-1441: LGTM - argc/argv synchronization issue addressed.The code now correctly synchronizes
ifp->argcwith the result ofcopy_argv: setting it toargcon success or0on failure. This resolves the previously flagged desynchronization concern.
1370-1370: Good clarifying comment.The comment correctly documents that
if_rebootis called with either control command argv or main context argv depending on the call path.
1551-1552: LGTM - signal handler correctly uses main context arguments.For SIGHUP-triggered rebinds, using
ctx->argcandctx->argvis appropriate as the signal handler should apply the original manager startup arguments. The calculationctx->argc - ctx->ifccorrectly computesfirst_iface_arg.
1795-1796: LGTM - control command handling correctly passes per-command arguments.The control command's
argc,argv, andoifind(first interface argument index) are correctly passed toreconf_reboot, ensuring per-interface options from control commands are preserved.
|
@rsmarples, is @DanielG 's response to your change requests adequate? |
3 participants
When an interface (re-)gains carrier dhcpcd_handlecarrier() runs dhcpcd_initstate() to kick off profile re-selection. Previously this used args originally passed when starting the manager (ctx->argv).
However interfaces started via the manager control interface (dhcpcd_initstate1() in dhcpcd_handleargs()) may be started with different args.
For example if we start a manager with
and then start only IPv4 on an interface with
a subsequent CARRIER event will reset the interface to what amounts to "default config +
-M --inactive" which in this case will enable ipv6 also!To fix this we keep a copy of the arguments used to start an interface in the manager (dhcpcd_handleargs()) code path around around (ifp->argv).
In the current implementation args passed during for renew following the initial interface start will not be persisted. This causes the interface to reset to a state of "defaults + config + profile + start-cmdline".
For example (continuing the scenario above) after enabling ipv6 with -n:
A subsequent CARRIER event will disable ipv6 again as the effective arguments remain
-4 iface0as passed during interface start.Note the per-interface daemon code path wasn't affected as ctx->args already contains the interface start args.