addrman: detect on-disk corrupted nNew and nTried during unserialization #22455

vasild commented at 11:48 AM on July 15, 2021: member

Negative nNew or nTried are not possible during normal operation. So, if we read such values during unserialize, report addrman corruption.

Fixes https://github.com/bitcoin/bitcoin/issues/22450

addrman: detect on-disk corrupted nNew and nTried during unserialization

Negative `nNew` or `nTried` are not possible during normal operation.
So, if we read such values during unserialize, report addrman
corruption.

Fixes https://github.com/bitcoin/bitcoin/issues/22450

816f29eab2

jnewbery commented at 12:10 PM on July 15, 2021: member

I don't think this is the right approach, and doesn't fix the underlying bug in #22450. It's addressing the symptom, rather than the fact that addrman's internal datastructures can get into an inconsistent state.

MarcoFalke commented at 12:22 PM on July 15, 2021: member

cr ACK 816f29eab296ebec2da8f8606ad618609e3ba228

can't hurt to check for corrupt data during deserialize

jonatack commented at 12:36 PM on July 15, 2021: member

ACK 816f29eab296ebec2da8f8606ad618609e3ba228

(nTried and nNew are declared as int but, like some of the int iterators in addrman.{h,cpp}, arguably should be size_t / unsigned)

vasild commented at 7:04 AM on July 16, 2021: member

@jnewbery, considering #22450 (comment), do you think that this PR fixes the underlying bug in #22450?

MarcoFalke commented at 7:27 AM on July 16, 2021: member

It looks like an improvement, but it doesn't fix the other issue(s) in #22450: #22450 (comment)

jnewbery commented at 7:47 AM on July 16, 2021: member

do you think that this PR fixes the underlying bug

No. I agree with Marco that this may be an improvement, but it's not fixing the bugs in #22450.

vasild commented at 7:52 AM on July 16, 2021: member

I am talking about the bug that is reported in the #22450 OP/description. Others would need separate PRs. I need to be sure that the #22450 OP/description is fixed properly, which I believe this PR does, but if you disagree, then maybe I am wrong and this PR needs an update.

MarcoFalke commented at 8:03 AM on July 16, 2021: member

Maybe remove the "Fixes #22450" from OP to avoid closing the issue. We can then use it as meta issue for addrman deserialize bugs.

MarcoFalke added this to the milestone 22.0 on Jul 16, 2021

sipa commented at 3:17 PM on July 16, 2021: member

I'm quite anxious about the fact that we're now modifying addrman in a way that apparently introduces inconsistencies.

Would it be possible to just revert that part, and rely on the higher level code to not insert invalid I2P address/ports? That means existing nodes running master won't be able to connect to the I2P addresses existing in their addrman, but I don't think that's much of an issue as I2P support isn't in any release yet?

vasild commented at 4:39 PM on July 16, 2021: member

Maybe remove the "Fixes #22450" from OP to avoid closing the issue. We can then use it as meta issue for addrman deserialize bugs.

I think it is better to report different problems as separate bug entries, so that none gets forgotten and the conversations do not mix in one bug report. So, now we have:

#22450 (the text "Introduced in commit e0a2b39" in the OP is misleading), fixed in #22455. Not relevant to I2P.
#22467, fixed in #22468. A bug in CAddrMan::ResetI2PPorts(), found by fuzzing.
#22470, fixed in #22471. A bug in CAddrMan::ResetI2PPorts(), found by @jnewbery looking at the code.

lsilva01 approved

lsilva01 commented at 4:54 AM on July 18, 2021: contributor

Code Review ACK https://github.com/bitcoin/bitcoin/pull/22455/commits/816f29eab296ebec2da8f8606ad618609e3ba228. This change provides a more accurate description of the error.

If I'm understanding the problem so far, I agree with @sipa.

I don't see a need for this logic to keep existing I2P connections when the support for them hasn't even been released yet. It seems simpler to me not to change the logic of a critical component like CAddrMan and only accept I2P connections with port = 0.

MarcoFalke commented at 12:24 PM on July 19, 2021: member

Upgrading my review ACK to tested ACK using commit fa740252d2dcc272204b745906785fe26e9a0b77 from #22493 and the following diff:

diff --git a/src/addrman.cpp b/src/addrman.cpp
index 8192b4eba6..4e21062444 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -468,6 +468,5 @@ int CAddrMan::Check_()
     std::unordered_map<int, int> mapNew;
 
-    if (vRandom.size() != (size_t)(nTried + nNew))
-        return -7;
+    assert(vRandom.size() == (size_t)(nTried + nNew));
 
     for (const auto& entry : mapInfo) {

crash-ce5f3356346c37f5a1eb17cf252105a87f6264ea.log

Before (commit fa740252d2dcc272204b745906785fe26e9a0b77 + patch):

$ FUZZ=addrman ./src/test/fuzz/fuzz ./crash-ce5f3356346c37f5a1eb17cf252105a87f6264ea.log 
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1743330088
INFO: Loaded 1 modules   (229079 inline 8-bit counters): 229079 [0x556cd64990f8, 0x556cd64d0fcf), 
INFO: Loaded 1 PC tables (229079 PCs): 229079 [0x556cd64d0fd0,0x556cd684fd40), 
./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
Running: ./crash-ce5f3356346c37f5a1eb17cf252105a87f6264ea.log
fuzz: addrman.cpp:470: int CAddrMan::Check_(): Assertion `vRandom.size() == (size_t)(nTried + nNew)' failed.

After (commit fa740252d2dcc272204b745906785fe26e9a0b77 + patch + 816f29eab296ebec2da8f8606ad618609e3ba228 cherry-picked):

$ FUZZ=addrman ./src/test/fuzz/fuzz ./crash-ce5f3356346c37f5a1eb17cf252105a87f6264ea.log 
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3797513599
INFO: Loaded 1 modules   (229109 inline 8-bit counters): 229109 [0x5619431f80f8, 0x56194322ffed), 
INFO: Loaded 1 PC tables (229109 PCs): 229109 [0x56194322fff0,0x5619435aef40), 
./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
Running: ./crash-ce5f3356346c37f5a1eb17cf252105a87f6264ea.log
Executed ./crash-ce5f3356346c37f5a1eb17cf252105a87f6264ea.log in 3 ms
***
*** NOTE: fuzzing was not performed, you have only
***       executed the target code on a fixed set of inputs.
***

MarcoFalke merged this on Jul 19, 2021

MarcoFalke closed this on Jul 19, 2021

vasild deleted the branch on Jul 19, 2021

sidhujag referenced this in commit 3b4a8af0d5 on Jul 23, 2021

PastaPastaPasta referenced this in commit 55d085a018 on Mar 5, 2022

PastaPastaPasta referenced this in commit fccdbd2da3 on Mar 5, 2022

PastaPastaPasta referenced this in commit 82a8a80fd6 on Mar 5, 2022

PastaPastaPasta referenced this in commit de08467941 on Mar 7, 2022

gades referenced this in commit 9223d69d83 on May 8, 2022

gwillen referenced this in commit 1a7c69b689 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022