Skip to content

Commit 1475ecf

Browse files
EthanHeilmanEthanHeilman
committed
Fix de-serialization bug where AddrMan is corrupted after exception
* CAddrDB modified so that when de-serialization code throws an exception Addrman is reset to a clean state * CAddrDB modified to make unit tests possible * Regression test created to ensure bug is fixed * StartNode modifed to clear adrman if CAddrDB::Read returns an error code.
1 parent 326f010 commit 1475ecf

File tree

4 files changed

+146
-0
lines changed

4 files changed

+146
-0
lines changed

‎src/Makefile.test.include‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ BITCOIN_TESTS =\
6060
test/merkle_tests.cpp \
6161
test/miner_tests.cpp \
6262
test/multisig_tests.cpp \
63+
test/net_tests.cpp \
6364
test/netbase_tests.cpp \
6465
test/pmt_tests.cpp \
6566
test/policyestimator_tests.cpp \

‎src/net.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1944,6 +1944,7 @@ void StartNode(boost::thread_group& threadGroup, CScheduler& scheduler)
19441944
if (adb.Read(addrman))
19451945
LogPrintf("Loaded %i addresses from peers.dat %dms\n", addrman.size(), GetTimeMillis() - nStart);
19461946
else {
1947+
addrman.Clear(); // Addrman can be in an inconsistent state after failure, reset it
19471948
LogPrintf("Invalid or missing peers.dat; recreating\n");
19481949
DumpAddresses();
19491950
}
@@ -2336,6 +2337,11 @@ bool CAddrDB::Read(CAddrMan& addr)
23362337
if (hashIn != hashTmp)
23372338
return error("%s: Checksum mismatch, data corrupted", __func__);
23382339

2340+
return Read(addr, ssPeers);
2341+
}
2342+
2343+
bool CAddrDB::Read(CAddrMan& addr, CDataStream& ssPeers)
2344+
{
23392345
unsigned char pchMsgTmp[4];
23402346
try {
23412347
// de-serialize file header (network specific magic number) and ..
@@ -2349,6 +2355,8 @@ bool CAddrDB::Read(CAddrMan& addr)
23492355
ssPeers >> addr;
23502356
}
23512357
catch (const std::exception& e) {
2358+
// de-serialization has failed, ensure addrman is left in a clean state
2359+
addr.Clear();
23522360
return error("%s: Deserialize or I/O error - %s", __func__, e.what());
23532361
}
23542362

‎src/net.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -779,6 +779,7 @@ class CAddrDB
779779
CAddrDB();
780780
bool Write(const CAddrMan& addr);
781781
bool Read(CAddrMan& addr);
782+
bool Read(CAddrMan& addr, CDataStream& ssPeers);
782783
};
783784

784785
/** Access to the banlist database (banlist.dat) */

