Fix UBSan warnings triggered when loading corrupt mempool.dat files

rajarshimaitra commented at 5:18 pm on June 25, 2020: contributor

This fixes some undefined behavior observed in the issue #19728.

The bugs can be reproduced by using corpus added here and the harness added in PR#19259

rajarshimaitra renamed this:
~~Fixes UB reported in issue #19278~~
tests: Fixes UB reported in issue #19278
on Jun 25, 2020

in src/validation.cpp:5090 in 8cc8548ace outdated

5086@@ -5087,7 +5087,9 @@ bool LoadMempool(CTxMemPool& pool)
5087         file >> mapDeltas;
5088 
5089         for (const auto& i : mapDeltas) {
5090-            pool.PrioritiseTransaction(i.first, i.second);
5091+            if(MoneyRange(i.second)){

jonatack commented at 5:30 pm on June 25, 2020:

0            if(MoneyRange(i.second)) {

rajarshimaitra commented at 12:56 pm on June 26, 2020:

Noted..

MarcoFalke commented at 6:52 pm on June 26, 2020:

You may install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to preempt whitespace nitpicking.

jonatack commented at 5:31 pm on June 25, 2020: member

Concept ACK, will review

DrahtBot added the label Tests on Jun 25, 2020

DrahtBot added the label Validation on Jun 25, 2020

practicalswift commented at 7:36 pm on June 25, 2020: contributor

Concept ACK: thanks for fixing! :)

In case of CAmount:s outside of the money range: should we perhaps let LoadMempool return false on those to indicate that something went (very) wrong (instead of just skipping PrioritiseTransaction)?

Update: Negative deltas are allowed!

MarcoFalke removed the label Tests on Jun 25, 2020

fanquake renamed this:
~~tests: Fixes UB reported in issue #19278~~
validation: Fixes UB reported in issue #19278
on Jun 26, 2020

rajarshimaitra commented at 2:38 pm on June 27, 2020: contributor

@practicalswift I was thinking along the same line. Also, It seems better to initiate shutdown once something like this happens. As just returning false in LoadMempool doesn’t seem to be triggering any catastrophe, and I am not sure what the node is doing about its mempool when loading from disk fails. The node seems to keep on running with just a failure message, My guess is its creates a fresh mempool and override the data in the disk. Which can seem fine, unless someone specifically wants a particular mempool and if the disk data is corrupted they might not notice that something is wrong.

So I made it to initiate shutdown if loadmempool fails. Not sure if that’s strictly necessary, looking for suggestions here before I update the PR.

The log

 02020-06-27T14:19:55Z Cant load mempool, something went wrong, initiating shutdown.
 12020-06-27T14:19:55Z Bound to [::]:18444
 22020-06-27T14:19:55Z Bound to 0.0.0.0:18444
 32020-06-27T14:19:55Z init message: Loading P2P addresses...
 42020-06-27T14:19:55Z Loaded 0 addresses from peers.dat  1ms
 52020-06-27T14:19:55Z init message: Starting network threads...
 62020-06-27T14:19:55Z net thread start
 72020-06-27T14:19:55Z dnsseed thread start
 82020-06-27T14:19:55Z 0 addresses found from DNS seeds
 92020-06-27T14:19:55Z dnsseed thread exit
102020-06-27T14:19:55Z init message: Done loading
112020-06-27T14:19:55Z addcon thread start
122020-06-27T14:19:55Z opencon thread start
132020-06-27T14:19:55Z torcontrol thread start
142020-06-27T14:19:55Z msghand thread start
152020-06-27T14:19:55Z tor: Thread interrupt
162020-06-27T14:19:55Z opencon thread exit
172020-06-27T14:19:55Z Shutdown: In progress...
182020-06-27T14:19:55Z torcontrol thread exit
192020-06-27T14:19:55Z addcon thread exit
202020-06-27T14:19:55Z net thread exit
212020-06-27T14:19:55Z msghand thread exit
222020-06-27T14:19:55Z scheduler thread exit
232020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
242020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
252020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
262020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
272020-06-27T14:19:55Z Shutdown: done

Also please give suggestions for a better error message.

practicalswift commented at 6:01 pm on June 28, 2020: contributor

~~@rajarshimaitra This is such an unlikely error that I don’t think we need to add any special logic beyond returning false in case of !MoneyRange(…) :)~~

Update: Negative deltas are allowed!

rajarshimaitra force-pushed on Jul 1, 2020

rajarshimaitra commented at 7:45 am on July 1, 2020: contributor

@practicalswift updated to return failure instead of skipping PrioritiseTransaction.

laanwj added the label Mempool on Jul 1, 2020

practicalswift commented at 4:38 pm on July 1, 2020: contributor

~~@rajarshimaitra Great! What about doing the same thing for the other PrioritiseTransaction call – that is return false in the case of !MoneyRange(i.second) :)~~

