Conversation
Apple route(4) has some limitations as does getifaddrs(3). Basically there is no means of being notified of carrier state because Apple only reports this via media state which is an ioctl. The good news is that we can build macOS on github so we can get some BSD traceability at least. Fixes #524
|
Caution Review failedPull request was closed or merged during review WalkthroughThis PR enables dhcpcd compilation and operation on macOS by adding platform detection in the build system, defining Apple-specific RFC 3542 support across network files, updating the BSD interface driver with Apple-specific logic for carrier polling and link-state detection, and adjusting privilege separation and header inclusion patterns for cross-BSD compatibility. ChangesmacOS Support Implementation
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/if-bsd.c (1)
1263-1281:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAlign
if_announcecall-site guards with its definition guard.
if_announceis compiled only whenIFAN_ARRIVALis defined (Line 1263), but the dispatch call is guarded only byRTM_IFANNOUNCE(Lines 1582-1585). That can produce an undefined symbol/implicit declaration build failure.Suggested fix
-#ifdef RTM_IFANNOUNCE +#if defined(RTM_IFANNOUNCE) && defined(IFAN_ARRIVAL) case RTM_IFANNOUNCE: return if_announce(ctx, (const void *)rtm); `#endif`Also applies to: 1582-1585
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/if-bsd.c` around lines 1263 - 1281, The if_announce function is only defined when IFAN_ARRIVAL is set but the call-site (RTM_IFANNOUNCE case in the netlink/route message dispatch) is not similarly guarded, which can cause undefined symbol errors; wrap the RTM_IFANNOUNCE dispatch branch in the same `#ifdef` IFAN_ARRIVAL (or alternatively undefine the function guard and always compile if_announce) so that the call to if_announce and the function definition are compiled under the same condition (reference: function if_announce, macro IFAN_ARRIVAL, dispatch RTM_IFANNOUNCE, and handler dhcpcd_handleinterface).
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
14-30: ⚖️ Poor tradeoffConsider adding explicit permissions and pinning action references.
Static analysis flagged security improvements:
- No
permissions:block - consider adding minimal permissions (e.g.,contents: read)- Actions not pinned to SHA - consider using full commit SHA for reproducibility
These are optional security hardening measures.
Example permissions block
jobs: macos: permissions: contents: read strategy: ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yml around lines 14 - 30, The workflow's macos job is missing an explicit permissions block and uses unpinned action references; update the job definition for the "macos" matrix job to add a minimal permissions block (e.g., set contents: read) and replace loose action refs like actions/checkout@v6 with pinned commit SHAs (use the full commit SHA for the checkout action and any other third-party actions in the steps) so the "macos" job's steps (the uses: actions/checkout and any other uses) are reproducible and run with least privilege.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yml:
- Line 13: The comment in the workflow contains a typo: change "plaform" to
"platform" in the comment line that currently reads "macos is a good plaform as
we know it ships with a sanitier we can use" so it correctly reads "macos is a
good platform as we know it ships with a sanitier we can use".
In `@src/if-bsd.c`:
- Around line 438-448: The preprocessor guard is mismatched: the block is
enabled by SIOCGIFXMEDIA but the code uses SIOCGIFMEDIA/struct ifmediareq/ifmr
and checks ifmr.ifm_status against IFM_AVALID/IFM_ACTIVE, causing platforms that
only define SIOCGIFMEDIA to skip this carrier detection and return LINK_UNKNOWN;
change the `#elif` defined(SIOCGIFXMEDIA) to `#elif` defined(SIOCGIFMEDIA) (or add
an || defined(SIOCGIFMEDIA)) so the ioctl(ctx->pf_inet_fd, SIOCGIFMEDIA, &ifmr)
call and subsequent IFM_AVALID/IFM_ACTIVE checks run on platforms that only
expose SIOCGIFMEDIA and thus return LINK_UP or LINK_DOWN instead of
LINK_UNKNOWN.
In `@src/privsep.c`:
- Line 130: The function ps_dropprivs contains an unconditional early "return 0"
that prevents executing the privilege-drop sequence; remove that early return in
ps_dropprivs so the subsequent chroot, chdir, group clearing, UID/GID drop and
RLIMIT hardening code runs, ensure calls like chroot(), chdir(), setgroups(0,
NULL), setresgid()/setegid()/setgid(), setresuid()/seteuid()/setuid() and
setrlimit() are performed in the intended order, propagate and handle errors
from those calls and only return 0 at the end of ps_dropprivs on success (or a
non-zero error on failure).
---
Outside diff comments:
In `@src/if-bsd.c`:
- Around line 1263-1281: The if_announce function is only defined when
IFAN_ARRIVAL is set but the call-site (RTM_IFANNOUNCE case in the netlink/route
message dispatch) is not similarly guarded, which can cause undefined symbol
errors; wrap the RTM_IFANNOUNCE dispatch branch in the same `#ifdef` IFAN_ARRIVAL
(or alternatively undefine the function guard and always compile if_announce) so
that the call to if_announce and the function definition are compiled under the
same condition (reference: function if_announce, macro IFAN_ARRIVAL, dispatch
RTM_IFANNOUNCE, and handler dhcpcd_handleinterface).
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 14-30: The workflow's macos job is missing an explicit permissions
block and uses unpinned action references; update the job definition for the
"macos" matrix job to add a minimal permissions block (e.g., set contents: read)
and replace loose action refs like actions/checkout@v6 with pinned commit SHAs
(use the full commit SHA for the checkout action and any other third-party
actions in the steps) so the "macos" job's steps (the uses: actions/checkout and
any other uses) are reproducible and run with least privilege.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 137c7d99-3a80-4711-953c-92143da77d52
📒 Files selected for processing (14)
.github/workflows/build.ymlcompat/crypt/sha256.cconfiguresrc/dhcp6.csrc/dhcpcd.csrc/if-bsd.csrc/if.csrc/if.hsrc/ipv4.hsrc/ipv6nd.csrc/privsep-bpf.csrc/privsep-bsd.csrc/privsep.csrc/sa.h
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
13-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTypo in comment: "sanitier" → "sanitizer".
The comment contains a typo that should be corrected.
📝 Fix the typo
- # macos is a good platform as we know it ships with a sanitier we can use + # macos is a good platform as we know it ships with a sanitizer we can use🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yml at line 13, Fix the typo in the inline comment that reads "macos is a good platform as we know it ships with a sanitier we can use" by changing "sanitier" to "sanitizer" so the comment reads "macos is a good platform as we know it ships with a sanitizer we can use"; update the single comment string accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/build.yml:
- Line 13: Fix the typo in the inline comment that reads "macos is a good
platform as we know it ships with a sanitier we can use" by changing "sanitier"
to "sanitizer" so the comment reads "macos is a good platform as we know it
ships with a sanitizer we can use"; update the single comment string
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e5c678f-abc6-4fba-8c44-ab7ca967c77e
📒 Files selected for processing (2)
.github/workflows/build.ymlsrc/if-bsd.c
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Apple route(4) has some limitations as does getifaddrs(3). Basically there is no means of being notified of carrier state because Apple only reports this via media state which is an ioctl.
The good news is that we can build macOS on github so we can get some BSD traceability at least.
Fixes #524