-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fix unaligned read of short_vec pubkey_size in sigverify #6388
Fix unaligned read of short_vec pubkey_size in sigverify #6388
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6388 +/- ##
========================================
+ Coverage 72.7% 77.4% +4.7%
========================================
Files 219 209 -10
Lines 45244 40004 -5240
========================================
- Hits 32901 30972 -1929
+ Misses 12343 9032 -3311 |
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.
One nit, but lgtm
core/src/sigverify.rs
Outdated
|
||
let message = Message { | ||
header: MessageHeader { | ||
num_required_signatures: required_num_sigs as u8, |
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.
Don't need this cast
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.
nice nits! 082fd71
Pull request has been modified.
yay! |
@ryoqun thanks! |
Yes looks good. Thanks for splitting it out into this commit, it was much easier to understand the change. |
(This is split from #6236 as requested by @sakridge !)
Problem
sigverify
's deserialization is wrong in a corner case; most of time it works as intended.From https://github.com/solana-labs/solana/pull/6236/files#r333704395
Solution
Shift the offset correctly, taking
size_of
MessageHeader
into account.