Check for overflow in Script::bytes_to_asm_fmt() #658
Conversation
This adds an overflow check in `Script::bytes_to_asm_fmt()` motivated by `electrs` issue. While it was not tested yet, I'm very confident that overflow is the cause of panic there and even if not it can cause panic becuase the public function takes unvalidated byte array and reads `data_len` from it. The `electrs` issue: romanz/electrs#490
Kixunil
force-pushed
the
script-fmt-overflow-fix
branch
from
c01f9ed
to
a0e1d2e
Compare
This adds a test case for script formatting which caused overflow in the past and a few others from the same "interesting" transaction. Note that to trigger the bug one has to run the test on 32 bit architecture.
|
We found the offending transaction and I added a test cases for its outputs and checked that old code causes them to fail because of overflow. Now I'm certain this was actually a bug and it was the bug that caused electrs issue. |
This adds i686 to CI which can help catching pointer-size-related bugs such as the one addressed by rust-bitcoin#658.
This adds i686 to CI which can help catching pointer-size-related bugs such as the one addressed by rust-bitcoin#658.
This adds i686 to CI which can help catching pointer-size-related bugs such as the one addressed by rust-bitcoin#658.
|
Note that I noticed a few more overflows that I think are unlikely to happen in practice but intend to send a PR later. |
This adds i686 to CI which can help catching pointer-size-related bugs such as the one addressed by rust-bitcoin#658.
|
Great idea! Though it'd be nice if this got fixed quickly, so not sure if worth dismissing a review. Also forgot to ask if we can get a point release with this fix (and possibly others). |
|
Yea, seems like a good thing to put in a new PR, doesn’t have to be here. |
There was a problem hiding this comment.
Found one more: n + 5 can overflow here too!
rust-bitcoin/src/blockdata/script.rs
Line 675 in b7f9849
You are welcome to fix this in this PR, otherwise, I can make another PR for it.
ACK a0e1d2e
|
Yes, that's one of them - each match harm has such. I was actually looking into converting iteration to iterator, systematically removing all overflows and merging repeated code via macro (not fn due to Anyway, since this has two approvals and it'd be really nice to get it into electrs could this be merged and released in point version, please? |
|
We can certainly make a release with this PR, and without any taproot-related breaking changes that have been merged in master. |
|
ACK 76cf74f |
…m_fmt() 6c3434c bump version to 0.27.1 (Andrew Poelstra) 8a529e6 Added test for the overflow bug and few others (Martin Habovstiak) 78b152e Check for overflow in Script::bytes_to_asm_fmt() (Martin Habovstiak) Pull request description: Backport of #658 ACKs for top commit: apoelstra: ACK 6c3434c sanket1729: ACK 6c3434c. Same as #658 which I ACKed Tree-SHA512: ad9e02e2c748467b351039c3ab7f23b9902507cfa45d7d1084bfbaaad1ff7a1f22327b7311849f18a1a03dfd354a9424a74b125a2f412b9f6f678979a037df0a
f2042bd Add i686 to tested architectures (Martin Habovstiak) Pull request description: This adds i686 to CI which can help catching pointer-size-related bugs such as the one addressed by #658. Opening a new PR seemed more appropriate than adding it to #658 I can change it if you disagree. ACKs for top commit: apoelstra: ACK f2042bd Tree-SHA512: b772c8cbc5faa6fed25faed275eb903cd7226d8ff618ff57ce3e6735cc53b5c797380a519c670faf4fd5aa86ae01d5232e8c21e03a282599e7caa532b2176be8
This adds an overflow check in
Script::bytes_to_asm_fmt()motivated byelectrsissue. While it was not tested yet, I'm very confident thatoverflow is the cause of panic there and even if not it can cause panic
becuase the public function takes unvalidated byte array and reads
data_lenfrom it.The
electrsissue: romanz/electrs#490Strangely, this breaks a test case and I can't see why. I'm publishing in case someone wants to help.Edit: One damn character. :D Should be OK now.