Fix UBSan warnings triggered when loading corrupt mempool.dat files by rajarshimaitra · Pull Request #19381 · bitcoin/bitcoin

rajarshimaitra · 2020-06-25T17:18:24Z

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

jonatack

Concept ACK, will review

src/validation.cpp

practicalswift · 2020-06-25T19:36:02Z

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!

rajarshimaitra · 2020-06-27T14:38:51Z

@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

2020-06-27T14:19:55Z Cant load mempool, something went wrong, initiating shutdown.
2020-06-27T14:19:55Z Bound to [::]:18444
2020-06-27T14:19:55Z Bound to 0.0.0.0:18444
2020-06-27T14:19:55Z init message: Loading P2P addresses...
2020-06-27T14:19:55Z Loaded 0 addresses from peers.dat  1ms
2020-06-27T14:19:55Z init message: Starting network threads...
2020-06-27T14:19:55Z net thread start
2020-06-27T14:19:55Z dnsseed thread start
2020-06-27T14:19:55Z 0 addresses found from DNS seeds
2020-06-27T14:19:55Z dnsseed thread exit
2020-06-27T14:19:55Z init message: Done loading
2020-06-27T14:19:55Z addcon thread start
2020-06-27T14:19:55Z opencon thread start
2020-06-27T14:19:55Z torcontrol thread start
2020-06-27T14:19:55Z msghand thread start
2020-06-27T14:19:55Z tor: Thread interrupt
2020-06-27T14:19:55Z opencon thread exit
2020-06-27T14:19:55Z Shutdown: In progress...
2020-06-27T14:19:55Z torcontrol thread exit
2020-06-27T14:19:55Z addcon thread exit
2020-06-27T14:19:55Z net thread exit
2020-06-27T14:19:55Z msghand thread exit
2020-06-27T14:19:55Z scheduler thread exit
2020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
2020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
2020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
2020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
2020-06-27T14:19:55Z Shutdown: done

Also please give suggestions for a better error message.

practicalswift · 2020-06-28T18:01:01Z

~~@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 · 2020-07-01T07:45:17Z

@practicalswift updated to return failure instead of skipping PrioritiseTransaction.

practicalswift · 2020-07-01T16:38:35Z

~~@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 · 2020-07-02T15:13:56Z

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

DrahtBot · 2020-07-11T11:18:12Z

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

Conflicts

No conflicts as of last run.

practicalswift

~~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"? :)

src/validation.cpp

rajarshimaitra · 2020-07-13T11:13:08Z

@practicalswift addressed all your comments and rebased.

practicalswift · 2020-07-13T16:07:29Z

@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 …

if (!MoneyRange(amountdelta)) {
    return false;
}
if (amountdelta) {
    pool.PrioritiseTransaction(tx->GetHash(), amountdelta);
}

... and ...

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

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

maflcko · 2020-07-13T16:45:09Z

src/validation.cpp

Suggested change

} else {

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

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

This was fixed in your last push AFAICT :)

rajarshimaitra · 2020-07-17T04:59:38Z

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

CAmount amountdelta = nFeeDelta;
            if (!MoneyRange(amountdelta)) {
                return false;
            }
            if (amountdelta) {
                pool.PrioritiseTransaction(tx->GetHash(), amountdelta);
            }

and

for (const auto& i : mapDeltas) {
            if (!MoneyRange(i.second)) {
                return false;
            }
            if (i.second) {
                pool.PrioritiseTransaction(i.first, i.second);
            }
        }

Let me know if this seems correct.

practicalswift · 2020-07-17T08:04:05Z

@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 · 2020-07-18T04:57:04Z

Fixed and updated.

practicalswift · 2020-07-18T10:14:26Z

~~ACK XXX~~

Update: Negative deltas are allowed!

Restarted Travis which failed spuriously :)

fjahr · 2020-07-21T18:52:12Z

Code review ACK 2cc2cbd

luke-jr · 2020-07-23T23:09:51Z

src/validation.cpp

+            if (!MoneyRange(amountdelta)) {
+                return false;
+            }


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

Before I push the commit does this look ok?

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

@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()]) :)

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.

practicalswift · 2020-08-18T18:48:34Z

@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:

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

DrahtBot · 2021-02-11T14:06:07Z

