manager: Fix loosing iface options on CARRIER #548
Conversation
WalkthroughAdds per-interface argv/argc fields and lifecyle (copy/free); implements copy_argv/free_argv_copy utilities; propagates control-command argv into newly discovered interfaces during reconfiguration and prefers per-interface argv during init; frees per-interface argv on interface teardown. Changes
Sequence Diagram(s)sequenceDiagram
participant Control as Control process (control cmd argv)
participant Reconf as reconf_reboot
participant Iface as Interface (ifp)
participant Init as preinit/start
Note over Control,Reconf `#DDEEFF`: Control command arrives with argc/argv
Control->>Reconf: invoke reconf_reboot(argc, argv)
alt ifp already has per-interface argv
Reconf->>Iface: use ifp->argc / ifp->argv for init
else new interface discovered
Reconf->>Iface: copy_argv(argc, argv) => store in ifp->argv/ifp->argc
Note right of Iface `#F0F8FF`: per-interface argv populated
Reconf->>Init: start preinit/start using ifp->argv
end
Init->>Iface: apply addresses / bring up interface
Note right of Iface `#E8F5E9`: on teardown call 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.
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
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. |
|
|
||
| dhcpcd_initstate1(ifp, ifp->ctx->argc, ifp->ctx->argv, options); | ||
| dhcpcd_initstate1(ifp, | ||
| ifp->argc ?: ifp->ctx->argc, |
There was a problem hiding this comment.
Is ?: a valid ternary operator in standard C?
| ipv4_applyaddr(ifp); | ||
| #endif | ||
| } else if (i != argc) { | ||
| /* iface wasnt found above -> it's new. start it. */ |
There was a problem hiding this comment.
This comment is misleading as the interface is not new, it's already been allocated.
The interface however is not active which is clearly noted above. Just remove the comment please.
| } | ||
|
|
||
| #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.
| { | ||
| int i; | ||
| size_t strslen = 0; | ||
| for (i = 0; i < argc; i++) { |
There was a problem hiding this comment.
Whitespace between declaration and code please.
| 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.
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.