Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughThis pull request fixes several validation and correctness issues across the database reader, decoder, and result computation. Changes include rejecting invalid search tree sizes, preventing IPv6 CIDR iteration in IPv4-only databases, treating truncated payloads as invalid during verification, and computing network prefixes using the reader's measured IPv4 subtree depth instead of hardcoded offsets. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/reader_test.rs`:
- Around line 1135-1144: The test in reader_test.rs currently asserts exact
equality on the error message inside the matches! pattern
(Err(MaxMindDbError::InvalidDatabase { ref message, .. }) and message == "search
tree appears to have a cycle or invalid structure (exceeded 492 iterations)"),
which couples the test to the fixture node_count; update the pattern to match
more robustly by checking the message starts with or contains the invariant
substring (e.g., message.starts_with("search tree appears to have a cycle") or
message.contains("search tree appears to have a cycle")) so the test no longer
depends on the exact iteration count.
In `@src/reader.rs`:
- Around line 730-751: The error message in search_tree_size_bytes is misleading
because the only failure is a checked_mul overflow; change the MaxMindDbError
text in search_tree_size_bytes to indicate an overflow or impossibly-large
search tree size (e.g., "search tree size calculation overflowed or is
impossibly large") so it is clearly distinct from the overlap/extent error
produced by validate_search_tree_layout; update the message used in
search_tree_size_bytes (and keep validate_search_tree_layout's existing message)
to make the two failure modes unambiguous.
- Around line 623-633: The code currently calls
find_metadata_start(self.buf.as_ref()) twice via verify_metadata() and
verify_database(); change verify() to compute metadata_start once (e.g., let
metadata_start = find_metadata_start(self.buf.as_ref())?) and pass that value
into verify_metadata(), verify_search_tree(), and verify_data_section() (or
store it on Reader if preferred), updating the signatures of
verify_metadata(&self, metadata_start: usize) and verify_database(&self,
metadata_start: usize) (and any helper like validate_search_tree_layout and
verify_data_section to accept the precomputed metadata_start) so the buffer scan
happens only once; keep pointer_base usage as-is since it is already cached at
construction.
In `@src/result.rs`:
- Around line 115-133: The branch selection that infers whether a prefix came
from iteration vs lookup by testing prefix_len ≤ 32 is ambiguous when
ipv4_start_bit_depth ≤ 32; instead add an explicit source flag on the
LookupResult (e.g., lookup_source: enum {Iter, Lookup}) and use that flag when
deciding how to adjust IPv4 prefix lengths in the code paths around
ipv4_start_bit_depth, prefix_len, and reader.has_ipv4_subtree(); update the
producer sites (iteration and lookup functions that construct LookupResult) to
set the flag and replace the numeric-based conditionals in the block handling
IpAddr::V6 with explicit checks against LookupResult::Iter vs
LookupResult::Lookup so the prefix subtraction logic is deterministic regardless
of ipv4_start_bit_depth.
In `@test-data`:
- Line 1: The test-data submodule pointer references a non-existent commit
(6e99232bb6a70d5169ecc96ed0614a52017ff654) which causes "not our ref" errors;
verify the correct commit SHA in the actual test-data repository and update the
superproject to point to a valid commit. To fix, check out the test-data repo,
identify the intended commit or branch (or create/push the missing commit), then
in the superproject update the submodule pointer to that valid SHA (or to the
correct branch head) and commit the change so the pointer in the submodule
reference for the test-data submodule matches an existing commit. Ensure you
push both the test-data repo (if you had to add the missing commit) and the
updated superproject commit so CI and other users can initialize submodules
successfully.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a117a835-3d40-4f19-86e9-968b6c5b301c
📒 Files selected for processing (7)
CHANGELOG.mdsrc/decoder.rssrc/lib.rssrc/reader.rssrc/reader_test.rssrc/result.rstest-data
| matches!( | ||
| result, | ||
| Err(MaxMindDbError::InvalidDatabase { ref message, .. }) | ||
| if message | ||
| == "search tree appears to have a cycle or invalid structure (exceeded 492 iterations)" | ||
| ), | ||
| "Expected specific InvalidDatabase error for broken-search-tree, got {:?}", | ||
| result | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pinning the exact iteration count couples the test to the fixture's node_count.
exceeded 492 iterations is node_count * 3, so if MaxMind-DB-test-broken-search-tree-24.mmdb is ever regenerated with a different node count this test fails for an unrelated reason. Consider matching on a substring (message.starts_with("search tree appears to have a cycle")) to reduce coupling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/reader_test.rs` around lines 1135 - 1144, The test in reader_test.rs
currently asserts exact equality on the error message inside the matches!
pattern (Err(MaxMindDbError::InvalidDatabase { ref message, .. }) and message ==
"search tree appears to have a cycle or invalid structure (exceeded 492
iterations)"), which couples the test to the fixture node_count; update the
pattern to match more robustly by checking the message starts with or contains
the invariant substring (e.g., message.starts_with("search tree appears to have
a cycle") or message.contains("search tree appears to have a cycle")) so the
test no longer depends on the exact iteration count.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests