-
Notifications
You must be signed in to change notification settings - Fork 704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sp-trie: minor fix to avoid possible panic during node decoding #6486
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
721dd2e
Fix possible panic in trie node decode
TDemeco 34f3d40
Minor bump to `sp-trie` version
TDemeco 1e12ba1
fmt with nightly
TDemeco 99db422
Add prdoc
TDemeco 59b1475
Update substrate/primitives/trie/Cargo.toml
TDemeco d37fc98
update Cargo.lock after reverting `sp-trie` version bump
TDemeco 72abbb0
Merge branch 'master' into master
TDemeco File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
title: "sp-trie: minor fix to avoid panic on badly-constructed proof" | ||
|
||
doc: | ||
- audience: ["Runtime Dev", "Runtime User"] | ||
description: | | ||
"Added a check when decoding encoded proof nodes in `sp-trie` to avoid panicking when receiving a badly constructed proof, instead erroring out." | ||
|
||
crates: | ||
- name: sp-trie | ||
bump: patch |
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative would be to do the check with let
offset = input.take(1)?.start
, then use offset instead of input.offset. (not exposing input.offset).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @cheme! Thanks for the feedback. I like your alternative solution but the reason I went for a simple
if
clause was becausetake
increments theoffset
ofinput
, so a big part of the code would have to be refactored to adapt to this which might not be worth it for a simple fix like this.Nevertheless, I made a mistake in this PR since the correct check would be
data.len() < input.offset + 1
to make suredata[input.offset]
does not panic 😅 I realized after testing and this PR was sadly already merged so I'm a bit ashamed, sorry about that. I already opened another PR (#6502) that fixes thisThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, would have needed a
peak
method (reader is implemented to be usable with scale api, iirc there another std trait that expose peak), but really not worth it.