‎src/test/net_tests.cpp‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
// Copyright (c) 2012-2016 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
#include "addrman.h"
5+
#include "test/test_bitcoin.h"
6+
#include <string>
7+
#include <boost/test/unit_test.hpp>
8+
#include "hash.h"
9+
#include "serialize.h"
10+
#include "streams.h"
11+
#include "net.h"
12+
#include "chainparams.h"
13+
14+
using namespace std;
15+
16+
class CAddrManSerializationMock : public CAddrMan
17+
{
18+
public:
19+
virtual void Serialize(CDataStream& s, int nType, int nVersionDummy) const = 0;
20+
};
21+
22+
class CAddrManUncorrupted : public CAddrManSerializationMock
23+
{
24+
public:
25+
void Serialize(CDataStream& s, int nType, int nVersionDummy) const
26+
{
27+
CAddrMan::Serialize(s, nType, nVersionDummy);
28+
}
29+
};
30+
31+
class CAddrManCorrupted : public CAddrManSerializationMock
32+
{
33+
public:
34+
void Serialize(CDataStream& s, int nType, int nVersionDummy) const
35+
{
36+
// Produces corrupt output that claims addrman has 20 addrs when it only has one addr.
37+
unsigned char nVersion = 1;
38+
s << nVersion;
39+
s << ((unsigned char)32);
40+
s << nKey;
41+
s << 10; // nNew
42+
s << 10; // nTried
43+
44+
int nUBuckets = ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30);
45+
s << nUBuckets;
46+
47+
CAddress addr = CAddress(CService("252.1.1.1", 7777));
48+
CAddrInfo info = CAddrInfo(addr, CNetAddr("252.2.2.2"));
49+
s << info;
50+
}
51+
};
52+
53+
CDataStream AddrmanToStream(CAddrManSerializationMock& addrman)
54+
{
55+
CDataStream ssPeersIn(SER_DISK, CLIENT_VERSION);
56+
ssPeersIn << FLATDATA(Params().MessageStart());
57+
ssPeersIn << addrman;
58+
std::string str = ssPeersIn.str();
59+
vector<unsigned char> vchData(str.begin(), str.end());
60+
return CDataStream(vchData, SER_DISK, CLIENT_VERSION);
61+
}
62+
63+
BOOST_FIXTURE_TEST_SUITE(net_tests, BasicTestingSetup)
64+
65+
BOOST_AUTO_TEST_CASE(caddrdb_read)
66+
{
67+
CAddrManUncorrupted addrmanUncorrupted;
68+
69+
CService addr1 = CService("250.7.1.1", 8333);
70+
CService addr2 = CService("250.7.2.2", 9999);
71+
CService addr3 = CService("250.7.3.3", 9999);
72+
73+
// Add three addresses to new table.
74+
addrmanUncorrupted.Add(CAddress(addr1), CService("252.5.1.1", 8333));
75+
addrmanUncorrupted.Add(CAddress(addr2), CService("252.5.1.1", 8333));
76+
addrmanUncorrupted.Add(CAddress(addr3), CService("252.5.1.1", 8333));
77+
78+
// Test that the de-serialization does not throw an exception.
79+
CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted);
80+
bool exceptionThrown = false;
81+
CAddrMan addrman1;
82+
83+
BOOST_CHECK(addrman1.size() == 0);
84+
try {
85+
unsigned char pchMsgTmp[4];
86+
ssPeers1 >> FLATDATA(pchMsgTmp);
87+
ssPeers1 >> addrman1;
88+
} catch (const std::exception& e) {
89+
exceptionThrown = true;
90+
}
91+
92+
BOOST_CHECK(addrman1.size() == 3);
93+
BOOST_CHECK(exceptionThrown == false);
94+
95+
// Test that CAddrDB::Read creates an addrman with the correct number of addrs.
96+
CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted);
97+
98+
CAddrMan addrman2;
99+
CAddrDB adb;
100+
BOOST_CHECK(addrman2.size() == 0);
101+
adb.Read(addrman2, ssPeers2);
102+
BOOST_CHECK(addrman2.size() == 3);
103+
}
104+
105+
106+
BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted)
107+
{
108+
CAddrManCorrupted addrmanCorrupted;
109+
110+
// Test that the de-serialization of corrupted addrman throws an exception.
111+
CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted);
112+
bool exceptionThrown = false;
113+
CAddrMan addrman1;
114+
BOOST_CHECK(addrman1.size() == 0);
115+
try {
116+
unsigned char pchMsgTmp[4];
117+
ssPeers1 >> FLATDATA(pchMsgTmp);
118+
ssPeers1 >> addrman1;
119+
} catch (const std::exception& e) {
120+
exceptionThrown = true;
121+
}
122+
// Even through de-serialization failed adddrman is not left in a clean state.
123+
BOOST_CHECK(addrman1.size() == 1);
124+
BOOST_CHECK(exceptionThrown);
125+
126+
// Test that CAddrDB::Read leaves addrman in a clean state if de-serialization fails.
127+
CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted);
128+
129+
CAddrMan addrman2;
130+
CAddrDB adb;
131+
BOOST_CHECK(addrman2.size() == 0);
132+
adb.Read(addrman2, ssPeers2);
133+
BOOST_CHECK(addrman2.size() == 0);
134+
}
135+
136+
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)