refactor: improve edge case handling for recursion limits #22988
Conversation
|
Warning Rate limit exceeded@haiyizxx has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 16 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several enhancements to the Cosmos SDK, focusing on improving functionality and error handling. Key changes include simulating nested messages, supporting hex key imports via standard input, and updating the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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 (3)
CHANGELOG.md (3)
Line range hint
1-1: Add title and description at the top of changelogThe changelog would benefit from having a clear title and brief description at the top explaining its purpose and how to read/interpret the entries.
+# Changelog + +All notable changes to the Cosmos SDK will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +
Line range hint
1-2000: Ensure consistent formatting of PR/Issue referencesThe changelog has inconsistent formatting of PR/Issue references - some use full URLs while others use just numbers. Consider standardizing on one format.
For example, standardize all references to use the short form
#1234rather than mixing with full URLs likehttps://github.com/cosmos/cosmos-sdk/pull/1234.
Line range hint
1-2000: Add release dates for all versionsSome version entries are missing their release dates. For historical tracking, all version headers should include their release date.
For example:
-## [v0.44.0] +## [v0.44.0] - 2021-01-08
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)codec/types/interface_registry.go(1 hunks)codec/unknownproto/unknown_fields.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
codec/types/interface_registry.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
codec/unknownproto/unknown_fields.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (3)
codec/types/interface_registry.go (2)
280-282: LGTM: Enhanced validation for maxCalls
The change properly handles edge cases by rejecting both zero and negative values for the call counter, maintaining consistency with the maxDepth check.
277-279: LGTM: Improved validation for maxDepth
The change strengthens the validation by rejecting both zero and negative values for maxDepth, which is a security improvement.
Let's verify there are no direct manipulations of maxDepth that could set it to negative values:
✅ Verification successful
Validation change for maxDepth is safe and well-implemented
The verification shows that maxDepth is safely managed:
- Initialized with constant
MaxUnpackAnyRecursionDepth = 10 - Only decremented during recursion via
cloneForRecursion() - No direct assignments that could set negative values
- Used in a controlled manner within the
statefulUnpackerstruct
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any assignments to maxDepth
rg -p "maxDepth\s*=[^=]" --type go
Length of output: 36
Script:
#!/bin/bash
# Let's try a broader search for maxDepth usage and initialization
rg -p "maxDepth" --type go -C 2
# Also search for the struct field declaration
ast-grep --pattern 'type $_ struct {
$$$
maxDepth $_
$$$
}'
Length of output: 1326
Script:
#!/bin/bash
# Let's check the MaxUnpackAnyRecursionDepth constant value and usage
rg -p "MaxUnpackAnyRecursionDepth" --type go -C 2
# Also check if there are any other places where maxDepth is modified
ast-grep --pattern '$_.maxDepth = $_'
Length of output: 525
codec/unknownproto/unknown_fields.go (1)
57-59: LGTM: Consistent improvement in recursion limit validation
The change aligns with the modifications in interface_registry.go and properly handles negative recursion limits, enhancing security.
Let's verify there are no places where recursionLimit could be set to negative values:
✅ Verification successful
LGTM: Recursion limit validation is properly implemented
The codebase search confirms that recursionLimit is:
- Only used as a function parameter in message processing functions
- Properly decremented (
recursionLimit-1) when processing nested messages - Consistently checked with the new
<= 0condition - No direct assignments that could set it to negative values
The change is safe and improves the handling of invalid recursion limits.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for assignments to recursionLimit
rg -p "recursionLimit\s*=[^=]" --type go
Length of output: 42
Script:
#!/bin/bash
# Let's try a broader search for recursionLimit usage and declarations
rg -p "recursionLimit" --type go -C 2
# Also search for any function parameters or struct fields named recursionLimit
ast-grep --pattern 'struct {
$$$
recursionLimit $_
$$$
}'
ast-grep --pattern 'func $_($$ recursionLimit $_) $_'
Length of output: 2095
|
👍 Can you update https://github.com/cosmos/cosmos-sdk/blob/main/x/tx/decode/unknown.go#L50 as well? |
julienrbrt
added
the
backport/v0.52.x
haiyizxx
requested review from
kocubinski,
testinginprod and
aaronc
as code owners
haiyizxx
changed the title
updated, thanks |
Description
Previously, checks for maxDepth and maxCalls.count only triggered when their values were strictly 0, which could lead to unintended behavior or bypassed validations if the surrounding code changes later on.
This PR updates the conditions to trigger for any non-positive values (<= 0), ensuring proper handling of edge cases even if validation was bypassed during one recursion step.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
client.tomland a helper method for broadcasting test transactions.Improvements
Bug Fixes
API Breaking Changes
testutil/networkpackage and client prompt validations.