Update: Negative deltas are allowed!

rajarshimaitra commented at 3:13 pm on July 2, 2020: contributor

@practicalswift silly me. Should have done that. will push the same.

rajarshimaitra force-pushed on Jul 3, 2020

DrahtBot commented at 11:18 am on July 11, 2020: member

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

Conflicts

No conflicts as of last run.

in src/validation.cpp:5060 in f19dcd4e41 outdated

5054@@ -5055,11 +5055,13 @@ bool LoadMempool(CTxMemPool& pool)
5055             file >> nFeeDelta;
5056 
5057             CAmount amountdelta = nFeeDelta;
5058-            if (amountdelta) {
5059+            if (!MoneyRange(amountdelta)) {
5060+                return false;
5061+            }else {

practicalswift commented at 5:52 pm on July 11, 2020:

0            }else {
1            } else {

rajarshimaitra commented at 11:11 am on July 13, 2020:

Done

in src/validation.cpp:5092 in f19dcd4e41 outdated

5088@@ -5087,7 +5089,11 @@ bool LoadMempool(CTxMemPool& pool)
5089         file >> mapDeltas;
5090 
5091         for (const auto& i : mapDeltas) {
5092-            pool.PrioritiseTransaction(i.first, i.second);
5093+            if(!MoneyRange(i.second)) {

practicalswift commented at 5:53 pm on July 11, 2020:

0            if(!MoneyRange(i.second)) {
1            if (!MoneyRange(i.second)) {

rajarshimaitra commented at 11:11 am on July 13, 2020:

Done

practicalswift commented at 5:56 pm on July 11, 2020: contributor

ACK modulo nits

Could perhaps use a more descriptive PR title and commit message: what about something along the lines of “Fix UBSan warnings triggered when loading corrupt mempool.dat files”? :)

rajarshimaitra force-pushed on Jul 13, 2020

rajarshimaitra commented at 11:13 am on July 13, 2020: contributor

@practicalswift addressed all your comments and rebased.

practicalswift commented at 4:07 pm on July 13, 2020: contributor

@rajarshimaitra I think you still want if (amountdelta) { before doing pool.PrioritiseTransaction(tx->GetHash(), amountdelta); in order to not change the existing logic.

~~I suggest leaving the existing PrioritiseTransaction logic intact in both cases, and simply add two stand-alone MoneyRange checks:~~

Update: Negative deltas allowed.

Something along the lines of …

0if (!MoneyRange(amountdelta)) {
1    return false;
2}
3if (amountdelta) {
4    pool.PrioritiseTransaction(tx->GetHash(), amountdelta);
5}

… and …

0if (!MoneyRange(i.second)) {
1    return false;
2}
3pool.PrioritiseTransaction(i.first, i.second);

Also, would you mind also updating the PR title to the more descriptive commit message? :)~~

in src/validation.cpp:5085 in 069043ee4e outdated

5079@@ -5078,7 +5080,11 @@ bool LoadMempool(CTxMemPool& pool)
5080         file >> mapDeltas;
5081 
5082         for (const auto& i : mapDeltas) {
5083-            pool.PrioritiseTransaction(i.first, i.second);
5084+            if (!MoneyRange(i.second)) {
5085+                return false;
5086+            } else {

MarcoFalke commented at 4:45 pm on July 13, 2020:

nit: No need to change indentation on early-return. (same above)

rajarshimaitra commented at 4:56 am on July 17, 2020:

Sorry, I didn’t get it. Can you explain a bit what are you suggesting here?

practicalswift commented at 10:14 am on July 18, 2020:

This was fixed in your last push AFAICT :)

rajarshimaitra commented at 4:59 am on July 17, 2020: contributor

@practicalswift If i understand you correctly this is what you are suggesting?

0CAmount amountdelta = nFeeDelta;
1            if (!MoneyRange(amountdelta)) {
2                return false;
3            }
4            if (amountdelta) {
5                pool.PrioritiseTransaction(tx->GetHash(), amountdelta);
6            }

and

0for (const auto& i : mapDeltas) {
1            if (!MoneyRange(i.second)) {
2                return false;
3            }
4            if (i.second) {
5                pool.PrioritiseTransaction(i.first, i.second);
6            }
7        }

Let me know if this seems correct.

practicalswift commented at 8:04 am on July 17, 2020: contributor

@rajarshimaitra Yes for the first case, but for the second case pool.PrioritiseTransaction(i.first, i.second); should be done unconditionally (skip if (i.second)) in order to not change the existing logic :)

Update: Negative deltas are allowed!

Also would you mind changing the PR title to Fix UBSan warnings triggered when loading corrupt mempool.dat files?

rajarshimaitra renamed this:
~~validation: Fixes UB reported in issue #19278~~
Fix UBSan warnings triggered when loading corrupt mempool.dat files
on Jul 18, 2020

Fix UBSan warnings triggered when loading corrupt mempool.dat files 2cc2cbde13

rajarshimaitra force-pushed on Jul 18, 2020

rajarshimaitra commented at 4:57 am on July 18, 2020: contributor

Fixed and updated.

practicalswift commented at 10:14 am on July 18, 2020: contributor

ACK XXX

Update: Negative deltas are allowed!

Restarted Travis which failed spuriously :)

fjahr commented at 6:52 pm on July 21, 2020: member

Code review ACK 2cc2cbde13072a2d244912aba7df8a97c81646a1

in src/validation.cpp:5051 in 2cc2cbde13

5045@@ -5046,11 +5046,14 @@ bool LoadMempool(CTxMemPool& pool)
5046             file >> nFeeDelta;
5047 
5048             CAmount amountdelta = nFeeDelta;
5049+            if (!MoneyRange(amountdelta)) {
5050+                return false;
5051+            }

luke-jr commented at 11:09 pm on July 23, 2020:

There is no requirement for deltas to be in MoneyRange… Better use a numeric_limit max

rajarshimaitra commented at 7:06 am on July 24, 2020:

Before I push the commit does this look ok?

0CAmount amountdelta = nFeeDelta;
1            if (amountdelta > numeric_limits<int64_t>::max() || amountdelta < numeric_limits<int64_t>::min())  {
2                return false;
3            }

practicalswift commented at 3:43 pm on July 25, 2020:

@luke-jr Oh, crap thanks for alerting about that! I naïvely assumed that the bounds of CAmount were defined by MoneyRange(…), but I now understand that this is a case where negative amounts are allowed for a CAmount. I assume the valid range here is [-MAX_MONEY, MAX_MONEY] then?

Should CAmount perhaps be reserved for cases where MoneyRange(…) defines the bounds? @rajarshimaitra I don’t think that is what @luke-jr is suggesting: note that CAmount is int64_t so amountdelta is guaranteed to be in the range you’re checking for in the suggested diff ([std:: numeric_limits<int64_t>::min(), std:: numeric_limits<int64_t>::max()]) :)

rajarshimaitra commented at 6:16 am on July 26, 2020:

Oh, right. Well, this sounds quite deep for my grasp and I wouldn’t know what exactly to do. Do suggest and will update the PR accordingly.

luke-jr changes_requested

practicalswift commented at 6:48 pm on August 18, 2020: contributor

@rajarshimaitra To allow for negative deltas: what about replacing the MoneyRange(…) usages in this PR with IsValidFeeDelta(…) which could be something along the lines of:

0NODISCARD bool IsValidFeeDelta(const CAmount fee_delta) {
1    return fee_delta >= -MAX_MONEY && fee_delta <= MAX_MONEY;
2}

DrahtBot commented at 2:06 pm on February 11, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Feb 11, 2021

in src/validation.cpp:5056 in 2cc2cbde13

5052             if (amountdelta) {
5053                 pool.PrioritiseTransaction(tx->GetHash(), amountdelta);
5054             }
5055             TxValidationState state;
5056-            if (nTime + nExpiryTimeout > nNow) {
5057+            if (nTime > nNow - nExpiryTimeout) {

MarcoFalke commented at 10:36 am on May 10, 2021:

Already in ee11a412a537f62aa46e8862678ce2069a2df5b7

MarcoFalke commented at 10:38 am on May 10, 2021: member

Part of this patch is already in ee11a412a537f62aa46e8862678ce2069a2df5b7. The other part I am not sure it fixes the issue. See also #20383 (comment)

MarcoFalke commented at 10:38 am on May 10, 2021: member

Closing due to inactivity

MarcoFalke closed this on May 10, 2021

DrahtBot locked this on Aug 18, 2022

Fix UBSan warnings triggered when loading corrupt mempool.dat files #19381

Conflicts