Repository navigation
Fix core panic paths and OUI generator silent-corruption modes (C-01, C-02, C-10, C-36, A-09) - #172
Merged
Merged
Conversation
C-01: `Instant - Duration` panics on underflow, and `now - ACTIVE_HOLD -
ACTIVE_HOLD` is reachable within ~360ms of the monotonic clock's origin --
so constructing a NetworkMonitor early in process life could panic. Uses
checked_sub, degrading to "as early as representable", which reads as
inactive exactly as intended.
C-02: per-interface byte counters were summed with `+=` across every
adapter, which panics on overflow in a debug build. Now saturating, and
the accumulators are explicitly u64 rather than inferred.
C-10: the MCP server clamped concurrency to 4096 while Ops::new re-clamped
to 1024, so 1025..=4096 was accepted at the API boundary and then silently
reduced -- and a comment claimed the two bounds matched. Introduces
netscli_core::MAX_CONCURRENCY and derives both from it, so they cannot
drift.
C-36: parse_wireshark_manuf sliced `&hex[0..2]` where `hex` had only had
colons removed, not hex-filtering, so any entry whose first token began
with a multi-byte character panicked. Routed through the existing
canon_prefix, which filters to ASCII hex first and also drops the
malformed rows that would have produced junk vendor keys.
A-09, three compounding defects that let the generator ship a gutted
vendor database while reporting success:
- No error_for_status, so a 403 body was handed to the CSV parser,
which found no header row and returned an empty map. IEEE rate-limits
unknown user agents, so this was reachable -- and the client set none.
Both are fixed: status is checked, and a real user agent is sent.
- A missing header row was indistinguishable from a legitimately empty
file. Now warns explicitly.
- The output was overwritten unconditionally with no floor. Now refuses
to write a dataset below 90% of the existing entry count, naming the
override. 90% rather than any-shrinkage because registrations do lapse
and a strict check would fail every run.
B-06 was already fixed in earlier work; verified rather than assumed.
Adds tests for C-01, C-02 and C-10. clippy clean with and without
--features pcap.
Adding the C-01 and C-02 regression tests pushed stats.rs to 311 lines, over the 300-line cap. Split to stats/tests.rs, matching the pattern netscli-mcp already uses for dispatch/tests.rs. Verified no tests were lost rather than assuming it: origin/main runs 65 lib tests, this branch runs 67 -- exactly the two added.
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
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.
Panics
Instant - Durationpanics on underflow, andnow - ACTIVE_HOLD - ACTIVE_HOLDis reachable within ~360 ms of the monotonic clock's origin — so constructing aNetworkMonitorearly in process life could panic. Nowchecked_sub, degrading to "as early as representable", which reads as inactive exactly as intended.+=across every adapter — panics on overflow in a debug build. Now saturating, with the accumulators explicitlyu64rather than inferred.parse_wireshark_manufsliced&hex[0..2]wherehexhad only had colons removed — not hex-filtered — so any entry whose first token began with a multi-byte character panicked. Routed through the existingcanon_prefix.C-10 — a bound that lied
The MCP server clamped concurrency to 4096 while
Ops::newre-clamped to 1024, so 1025–4096 was accepted at the API boundary and then silently reduced. A comment inops/config.rsclaimed the two "match".Introduces
netscli_core::MAX_CONCURRENCYand derives both from it, so they cannot drift. The test asserts both that the bound is enforced and that a value just inside it survives untouched — otherwise the constant would only be an upper limit, not the real one.A-09 — the generator could ship a gutted vendor database and report success
Three compounding defects:
error_for_status— a 403 body went straight to the CSV parser, which found no header row and returned an empty map. IEEE rate-limits unknown user agents, andClient::builder()set none, so this was genuinely reachable. Both fixed.90% rather than any-shrinkage because registrations do lapse — a strict check would fail every run.
Note
B-06 (the
&key[..6]byte-slice panic) was already fixed in earlier work. I verified that rather than assuming it from the audit.Tests added for C-01, C-02 and C-10.
cargo clippy -D warningsclean both with and without--features pcap— I check the pcap path locally now, after CI caught me on it in #169.