Skip to content

Commit

Permalink
Merge #20140: Restore compatibility with old CSubNet serialization
Browse files Browse the repository at this point in the history
886be97 Ignore incorrectly-serialized banlist.dat entries (Pieter Wuille)
883cea7 Restore compatibility with old CSubNet serialization (Pieter Wuille)

Pull request description:

  #19628 changed CSubNet for IPv4 netmasks, using the first 4 bytes of `netmask` rather than the last 4 to store the actual mask. Unfortunately, CSubNet objects are serialized on disk in banlist.dat, breaking compatibility with existing banlists (and bringing them into an inconsistent state where entries reported in `listbanned` cannot be removed).

  Fix this by reverting to the old format (just for serialization). Also add a sanity check to the deserializer so that nonsensical banlist.dat entries are ignored (which would otherwise be possible if someone added IPv4 entries after #19628 but without this PR).

  Reported by Greg Maxwell.

ACKs for top commit:
  laanwj:
    Code review ACK 886be97
  vasild:
    ACK 886be97

Tree-SHA512: d3fb91e8ecd933406e527187974f22770374ee2e12a233e7870363f52ecda471fb0b7bae72420e8ff6b6b1594e3037a5115984c023dbadf38f86aeaffcd681e7
  • Loading branch information
laanwj committed Oct 15, 2020
2 parents 3956165 + 886be97 commit e3b474c
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/banman.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ void BanMan::SweepBanned()
while (it != m_banned.end()) {
CSubNet sub_net = (*it).first;
CBanEntry ban_entry = (*it).second;
if (now > ban_entry.nBanUntil) {
if (!sub_net.IsValid() || now > ban_entry.nBanUntil) {
m_banned.erase(it++);
m_is_dirty = true;
notify_ui = true;
Expand Down
11 changes: 11 additions & 0 deletions src/netaddress.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1109,6 +1109,17 @@ bool CSubNet::IsValid() const
return valid;
}

bool CSubNet::SanityCheck() const
{
if (!(network.IsIPv4() || network.IsIPv6())) return false;

for (size_t x = 0; x < network.m_addr.size(); ++x) {
if (network.m_addr[x] & ~netmask[x]) return false;
}

return true;
}

bool operator==(const CSubNet& a, const CSubNet& b)
{
return a.valid == b.valid && a.network == b.network && !memcmp(a.netmask, b.netmask, 16);
Expand Down
20 changes: 19 additions & 1 deletion src/netaddress.h
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,8 @@ class CSubNet
/// Is this value valid? (only used to signal parse errors)
bool valid;

bool SanityCheck() const;

public:
CSubNet();
CSubNet(const CNetAddr& addr, uint8_t mask);
Expand All @@ -476,7 +478,23 @@ class CSubNet
friend bool operator!=(const CSubNet& a, const CSubNet& b) { return !(a == b); }
friend bool operator<(const CSubNet& a, const CSubNet& b);

SERIALIZE_METHODS(CSubNet, obj) { READWRITE(obj.network, obj.netmask, obj.valid); }
SERIALIZE_METHODS(CSubNet, obj)
{
READWRITE(obj.network);
if (obj.network.IsIPv4()) {
// Before commit 102867c587f5f7954232fb8ed8e85cda78bb4d32, CSubNet used the last 4 bytes of netmask
// to store the relevant bytes for an IPv4 mask. For compatiblity reasons, keep doing so in
// serialized form.
unsigned char dummy[12] = {0};
READWRITE(dummy);
READWRITE(MakeSpan(obj.netmask).first(4));
} else {
READWRITE(obj.netmask);
}
READWRITE(obj.valid);
// Mark invalid if the result doesn't pass sanity checking.
SER_READ(obj, if (obj.valid) obj.valid = obj.SanityCheck());
}
};

/** A combination of a network address (CNetAddr) and a (TCP) port */
Expand Down

0 comments on commit e3b474c

Please sign in to comment.