feat(platform): add macOS listener inspector - #301
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe platform crate adds a listener-inspection abstraction. macOS discovers loopback TCP listeners through kernel table parsing and netstat, while Linux, Windows, and unsupported targets return typed unsupported errors. Public exports and macOS-specific tests are updated accordingly. ChangesLoopback listener inspection
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The acceptance-only kernel parser does not yet handle two states emitted by the XNU contract, so it can fail on an empty table or accept an unstable snapshot.
Reviewed changes — This PR introduces a target-specific listener-inspection facade and an acceptance-only macOS kernel-table inspector while preserving the current production macOS inspection path.
- Add the
platform::listenerfacade — Public listener APIs now dispatch to private macOS, Linux, Windows, or fallback implementations with typed unsupported behavior off macOS. - Add private macOS PCB-table inspection —
kernel_table.rsfetchesnet.inet.tcp.pcblist_n, parses bounded private-ABI records, and detects IPv4/IPv6 loopback and wildcard listeners. - Preserve production listener detection — macOS continues to union
netstat-esrresults with/usr/sbin/netstatoutput while the kernel inspector remains acceptance-only. - Cover parser and platform behavior — Synthetic snapshots exercise address families, states, generations, malformed records, and fetch retries; live macOS coverage samples four controlled listener classes.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — The new commit resolves the empty macOS PCB snapshot failure and documents the intended snapshot-coherence semantics.
- Accepted XNU's empty PCB snapshot shape —
parse_tcp_tablenow returns an empty listener set for a single valid zero-countxinpgenenvelope while preservingMissingTrailerfor nonzero snapshots. - Clarified the generation check — The parser documents why global
xig_sogenchanges do not invalidate a TCP PCB snapshot, matching XNU and Apple netstat behavior. - Added regression coverage — A snapshot fixture covers both zero-count and nonzero single-envelope tables.
GPT Sol ｜ 𝕏
![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
🧹 Nitpick comments (2)
crates/platform/src/listener/macos.rs (1)
24-31: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider making the
netstatsubprocess a fallback rather than an unconditional second source.Every call now spawns
/usr/sbin/netstatin addition to the in-process socket-table query, andloopback_tcp_port_has_listenerfunnels through here, so a caller polling a port pays a process spawn per check. Since the netstat pass exists to cover listeners the crate query misses, running it only when the socket table yields nothing keeps the coverage while removing the spawn from the common path.♻️ Suggested fallback ordering
pub(super) fn loopback_tcp_listener_ports() -> Result<BTreeSet<u16>, PlatformError> { - let mut ports = loopback_tcp_listener_ports_from_socket_table()?; - ports.extend(parse_netstat_tcp_listener_ports( - &netstat_tcp_socket_table()? - )); - - Ok(ports) + let ports = loopback_tcp_listener_ports_from_socket_table()?; + if !ports.is_empty() { + return Ok(ports); + } + + Ok(parse_netstat_tcp_listener_ports( + &netstat_tcp_socket_table()?, + )) }If the two sources are known to disagree in practice, a short comment recording that would justify keeping both.
🤖 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 `@crates/platform/src/listener/macos.rs` around lines 24 - 31, Update loopback_tcp_listener_ports to invoke netstat_tcp_socket_table and parse its results only when loopback_tcp_listener_ports_from_socket_table returns no ports. Preserve the existing error propagation and merge behavior for the fallback so missed listeners remain covered, while avoiding the subprocess on the common non-empty path.crates/platform/src/listener/macos/kernel_table.rs (1)
637-703: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild fixtures from the parser's offset constants instead of literals.
18..20,28..36,44,64..80,76..80,36..40, and the24envelope length restate offsets that already exist as constants. On private-ABI code the fixtures are the main regression guard, so keeping the two copies in sync by hand is the part most likely to drift.♻️ Suggested wiring
use super::{ - FetchError, INP_IPV4, INP_IPV6, MAX_ATTEMPTS, TCPS_LISTEN, XINPCB_MINIMUM_LENGTH, - XSO_INPCB, XSO_TCPCB, XTCPCB_MINIMUM_LENGTH, fetch_tcp_table_with, - loopback_tcp_listener_ports, parse_tcp_table, + FetchError, INP_IPV4, INP_IPV6, INPCB_GENERATION_OFFSET, INPCB_IPV4_ADDRESS_OFFSET, + INPCB_LOCAL_ADDRESS_LENGTH, INPCB_LOCAL_ADDRESS_OFFSET, INPCB_LOCAL_PORT_OFFSET, + INPCB_VERSION_FLAGS_OFFSET, MAX_ATTEMPTS, TCPCB_STATE_OFFSET, TCPS_LISTEN, + XINPCB_MINIMUM_LENGTH, XINPGEN_LENGTH, XSO_INPCB, XSO_TCPCB, XTCPCB_MINIMUM_LENGTH, + fetch_tcp_table_with, loopback_tcp_listener_ports, parse_tcp_table, };- tcp_record[36..40].copy_from_slice(&state.to_ne_bytes()); + tcp_record[TCPCB_STATE_OFFSET..TCPCB_STATE_OFFSET + 4] + .copy_from_slice(&state.to_ne_bytes());- internet_record[18..20].copy_from_slice(&port.to_be_bytes()); - internet_record[28..36].copy_from_slice(&generation.to_ne_bytes()); - internet_record[44] = version_flags; + internet_record[INPCB_LOCAL_PORT_OFFSET..INPCB_LOCAL_PORT_OFFSET + 2] + .copy_from_slice(&port.to_be_bytes()); + internet_record[INPCB_GENERATION_OFFSET..INPCB_GENERATION_OFFSET + 8] + .copy_from_slice(&generation.to_ne_bytes()); + internet_record[INPCB_VERSION_FLAGS_OFFSET] = version_flags; match address { IpFixture::V4(address) => { - internet_record[76..80].copy_from_slice(&address.octets()); + internet_record[INPCB_IPV4_ADDRESS_OFFSET..INPCB_IPV4_ADDRESS_OFFSET + 4] + .copy_from_slice(&address.octets()); } IpFixture::V6(address) => { - internet_record[64..80].copy_from_slice(&address.octets()); + internet_record[INPCB_LOCAL_ADDRESS_OFFSET + ..INPCB_LOCAL_ADDRESS_OFFSET + INPCB_LOCAL_ADDRESS_LENGTH] + .copy_from_slice(&address.octets()); } }fn envelope(count: u32, generation: u64, socket_generation: u64) -> Vec<u8> { - let mut bytes = Vec::with_capacity(24); - bytes.extend(24_u32.to_ne_bytes()); + let mut bytes = Vec::with_capacity(XINPGEN_LENGTH); + bytes.extend((XINPGEN_LENGTH as u32).to_ne_bytes());The same applies to the
- 24trailer offset at Line 517.🤖 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 `@crates/platform/src/listener/macos/kernel_table.rs` around lines 637 - 703, Update the fixture builder methods push_listener, push_internet_pcb, and envelope to derive all record field ranges and envelope/trailer lengths from the parser’s existing offset and size constants instead of literal offsets such as 18..20, 28..36, 44, 64..80, 76..80, 36..40, and 24. Also replace the trailer’s “- 24” offset with the corresponding existing constant, keeping fixture layout synchronized with the parser definitions.
🤖 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 `@crates/platform/src/listener/macos/kernel_table.rs`:
- Around line 586-612: Mark
live_kernel_table_repeatedly_detects_all_controlled_listener_classes as an
acceptance-only test using the repository’s existing acceptance-test gating
mechanism, so it is excluded from the normal macOS test pass while remaining
runnable explicitly in acceptance coverage.
---
Nitpick comments:
In `@crates/platform/src/listener/macos.rs`:
- Around line 24-31: Update loopback_tcp_listener_ports to invoke
netstat_tcp_socket_table and parse its results only when
loopback_tcp_listener_ports_from_socket_table returns no ports. Preserve the
existing error propagation and merge behavior for the fallback so missed
listeners remain covered, while avoiding the subprocess on the common non-empty
path.
In `@crates/platform/src/listener/macos/kernel_table.rs`:
- Around line 637-703: Update the fixture builder methods push_listener,
push_internet_pcb, and envelope to derive all record field ranges and
envelope/trailer lengths from the parser’s existing offset and size constants
instead of literal offsets such as 18..20, 28..36, 44, 64..80, 76..80, 36..40,
and 24. Also replace the trailer’s “- 24” offset with the corresponding existing
constant, keeping fixture layout synchronized with the parser definitions.
🪄 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 Plus
Run ID: 4047e29f-54c0-4362-a129-eb65e9c6bb0a
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockcrates/platform/src/listener/macos/snapshots/platform__listener__implementation__kernel_table__tests__empty_pcb_fixture_matches_xnu_single_envelope_shape.snapis excluded by!**/*.snapcrates/platform/src/listener/macos/snapshots/platform__listener__implementation__kernel_table__tests__live_kernel_table_repeatedly_detects_all_controlled_listener_classes.snapis excluded by!**/*.snapcrates/platform/src/listener/macos/snapshots/platform__listener__implementation__kernel_table__tests__malformed_pcb_fixtures_return_deterministic_typed_errors.snapis excluded by!**/*.snapcrates/platform/src/listener/macos/snapshots/platform__listener__implementation__kernel_table__tests__pcb_fixture_covers_address_families_states_generations_and_unknown_records.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
Cargo.tomlcrates/platform/Cargo.tomlcrates/platform/src/lib.rscrates/platform/src/listener.rscrates/platform/src/listener/linux.rscrates/platform/src/listener/macos.rscrates/platform/src/listener/macos/kernel_table.rscrates/platform/src/listener/unsupported.rscrates/platform/src/listener/windows.rs
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — The latest commit keeps live macOS kernel-table acceptance coverage out of normal test runs while preserving explicit execution.
- Gated live listener acceptance coverage — Added
#[ignore]tolive_kernel_table_repeatedly_detects_all_controlled_listener_classesso routine macOS test runs no longer depend on the host's live TCP PCB table.
GPT Sol ｜ 𝕏
1 participant
Summary
platform::listenerfacade with private macOS, Linux, Windows, and fallback implementationsnet.inet.tcp.pcblist_nUnsupportednetstat-esrand/usr/sbin/netstatWhy
PV needs a narrowly scoped, PV-owned listener inspection capability before it can safely replace the current dual macOS mechanisms. The existing published-library spike proved that named kernel-table lookup is viable, while also exposing IPv6 parsing, stale numeric sysctl, and failure-handling problems that require a purpose-built implementation.
Impact
There is no production listener-inspection behavior change in this PR. The new kernel inspector remains acceptance-only until the supported macOS version and architecture matrix passes. The existing public platform APIs are unchanged.
Validation
cargo nextest run -p platform --all-features --locked— 50/50 passedcargo clippy --workspace --all-targets --all-features --locked -- -D warningscargo shearcargo fmt --all --checkgit diff --checkRemaining acceptance gate
Before cutover:
netstatcomparison matrix on every supported macOS version beginning with macOS 13netstat-esrand/usr/sbin/netstattogether only after that evidence passesEnvironment notes
A Linux cross-target check was blocked by the local host lacking
x86_64-linux-gnu-gccfor C dependencies. A full-workspace nextest attempt compiled the workspace but stalled while macOS loaded the final unrelated test binaries for discovery; the changed platform crate's complete 50-test nextest suite passed.Summary by CodeRabbit