Fix assumeutxo crash due to missing base_blockhash #21582

maflcko · 2021-04-03T13:27:32Z

This fixes an UB (which results in a crash with sanitizers enabled). Can be reproduced by cherry-picking the test without the other code changes. The fix:

Adds an Assert to transform the UB into a clean crash, even when sanitizers are disabled
Adds an early-fail condition to avoid the crash

maflcko · 2021-04-03T13:30:45Z

Introduced in #19806

jamesob · 2021-04-03T13:37:36Z

Thanks for catching this. Why change behavior not to allow waiting for the headers to load?

maflcko · 2021-04-03T14:03:17Z

I couldn't find a code path that needs this behavior, so it seemed odd to keep the dead code. Note that in your parent PR the RPC does the waiting, so it doesn't have to be done again in validation.

Edit: See max_secs_to_wait_for_headers in commit 1cf4521

Ouch555

Remove authorizationToken

jamesob · 2021-04-03T14:12:25Z

Note that in your parent PR the RPC does the waiting, so it doesn't have to be done again in validation.

Oh good point. I guess if we add some flag that allows specifying a snapshot to load (absent RPC) we'll have to add some similar code to init, but we can cross that bridge when we come to it.

In any case, concept ACK. Will test later today.

maflcko · 2021-04-03T14:25:33Z

we can cross that bridge when we come to it