🐙 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".}

maflcko

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

maflcko · 2021-05-10T10:36:43Z

src/validation.cpp

            }
            TxValidationState state;
-            if (nTime + nExpiryTimeout > nNow) {
+            if (nTime > nNow - nExpiryTimeout) {


Already in ee11a41

maflcko · 2021-05-10T10:38:24Z

Closing due to inactivity

rajarshimaitra changed the title ~~Fixes UB reported in issue #19278~~ tests: Fixes UB reported in issue #19278 Jun 25, 2020

jonatack reviewed Jun 25, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot added Tests Validation labels Jun 25, 2020

maflcko removed the Tests label Jun 25, 2020

fanquake changed the title ~~tests: Fixes UB reported in issue #19278~~ validation: Fixes UB reported in issue #19278 Jun 26, 2020

rajarshimaitra force-pushed the loadmempool-UB branch from 8cc8548 to c58261d Compare July 1, 2020 07:44

laanwj added the Mempool label Jul 1, 2020

rajarshimaitra force-pushed the loadmempool-UB branch from c58261d to f19dcd4 Compare July 3, 2020 15:34

DrahtBot mentioned this pull request Jul 11, 2020

Refactor mempool.dat to be extensible, and store missing info #19488

Closed

practicalswift reviewed Jul 11, 2020

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

rajarshimaitra force-pushed the loadmempool-UB branch from f19dcd4 to 069043e Compare July 13, 2020 11:12

maflcko reviewed Jul 13, 2020

View reviewed changes

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

Fix UBSan warnings triggered when loading corrupt mempool.dat files

2cc2cbd

rajarshimaitra force-pushed the loadmempool-UB branch from 069043e to 2cc2cbd Compare July 18, 2020 04:54

luke-jr suggested changes Jul 23, 2020

View reviewed changes

DrahtBot mentioned this pull request Oct 5, 2020

validation: Increase robustness when loading malformed mempool.dat files (LoadMempool) #20089

Closed

DrahtBot mentioned this pull request Nov 13, 2020

Avoid signed integer overflow and invalid integer negation when loading malformed mempool.dat files #20383

Closed

This was referenced Jan 27, 2021

rpc/validation: enable packages through testmempoolaccept #20833

Merged

refactor: return MempoolAcceptResult from ATMP #21062

Merged

This was referenced Feb 10, 2021

fuzz: Add tx_pool fuzz target #21142

Merged

validation/coins: limit MemPoolAccept access to coins cache + make it responsible for uncaching #21146

Closed

DrahtBot added the Needs rebase label Feb 11, 2021

maflcko reviewed May 10, 2021

View reviewed changes

maflcko closed this May 10, 2021

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

Conversation

rajarshimaitra commented Jun 25, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajarshimaitra commented Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajarshimaitra commented Jul 1, 2020

Uh oh!

practicalswift commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajarshimaitra commented Jul 2, 2020

Uh oh!

DrahtBot commented Jul 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rajarshimaitra commented Jul 13, 2020

Uh oh!

practicalswift commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

rajarshimaitra Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

practicalswift Jul 18, 2020

Choose a reason for hiding this comment

Uh oh!

rajarshimaitra commented Jul 17, 2020

Uh oh!

practicalswift commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajarshimaitra commented Jul 18, 2020

Uh oh!

practicalswift commented Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr commented Jul 21, 2020

Uh oh!

luke-jr Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

rajarshimaitra Jul 24, 2020

Choose a reason for hiding this comment

Uh oh!

practicalswift Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajarshimaitra Jul 26, 2020

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Aug 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 11, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jun 25, 2020 •

edited

Loading

rajarshimaitra commented Jun 27, 2020 •

edited

Loading

practicalswift commented Jun 28, 2020 •

edited

Loading

practicalswift commented Jul 1, 2020 •

edited

Loading

DrahtBot commented Jul 11, 2020 •

edited

Loading

practicalswift left a comment •

edited

Loading

practicalswift commented Jul 13, 2020 •

edited

Loading

practicalswift commented Jul 17, 2020 •

edited

Loading

practicalswift commented Jul 18, 2020 •

edited

Loading

practicalswift Jul 25, 2020 •

edited

Loading

practicalswift commented Aug 18, 2020 •

edited

Loading