chore(deps)!: take the major-version wave, fixing what it exposed - #8
Conversation
Supersedes Dependabot #3 and #4. Those are the same wave from two directions: the "fuzz-deps" group turned out to rewrite the root workspace manifest, because `fuzz/` depends on the workspace by path. Neither was safe to merge as proposed. ## Two of the riskiest majors needed deleting, not migrating `etherparse` 0.15 -> 0.21 and `socket2` 0.5 -> 0.6 were the two large API rewrites in the group. Both crates are declared in three manifests each and imported by **zero** source files -- the same pattern as `quick-xml` and `tui-input` earlier on this branch. Removed. ## The real migration was rand `rand` 0.8 -> 0.9: 24 `thread_rng`, 17 `gen`/`gen::<T>`, and 16 `gen_range` call sites across 10 files, renamed to `rng`, `random`, `random_range`. Every one was a deprecation rather than a removal, so `cargo build` stayed green throughout and only `-D warnings` surfaced them. ## sysinfo 0.38, not 0.39 0.39 declares `rust-version = "1.95"`, well above this workspace's 1.88 floor, and it is directly reachable from `prtip-core`. A local toolchain newer than the MSRV compiles it without complaint; only checking resolved metadata against the declared floor catches it. 0.38.4 is the newest line that fits and carries the same CPU API. `refresh_cpu()`/`global_cpu_info()` -> `refresh_cpu_usage()`/ `global_cpu_usage()`. Verified after: 0 of 502 resolved packages require more than 1.88. Also `colored` 3.1, `dirs` 6.0, `governor` 0.10, `ipnetwork` 0.21 (serde moved behind a feature; `ScanTarget` derives over `IpNetwork`, so it is now explicit), `mlua` 0.12, `nix` 0.31, `rlimit` 0.11, `thiserror` 2.0, `toml` 1.1, `windows` 0.62, `x509-parser` 0.18. `dirs`, `toml` and `ipnetwork` had crate-local pins outside the workspace table and were each being built twice. ## What the wave exposed The `ipnetwork` bump broke one test, and following it down found three real defects that had nothing to do with dependency versions. **Unbounded expansion (the serious one).** `expand_hosts()` was `network.iter().collect()` with no limit, so `prtip -sT -p 80 0.0.0.0/0` tried to allocate 4.3 billion addresses -- about 68 GB -- from eight characters of user input. Measured before and after: the old build printed its banner and hung until SIGKILL (exit 137); it now exits 1 immediately with Invalid target: 0.0.0.0/0 contains 4294967296 addresses, above the 16777216 limit for host expansion. Earlier releases were protected only by accident -- expanding a `/0` overflowed an integer and panicked fast. `ipnetwork` 0.21 removed the overflow and turned a loud failure into a silent hang. The bound is the check that panic was standing in for, and is exactly what the test's own note ("Future enhancement: should validate CIDR size before expansion") had been asking for. `expand_hosts()` now returns `Result`; 12 call sites updated. **`host_count()` returned 0 for a /31.** RFC 3021 makes both addresses of a point-to-point /31 usable, but the code subtracted network and broadcast unconditionally below /32. Latent while nothing trusted the number; it became `Scanner error: Result queue full (0/0)` the moment the scheduler sized its queue from it. **`host_count()` and `expand_hosts()` measured different things.** Usable hosts versus every address -- so a /30 counted 2 and scanned 4. Split into `address_count()` (what expansion yields, what queues size from) and `host_count()` (usable hosts, what a user is shown), with a test pinning `address_count()` to the actual expansion length across five prefixes. My own test caught this; the /31 fix alone would have left the /30 case broken. ## Two optimizations from the same reading - `scheduler.rs` computed `targets.iter().map(|t| t.expand_hosts().len()).sum()` -- materialising every address purely to count it. Now O(1) arithmetic. - `ScanTarget::first_host()` added. Four call sites in `decoy_scanner.rs` expanded an entire target to read `hosts[0]`, so a /8 allocated 268 MB to obtain one address. It is `iter().next()`, and works even on targets too large to expand. ## Test harness The CLI test helper selected the release binary with `release_path.exists()`, true for a *directory*. `docker run -v "$PWD/target/release/prtip:/prtip"` with no release build present makes Docker create the mount source as a root-owned directory, after which all 52 CLI integration tests failed with a baffling `PermissionDenied` rather than falling back to the working debug binary. Now `is_file()`. ## Verification cargo test --workspace 2,587 passed, 0 failed, 121 ignored (exit 0) cargo clippy --workspace --all-targets --locked -- -D warnings exit 0 cargo fmt --all -- --check exit 0 cargo audit exit 0 cargo deny check exit 0 advisories, licenses, bans, sources both generators --check exit 0 resolved MSRV 0 of 502 packages require > 1.88 2,581 -> 2,587; the +6 are regression tests for the defects above, including one asserting `/0` is refused and one that `first_host()` works on a target `expand_hosts()` rejects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NBDCJvLbq7nuor7RtT57rD
|
🤖 Hi @doublegate, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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 |
|
🤖 I'm sorry @doublegate, but I was unable to process your request. Please see the logs for more details. |
There was a problem hiding this comment.
🟡 Changes recommended
IPv6 address_count() can overflow for /64 and potentially bypass expansion limits, and scheduler sizing should validate/saturate counts to avoid overflow/panic before rejecting oversized targets.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR consolidates a large dependency-major upgrade wave across the Rust workspace and addresses multiple correctness and reliability issues that surfaced as a result (notably around target expansion/counting and test harness behavior), while updating call sites to rand/sysinfo API changes.
Changes:
- Upgrade and de-duplicate workspace dependencies (e.g.,
rand0.9,ipnetwork0.21 w/ serde feature,sysinfo0.38,toml1.1), removing unused majors (socket2,etherparse) and folding crate-local pins into workspace versions. - Make
ScanTargetcounting/expansion safer and more explicit (address_count, correctedhost_countsemantics, bounded+fallibleexpand_hosts, andfirst_hostto avoid full materialization). - Update scanners/tests to the new APIs and improved target semantics; fix CLI integration-test binary selection (
is_file()).
File summaries
| File | Description |
|---|---|
| crates/prtip-tui/Cargo.toml | Switch ipnetwork to workspace dependency to avoid duplicate versions. |
| crates/prtip-scanner/tests/integration_scanner.rs | Update target expansion test to handle fallible expand_hosts(). |
| crates/prtip-scanner/tests/common/error_injection.rs | Migrate probabilistic failure injection to rand 0.9 APIs. |
| crates/prtip-scanner/src/udp_scanner.rs | Update source-port randomization to rand 0.9 APIs. |
| crates/prtip-scanner/src/timing.rs | Update jitter generation to rand 0.9 APIs. |
| crates/prtip-scanner/src/tcp_connect.rs | Propagate expand_hosts() failures via ? instead of assuming infallible expansion. |
| crates/prtip-scanner/src/syn_scanner.rs | Update RNG usage (rng/random/random_range) and sequence generation. |
| crates/prtip-scanner/src/stealth_scanner.rs | Update RNG usage for source ports / sequence numbers. |
| crates/prtip-scanner/src/scheduler.rs | Stop allocating expansions purely to count; use address_count() for sizing. |
| crates/prtip-scanner/src/idle/idle_scanner.rs | Update spoofed SYN randomization to rand 0.9 APIs. |
| crates/prtip-scanner/src/decoy_scanner.rs | Avoid full expansion when only first address is needed; update RNG APIs. |
| crates/prtip-scanner/Cargo.toml | Remove unused deps (socket2, etherparse); bump x509-parser; use workspace toml. |
| crates/prtip-network/src/packet_builder.rs | Update packet builder random defaults to rand 0.9 APIs. |
| crates/prtip-network/src/ipv6_packet.rs | Update IPv6 fragment ID generation to rand 0.9 APIs. |
| crates/prtip-network/src/fragmentation.rs | Update fragment ID generation to rand 0.9 APIs. |
| crates/prtip-network/Cargo.toml | Remove unused deps (socket2, etherparse). |
| crates/prtip-core/tests/integration.rs | Update expansion test to handle fallible expand_hosts(). |
| crates/prtip-core/src/types.rs | Add address_count(), fix /31 host counting, add bounded/fallible expand_hosts(), add first_host(), and add regression tests. |
| crates/prtip-core/src/retry.rs | Update retry jitter to rand 0.9 APIs. |
| crates/prtip-core/src/resource_monitor.rs | Update CPU refresh/reading to sysinfo 0.38 APIs. |
| crates/prtip-core/Cargo.toml | Switch dirs to workspace dependency. |
| crates/prtip-cli/tests/test_edge_cases.rs | Update /0 behavior test commentary to reflect bounded expansion behavior. |
| crates/prtip-cli/tests/common/mod.rs | Fix binary selection to require an actual file (is_file()) for release/debug paths. |
| crates/prtip-cli/Cargo.toml | Switch dirs to workspace dependency. |
| CHANGELOG.md | Document the dependency wave and the exposed/fixed defects and safeguards. |
| Cargo.toml | Update workspace dependency versions/features; remove unused deps; pin sysinfo below MSRV-breaking line. |
| Cargo.lock | Refresh lockfile for upgraded majors, removals, and dependency graph changes. |
Review details
Suppressed comments (3)
crates/prtip-core/src/types.rs:97
address_count()can overflow for IPv6 prefixes like /64 (2^64), which may wrap/panic and then letexpand_hosts()attempt to collect an effectively unbounded iterator. Usechecked_powand clamp tou64::MAX(or otherwise signal “too large”) to keep the count monotonic and prevent bypassing the expansion limit.
This issue also appears on line 167 of the same file.
IpNetwork::V6(net) => {
let prefix = net.prefix();
if prefix >= 64 {
2u64.pow((128 - prefix) as u32)
} else {
crates/prtip-core/src/types.rs:168
- The
expand_hosts()error message says “a /8 is the widest accepted”, which is only meaningful for IPv4; for IPv6 the widest accepted prefix would be much narrower. Consider making the guidance version-agnostic to avoid misleading users.
"{} contains {} addresses, above the {} limit for host expansion. \
Narrow the prefix (a /8 is the widest accepted) or split the scan.",
crates/prtip-core/src/types.rs:1085
test_expand_hosts_limit_boundaryuseshost_count()to test the expansion cap, but the cap is defined in terms of allocated addresses (whatexpand_hosts()yields). Usinghost_count()makes the boundary check off by 2 for IPv4 networks (network/broadcast), weakening the regression coverage.
// A /8 is exactly at the cap and must still be permitted; a /7 is over.
let at_limit = ScanTarget::parse("10.0.0.0/8").unwrap();
assert!(at_limit.host_count() <= ScanTarget::MAX_EXPANDABLE_HOSTS);
let over_limit = ScanTarget::parse("10.0.0.0/7").unwrap();
assert!(over_limit.host_count() > ScanTarget::MAX_EXPANDABLE_HOSTS);
- Files reviewed: 26/27 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // These two must never disagree: the scheduler sizes its result queue | ||
| // from host_count() and then scans expand_hosts(). They diverged for | ||
| // /31, where host_count() returned 0 while expansion produced 2. |
| // address_count(), not host_count(): this sizes a queue for what | ||
| // expand_hosts() actually yields, which includes the network and | ||
| // broadcast addresses. host_count() reports usable hosts and would | ||
| // undersize the queue by 2 per target. | ||
| let estimated_hosts: usize = targets.iter().map(|t| t.address_count() as usize).sum(); |
`Test (windows-latest)` failed on this branch while the Linux suite was green:
the `windows` 0.52 -> 0.62 bump moved `BOOL` out of `Win32::Foundation`.
error[E0432]: unresolved import `windows::Win32::Foundation::BOOL`
--> crates/prtip-network/src/privilege.rs:160:9
| no `BOOL` in `Win32::Foundation`
Reproduced locally with `cargo check --target x86_64-pc-windows-gnu -p
prtip-network` rather than waiting on CI; `prtip-network` now checks clean for
that target. Both call sites -- `privilege.rs` and the privilege test -- import
from `windows::core` instead.
A whole-workspace Windows check still stops at `aws-lc-sys`, which needs a
Windows C toolchain this machine does not have. That is a local
cross-compilation limit, not a defect: CI builds natively on windows-latest
with MSVC.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NBDCJvLbq7nuor7RtT57rD
Supersedes #3 and #4. Those are the same wave from two directions — the "fuzz-deps" group turned out to rewrite the root workspace manifest, because
fuzz/depends on the workspace by path. Neither was safe to merge as proposed.Two of the riskiest majors needed deleting, not migrating
etherparse0.15 → 0.21 andsocket20.5 → 0.6 were the two large API rewrites in the group. Both are declared in three manifests each and imported by zero source files — the same pattern asquick-xmlandtui-input. Removed.The real migration was
rand0.8 → 0.9: 24
thread_rng, 17gen/gen::<T>, 16gen_rangesites across 10 files →rng,random,random_range. All were deprecations, not removals, socargo buildstayed green and only-D warningssurfaced them.sysinfo0.38, not 0.390.39 declares
rust-version = "1.95"— above this workspace's 1.88 floor, and directly reachable fromprtip-core. A local toolchain newer than the MSRV compiles it happily; only checking resolved metadata against the declared floor catches it. Verified after: 0 of 502 resolved packages require > 1.88.Also
colored3.1,dirs6.0,governor0.10,ipnetwork0.21 (serde moved behind a feature — now explicit),mlua0.12,nix0.31,rlimit0.11,thiserror2.0,toml1.1,windows0.62,x509-parser0.18.dirs/toml/ipnetworkhad crate-local pins outside the workspace table and were each built twice.What the wave exposed
The
ipnetworkbump broke one test. Following it down found three real defects unrelated to dependency versions.Unbounded expansion — the serious one
expand_hosts()wasnetwork.iter().collect()with no limit, soprtip -sT -p 80 0.0.0.0/0— eight characters of user input — tried to allocate 4.3 billion addresses, ~68 GB.Measured before and after:
Invalid target: 0.0.0.0/0 contains 4294967296 addresses, above the 16777216 limit for host expansionEarlier releases were protected only by accident — expanding a
/0overflowed an integer and panicked fast.ipnetwork0.21 removed the overflow, turning a loud failure into a silent hang. The bound is the check that panic was standing in for, and is what the test's own note — "Future enhancement: should validate CIDR size before expansion" — had been asking for.expand_hosts()now returnsResult; 12 call sites updated.host_count()returned 0 for a/31RFC 3021 makes both addresses of a point-to-point
/31usable, but the code subtracted network and broadcast unconditionally below/32. Latent while nothing trusted the number — it becameScanner error: Result queue full (0/0)the moment the scheduler sized its queue from it.host_count()andexpand_hosts()measured different thingsUsable hosts vs. every address — so a
/30counted 2 and scanned 4. Split intoaddress_count()(what expansion yields; what queues size from) andhost_count()(usable hosts; what a user sees), with a test pinningaddress_count()to the actual expansion length across five prefixes. My own new test caught this — the/31fix alone would have left the/30case broken.Two optimizations from the same reading
scheduler.rscomputedtargets.iter().map(|t| t.expand_hosts().len()).sum()— materialising every address purely to count it. Now O(1).ScanTarget::first_host()added. Fourdecoy_scanner.rssites expanded a whole target to readhosts[0], so a/8allocated 268 MB for one address. It'siter().next(), and works even on targets too large to expand.Test harness
The CLI helper picked the release binary with
release_path.exists()— true for a directory. Adocker runbind-mount oftarget/release/prtipwith no release build present makes Docker create the mount source as a root-owned directory, after which all 52 CLI integration tests failed with a bafflingPermissionDeniedinstead of falling back to the working debug binary. Nowis_file().Verification
2,581 → 2,587; the +6 are regression tests for the defects above, including one asserting
/0is refused and one thatfirst_host()works on a targetexpand_hosts()rejects.🤖 Generated with Claude Code
https://claude.ai/code/session_01NBDCJvLbq7nuor7RtT57rD