Indeed, I was never a fan of the "idle-loop" inside validation (See #19806 (comment)), so moving it to the caller (if needed at all) seems the way to go.

ryanofsky

Code review ACK fafcddc. All changes look good, but I don't know when the bug happens in practice or how it was discovered.

Also it would be nice to have just the minimal fix and test together in same commit so it is more clear how the bug can be triggered and how it is fixed. Then it would be good to have the other cleanups in separate commits. IIUC:

The bugfix is the new if (!snapshot_start_block) check in PopulateAndValidateSnapshot, triggered by the new test passing base hash uint256::ONE.
Assert in GetUTXOStats adds an additional check at the point where the segfault previously happened
Removing the header wait loop is dead code removal and bit of a design change (good change breaking up a big function)

maflcko · 2021-04-04T05:40:54Z

separate commits

Thanks, force pushed so that the Assert is a separate commit, no code changes.

DrahtBot · 2021-04-04T06:47:31Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Fix assumeutxo crash due to invalid base_blockhash #21584 (Fix assumeutxo crash due to invalid base_blockhash by MarcoFalke)
Coinstats Index #19521 (Coinstats Index by fjahr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jamesob

ACK fa9b74f (jamesob/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due)

Looks good, thanks.

Show signature data

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK fa9b74f5ea89624e052934c48391b5076a87ffef ([`jamesob/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due))

Looks good, thanks.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmBrT/QACgkQepNdrbLE
TwULqw//SIwBkIK7OsgTB1ycHuwVJnJqnbO67v0oG/KI/2483t7jBJvMCznX2ecT
at4FcfJ7HmyrWve4EKz5mpa7851s565ftUNMy+K6nGj53tr+ejXyioDmEamol/AT
KzYah/LQjtwdS/WbNs1JyXE3NQ0briscUhi6053tdYGDwmtc+bdh4wdkcySRWHFR
a7SLoASpQ4rj3c2VlSyDzXoUCLNtpvylO9aRxgINC/IOU8UKS/F51LjMIGmeNHHP
oDDJ8cZwQURfSqg8bIQWpVDFwGBhPdXAFrQdl2kBdqJa48yp4PEg7KIHkh+H3xKV
W4sn2xMghzwB+MtFOCUDTY6ggy7yuh/6w/ZLqBuwrQaZyepMu3OUJ8PnEH8mk4OV
ddVL43/iGoEf9lH+X3f6WykZFIDTeMy1Np0YGjrDcFzc3qxDpQip++dvcm3rblDP
BlaUUm1UucSU8ZafZZJpv5Jg0EpbPaNlAMb8qp1N73AD94ngYq/LIlgahu280obM
v7lAxApGzUpufGl0Fg5ARqvKPWpwEuS8FoM0EUkv8wNaJeNxZAuUgIJ3R8PmoffH
ffAgxlNS5Q6o3aAPsTin9hSR7Bfkzit4/Tm9xMHiH+WCHJPiML4mlO4h5DbSUjYn
dyxgaI1e32gJBrY/xroaiu8v+EmyJeHeuDlcbN+lP7AzhDN4XfU=
=Pv7n
-----END PGP SIGNATURE-----

Show platform data

Tested on Linux-4.19.0-10-amd64-x86_64-with-glibc2.28

Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ -L/home/james/.local/lib CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ -I/home/james/.local/include CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default --enable-multiprocess PKG_CONFIG_PATH=/home/james/.local/lib/pkgconfig CXX=/usr/bin/clang++ CC=/usr/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion

Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv  -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2  i

Compiler version: clang version 7.0.1-8 (tags/RELEASE_701/final)

ryanofsky

Code review ACK fa9b74f with no code changes since last review, just splitting up combocommit a little.

This fixes an UB (which results in a crash with sanitizers enabled). Can be reproduced by cherry-picking the test without the other code changes.

Look good, though I'm curious if the crash was happening before the new test. Was the bug causing a different crash in another test? Or was it just discovered reading code?

maflcko · 2021-04-06T06:13:46Z

was it just discovered reading code?

This particular one was discovered by a fuzz target, which I wrote after I discovered a crash with code review: #21585

fa9b74f Fix assumeutxo crash due to missing base_blockhash (MarcoFalke) fa8fffe refactor: Prefer clean assert over UB in coinstats (MarcoFalke) Pull request description: This fixes an UB (which results in a crash with sanitizers enabled). Can be reproduced by cherry-picking the test without the other code changes. The fix: * Adds an `Assert` to transform the UB into a clean crash, even when sanitizers are disabled * Adds an early-fail condition to avoid the crash ACKs for top commit: jamesob: ACK fa9b74f ([`jamesob/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due)) ryanofsky: Code review ACK fa9b74f with no code changes since last review, just splitting up combocommit a little. Tree-SHA512: dd36808a09f49c647543a9eaa6fdb785b3f1109af48ba4cc983153b22a144da9ca61af22034dcfaa0e192a65b1ee7de744f187555079aff55bec0efa0ce87cd4

practicalswift · 2021-04-09T16:05:46Z

Post merge cr ACK fa9b74f

was it just discovered reading code?

This particular one was discovered by a fuzz target, which I wrote after I discovered a crash with code review: #21585

Nice fuzzing find! Fuzzing makes us: harder better faster^Wresilient stronger! 🎶

Don't forget to add your find to the growing list of trophies over at https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Fuzz-Trophies :)

Summary: bitcoin/bitcoin@769a1ef > test: Add tests with maleated snapshot data ---- https://github.com/bitcoin/bitcoin/pull/21681/files > validation: fix ActivateSnapshot to use hardcoded nChainTx > validation: remove nchaintx from assumeutxo metadata Only test related changes for these commits are in this revision ---- bitcoin/bitcoin@fa8fffe > refactor: Prefer clean assert over UB in coinstats ---- bitcoin/bitcoin@fa9b74f > Fix assumeutxo crash due to missing base_blockhash Only test related changes from this commit are in this revision. ---- bitcoin/bitcoin@fae33f9 > Fix assumeutxo crash due to invalid base_blockhash Only test related changes from this commit are in this revision. ---- This is a backport of [[bitcoin/bitcoin#19806 | core#19806]] [7/8] and test related changes from [[bitcoin/bitcoin#21681 | core#21681]], [[bitcoin/bitcoin#21582 | core#21582]] and [[bitcoin/bitcoin#21584 | core#21584]] Depends on D11240 Test Plan: `ninja check` With UBSAN: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11241

Ouch555 suggested changes Apr 3, 2021

View reviewed changes

maflcko force-pushed the 2104-assumeutxoCrash01 branch from fa64415 to fafcddc Compare April 3, 2021 14:10

maflcko mentioned this pull request Apr 3, 2021

Fix assumeutxo crash due to invalid base_blockhash #21584

Merged

DrahtBot added the Validation label Apr 3, 2021

ryanofsky approved these changes Apr 3, 2021

View reviewed changes

MarcoFalke added 2 commits April 4, 2021 07:37

refactor: Prefer clean assert over UB in coinstats

fa8fffe

Fix assumeutxo crash due to missing base_blockhash

fa9b74f

maflcko force-pushed the 2104-assumeutxoCrash01 branch from fafcddc to fa9b74f Compare April 4, 2021 05:38

jamesob approved these changes Apr 5, 2021

View reviewed changes

ryanofsky approved these changes Apr 6, 2021

View reviewed changes

DrahtBot mentioned this pull request Apr 6, 2021

Coinstats Index #19521

Merged

maflcko merged commit 41a8d2b into bitcoin:master Apr 7, 2021

maflcko deleted the 2104-assumeutxoCrash01 branch April 7, 2021 05:38

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix assumeutxo crash due to missing base_blockhash #21582

Fix assumeutxo crash due to missing base_blockhash #21582

Uh oh!

maflcko commented Apr 3, 2021

Uh oh!

maflcko commented Apr 3, 2021

Uh oh!

jamesob commented Apr 3, 2021

Uh oh!

maflcko commented Apr 3, 2021 •

edited

Loading

Uh oh!

Ouch555 left a comment

Uh oh!

jamesob commented Apr 3, 2021

Uh oh!

maflcko commented Apr 3, 2021

Uh oh!

ryanofsky left a comment

Uh oh!

maflcko commented Apr 4, 2021

Uh oh!

DrahtBot commented Apr 4, 2021 •

edited

Loading

Uh oh!

jamesob left a comment

Uh oh!

ryanofsky left a comment

Uh oh!

maflcko commented Apr 6, 2021

Uh oh!

practicalswift commented Apr 9, 2021

Uh oh!

Uh oh!

Fix assumeutxo crash due to missing base_blockhash #21582

Fix assumeutxo crash due to missing base_blockhash #21582

Uh oh!

Conversation

maflcko commented Apr 3, 2021

Uh oh!

maflcko commented Apr 3, 2021

Uh oh!

jamesob commented Apr 3, 2021

Uh oh!

maflcko commented Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ouch555 left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob commented Apr 3, 2021

Uh oh!

maflcko commented Apr 3, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 4, 2021

Uh oh!

DrahtBot commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 6, 2021

Uh oh!

practicalswift commented Apr 9, 2021

Uh oh!

Uh oh!

maflcko commented Apr 3, 2021 •

edited

Loading

DrahtBot commented Apr 4, 2021 •

edited

Loading