electrs: Bug: 0.9.0-rc1 causes `rust-bitcoin` to panic when logging a non-standard script on 32-bit platforms

To add, it’s not a huge issue, just don’t trace on 32 bit architectures and you’re fine.

Yes 😃 The bug was in rust-bitcoin Debug formatting of non-standard scripts: https://github.com/romanz/electrs/issues/490#issuecomment-922496191

FTR I just realized why debug-assertions didn’t help. Overflow checking actually uses a different option to set it: overflow-checks = true. @dematerializer if you happen to enjoy recompiling you can try it but I don’t see a good reason to think it was something else at this point. 😃

No worries, I’m going to test it differently anyway - by adding a test case into rust-bitcoin library and compiling/running for 32-bit architecture. This will be useful to avoid regressions.

The test case is:

assert_eq!(hex_script!("4effffffff01").asm(), " OP_PUSHDATA4 <push past end>");

Added to the script fmt tests.

@Kixunil

I’m very confident it’d confirm my hypothesis and thus become a strong argument for my PR. 😃 However, it’d require rebuild of whole electrs (long time)

I rebuilt electrs with debug assertions enabled, did not take that long. Now here’s the output of the re-run: electrs-panic-with-assertions.log

However, it’d require rebuild of whole electrs (long time), so if you don’t want to do that you can try my fix - add to Cargo.toml this instead of debug-assertions

And here is the result of the re-run of electrs built with your patch: it creates massive debug output but does not seem to fail.

I believe it’s caused by transaction ebc9fa1196a59e192352d76c0f6e73167046b9d37b8302b6bb6968dfd279b767 - bitcoin-cli prints [error] for it’s scriptPubkey

Sounds good, I plan to do another RC in a ~week 😃

Good to have a workaround prepared, maybe wait a bit and see if we can get point release of bitcoin and use that instead?

Yep, the test fails in case of version before my fix and passes after my fix. I’m now certain my fix is correct. 😃

@romanz indeed, if you look at those outputs it seems like someone was testing weird edge cases, the last one contains a large push: 4effffffff01, ffffffff is usize::MAX on 32-bit machines, so 64 bits will correctly compute index + data_let and then reject it because of it being too long while on 32 bits it overflows and causes it to be computed as index - 1 which is not detected as longer than script leading to a panic later.

Thank God this was not C code, would be an ugly vulnerability.

@dematerializer that’s weird the panic message doesn’t confirm overflow but the fix works anyway. Did cargo write Compiling bitcoin when you recompiled?

The block contents looks correct:

$ bitcoin-cli getblock 000000000000000b7e48f88e86ceee3e97b4df7c139f5411d14735c1b3c36791 0 | sha256sum 
3286900c88b2648048d89487621b5736737cbff7798946a541668475173bda3f  -
$ sha256sum block_265458.txt 
3286900c88b2648048d89487621b5736737cbff7798946a541668475173bda3f  block_265458.txt

Here comes the block: block_265458.txt Do you still need me to rebuild and run electrs with debug assertions enabled?

IIUC, data_len has to be “-1” for this to happen

Or usize::MAX so that it overflows. This would also explain why it only happens in case of 32-bit systems. I’m not sure how such script would ever get into the chain but I guess it could be in an output script (burning the coins in the process).

To confirm this we should test with debug assertions. @dematerializer could you modify Cargo.toml in electrs directory and add:

[profile.release]
debug-assertions = true

recompile and run again?

Although it’d still be nice to get exact transaction causing this so we can add it as a test case for rust-bitcoin in the future.

No, I haven’t seen it, but I only use 64bit builds (arm64) in Docker.