Permalink
Browse files

Merge #7696: Fix de-serialization bug where AddrMan is left corrupted

1475ecf 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. (EthanHeilman)
  • Loading branch information...
sipa committed May 17, 2016
2 parents e2bf830 + 1475ecf commit 5c3f8ddcaa1164079105c452429fccf8127b01b6
Showing with 146 additions and 0 deletions.
  1. +1 −0 src/Makefile.test.include
  2. +8 −0 src/net.cpp
  3. +1 −0 src/net.h
  4. +136 −0 src/test/net_tests.cpp
@@ -59,6 +59,7 @@ BITCOIN_TESTS =\
test/merkle_tests.cpp \
test/miner_tests.cpp \
test/multisig_tests.cpp \
test/net_tests.cpp \
test/netbase_tests.cpp \
test/pmt_tests.cpp \
test/policyestimator_tests.cpp \
View
@@ -1950,6 +1950,7 @@ void StartNode(boost::thread_group& threadGroup, CScheduler& scheduler)
if (adb.Read(addrman))
LogPrintf("Loaded %i addresses from peers.dat %dms\n", addrman.size(), GetTimeMillis() - nStart);
else {
addrman.Clear(); // Addrman can be in an inconsistent state after failure, reset it
LogPrintf("Invalid or missing peers.dat; recreating\n");
DumpAddresses();
}
@@ -2320,6 +2321,11 @@ bool CAddrDB::Read(CAddrMan& addr)
if (hashIn != hashTmp)
return error("%s: Checksum mismatch, data corrupted", __func__);
return Read(addr, ssPeers);
}
bool CAddrDB::Read(CAddrMan& addr, CDataStream& ssPeers)
{
unsigned char pchMsgTmp[4];
try {
// de-serialize file header (network specific magic number) and ..
@@ -2333,6 +2339,8 @@ bool CAddrDB::Read(CAddrMan& addr)
ssPeers >> addr;
}
catch (const std::exception& e) {
// de-serialization has failed, ensure addrman is left in a clean state
addr.Clear();
return error("%s: Deserialize or I/O error - %s", __func__, e.what());
}
View
@@ -794,6 +794,7 @@ class CAddrDB
CAddrDB();
bool Write(const CAddrMan& addr);
bool Read(CAddrMan& addr);
bool Read(CAddrMan& addr, CDataStream& ssPeers);
};
/** Access to the banlist database (banlist.dat) */
View
@@ -0,0 +1,136 @@
// Copyright (c) 2012-2016 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include "addrman.h"
#include "test/test_bitcoin.h"
#include <string>
#include <boost/test/unit_test.hpp>
#include "hash.h"
#include "serialize.h"
#include "streams.h"
#include "net.h"
#include "chainparams.h"
using namespace std;
class CAddrManSerializationMock : public CAddrMan
{
public:
virtual void Serialize(CDataStream& s, int nType, int nVersionDummy) const = 0;
};
class CAddrManUncorrupted : public CAddrManSerializationMock
{
public:
void Serialize(CDataStream& s, int nType, int nVersionDummy) const
{
CAddrMan::Serialize(s, nType, nVersionDummy);
}
};
class CAddrManCorrupted : public CAddrManSerializationMock
{
public:
void Serialize(CDataStream& s, int nType, int nVersionDummy) const
{
// Produces corrupt output that claims addrman has 20 addrs when it only has one addr.
unsigned char nVersion = 1;
s << nVersion;
s << ((unsigned char)32);
s << nKey;
s << 10; // nNew
s << 10; // nTried
int nUBuckets = ADDRMAN_NEW_BUCKET_COUNT ^ (1 << 30);
s << nUBuckets;
CAddress addr = CAddress(CService("252.1.1.1", 7777));
CAddrInfo info = CAddrInfo(addr, CNetAddr("252.2.2.2"));
s << info;
}
};
CDataStream AddrmanToStream(CAddrManSerializationMock& addrman)
{
CDataStream ssPeersIn(SER_DISK, CLIENT_VERSION);
ssPeersIn << FLATDATA(Params().MessageStart());
ssPeersIn << addrman;
std::string str = ssPeersIn.str();
vector<unsigned char> vchData(str.begin(), str.end());
return CDataStream(vchData, SER_DISK, CLIENT_VERSION);
}
BOOST_FIXTURE_TEST_SUITE(net_tests, BasicTestingSetup)
BOOST_AUTO_TEST_CASE(caddrdb_read)
{
CAddrManUncorrupted addrmanUncorrupted;
CService addr1 = CService("250.7.1.1", 8333);
CService addr2 = CService("250.7.2.2", 9999);
CService addr3 = CService("250.7.3.3", 9999);
// Add three addresses to new table.
addrmanUncorrupted.Add(CAddress(addr1), CService("252.5.1.1", 8333));
addrmanUncorrupted.Add(CAddress(addr2), CService("252.5.1.1", 8333));
addrmanUncorrupted.Add(CAddress(addr3), CService("252.5.1.1", 8333));
// Test that the de-serialization does not throw an exception.
CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted);
bool exceptionThrown = false;
CAddrMan addrman1;
BOOST_CHECK(addrman1.size() == 0);
try {
unsigned char pchMsgTmp[4];
ssPeers1 >> FLATDATA(pchMsgTmp);
ssPeers1 >> addrman1;
} catch (const std::exception& e) {
exceptionThrown = true;
}
BOOST_CHECK(addrman1.size() == 3);
BOOST_CHECK(exceptionThrown == false);
// Test that CAddrDB::Read creates an addrman with the correct number of addrs.
CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted);
CAddrMan addrman2;
CAddrDB adb;
BOOST_CHECK(addrman2.size() == 0);
adb.Read(addrman2, ssPeers2);
BOOST_CHECK(addrman2.size() == 3);
}
BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted)
{
CAddrManCorrupted addrmanCorrupted;
// Test that the de-serialization of corrupted addrman throws an exception.
CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted);
bool exceptionThrown = false;
CAddrMan addrman1;
BOOST_CHECK(addrman1.size() == 0);
try {
unsigned char pchMsgTmp[4];
ssPeers1 >> FLATDATA(pchMsgTmp);
ssPeers1 >> addrman1;
} catch (const std::exception& e) {
exceptionThrown = true;
}
// Even through de-serialization failed adddrman is not left in a clean state.
BOOST_CHECK(addrman1.size() == 1);
BOOST_CHECK(exceptionThrown);
// Test that CAddrDB::Read leaves addrman in a clean state if de-serialization fails.
CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted);
CAddrMan addrman2;
CAddrDB adb;
BOOST_CHECK(addrman2.size() == 0);
adb.Read(addrman2, ssPeers2);
BOOST_CHECK(addrman2.size() == 0);
}
BOOST_AUTO_TEST_SUITE_END()

0 comments on commit 5c3f8dd

Please sign in to comment.