fix(query): clamp offset+limit to avoid uint64 overflow (Fixes #25006) #25049
Conversation
📝 WalkthroughWalkthroughA new function, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Paginate
participant ClampPageRequestLimit
Caller->>Paginate: Call Paginate(pageRequest, ...)
Paginate->>ClampPageRequestLimit: ClampPageRequestLimit(pageRequest)
ClampPageRequestLimit-->>Paginate: returns clamped pageRequest
Paginate->>Paginate: Continue pagination logic with clamped values
Paginate-->>Caller: Return paginated results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. All modifications directly address the overflow handling and validation requirements described in the linked issue. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
types/query/pagination_test.go (1)
379-391: Good test coverage for the overflow scenario.The test correctly verifies that when
Offset + Limitwould overflow uint64, the limit is properly clamped toPaginationMaxLimit - Offset. The test setup withOffset: 5andLimit: ^uint64(0)(MaxUint64) creates a clear overflow condition and validates the fix.Consider adding a few more test cases for comprehensive coverage:
+func (s *paginationTestSuite) TestClampOverflowEdgeCases() { + s.T().Log("verify no clamping when no overflow would occur") + pageReq := &query.PageRequest{ + Offset: 10, + Limit: 100, + } + clamped := query.ClampPageRequestLimit(pageReq) + s.Require().Equal(uint64(100), clamped.Limit) + + s.T().Log("verify nil input handling") + clamped = query.ClampPageRequestLimit(nil) + s.Require().Equal(uint64(0), clamped.Offset) + s.Require().Equal(uint64(0), clamped.Limit) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
types/query/pagination.go(2 hunks)types/query/pagination_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
types/query/pagination.go (2)
163-174: LGTM! Mathematically sound overflow prevention.The
ClampPageRequestLimitfunction correctly prevents uint64 overflow by checking ifLimit > PaginationMaxLimit - Offsetand clamping the limit accordingly. The implementation properly:
- Handles nil input gracefully
- Creates a copy to avoid mutation
- Uses correct overflow detection logic
- Sets the safe maximum limit
58-58: Proper integration of overflow protection.The placement of
ClampPageRequestLimitafterinitPageRequestDefaultsis correct, ensuring defaults are established before applying overflow protection.
Head branch was pushed to by a user without write access
|
Hi @aljo242, I've just pushed the missing CHANGELOG.md entry that was causing the previous CI check to fail. Would it be possible for you to try merging it again? |
|
could you please review this PR when you have a chance? |
Description
Closes: #25006
This PR fixes an integer overflow in the RPC pagination logic that was causing empty results when
offset + limitexceeded the maximumuint64.ClampPageRequestLimit(offset, limit) uint64that capslimitatMaxUint64 - offset.ClampPageRequestLimitdirectly intoPaginate, so every pagination call now applies the clamp before slicing.types/query/pagination_test.go(TestClampPageRequestLimit) to verify that overly large limits are correctly reduced.Tests
go test ./types/querylocally; all existing tests pass.TestClampPageRequestLimitcovering the new clamp behavior.Summary by CodeRabbit
Bug Fixes
Tests