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)

4 files changed, 146 insertions, 0 deletions

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 08e2f6af4d..897a7dd4a2 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -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 \
diff --git a/src/net.cpp b/src/net.cpp
index aa5b47340a..abc492e2b9 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -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());
     }
 
diff --git a/src/net.h b/src/net.h
index fd80056c6b..9ab820ca69 100644
--- a/src/net.h
+++ b/src/net.h
@@ -794,6 +794,7 @@ public:
     CAddrDB();
     bool Write(const CAddrMan& addr);
     bool Read(CAddrMan& addr);
+    bool Read(CAddrMan& addr, CDataStream& ssPeers);
 };
 
 /** Access to the banlist database (banlist.dat) */
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
new file mode 100644
index 0000000000..6debf6ac5e
--- /dev/null
+++ b/src/test/net_tests.cpp
@@ -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()