fix(claimsmanager): check index to avoid runtime panic #1223
Conversation
minhngoc274
requested review from
joe-bowman,
faddat and
tuantran1702
as code owners
WalkthroughThe update in the Changes
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/claimsmanager/keeper/grpc_query.go (2 hunks)
Additional comments: 2
x/claimsmanager/keeper/grpc_query.go (2)
- 74-74: The addition of the index check
idx >= 0before accessing the slice using the index is a crucial improvement. This change effectively prevents potential runtime panics that could occur from out-of-bounds slice access when the key does not contain the expected byte structure. This is a direct and effective solution to the issue described in the PR objectives and enhances the robustness of the code by ensuring that the index is valid before proceeding with byte comparison operations.- 91-91: Similar to the change in the
UserClaimsfunction, the addition of the index checkidx >= 0in theUserLastEpochClaimsfunction is an essential improvement. It ensures that the code does not attempt to access a slice using an invalid index, thereby preventing runtime panics and potential data corruption. This change aligns with the PR's objective to enhance the module's robustness by handling unexpected key structures gracefully.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1223 +/- ##
=======================================
Coverage 63.16% 63.16%
=======================================
Files 171 171
Lines 11261 11261
=======================================
Hits 7113 7113
Misses 3425 3425
Partials 723 723
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Am not sure though that this change is the right fix, I had prescribed that we check the value of bytes.Index not of bytes.Index + 1 + 1 |
| @@ -71,7 +71,7 @@ func (k Keeper) UserClaims(c context.Context, q *types.QueryClaimsRequest) (*typ | |||
| // check for the presence of the addr bytes in the key. | |||
| // first prefix byte is 0x00; so cater for that! Then + 1 to skip the separator. | |||
| idx := bytes.Index(key[1:], []byte{0x00}) + 1 + 1 | |||
There was a problem hiding this comment.
We need to do the check here firstly before adding + 1 + 1
idx := bytes.Index(key[1:], []byte{0x00})
if idx >= 0 {
idx += 1 + 1 // Cater to skip the separator.
if bytes.Equal(key[idx:idx+len(addrBytes)], addrBytes) {
out = append(out, claim)
}
}
…rClaims,UserLastEpochClaims This change ensures that we correctly check for missing values in keeper.(UserClaims, UserLastEpochClaims) because otherwise due to the blind assumption that the key would always be well formed, +1 and +1+1 were being added respectively which would mean that the condition "idx >= 0" would ALWAYS pass and send over the wrong data if the key happened to contain the address bytes. Fixes quicksilver-zone#1217 Supersedes PR quicksilver-zone#1223
…rClaims,UserLastEpochClaims This change ensures that we correctly check for missing values in keeper.(UserClaims, UserLastEpochClaims) because otherwise due to the blind assumption that the key would always be well formed, +1 and +1+1 were being added respectively which would mean that the condition "idx >= 0" would ALWAYS pass and send over the wrong data if the key happened to contain the address bytes. Fixes quicksilver-zone#1217 Supersedes PR quicksilver-zone#1223
…rClaims,UserLastEpochClaims (#1234) This change ensures that we correctly check for missing values in keeper.(UserClaims, UserLastEpochClaims) because otherwise due to the blind assumption that the key would always be well formed, +1 and +1+1 were being added respectively which would mean that the condition "idx >= 0" would ALWAYS pass and send over the wrong data if the key happened to contain the address bytes. Fixes #1217 Supersedes PR #1223
1. Summary
Fixes #1217
2.Type of change
3. Implementation details
4. How to test/use
5. Checklist
6. Limitations (optional)
7. Future Work (optional)
Summary by CodeRabbit