Restore compatibility with old CSubNet serialization

sipa commented at 9:27 pm on October 12, 2020: member

#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.

sipa added the label Bug on Oct 12, 2020

sipa added the label Data corruption on Oct 12, 2020

sipa added this to the milestone 0.21.0 on Oct 12, 2020

sipa requested review from vasild on Oct 12, 2020

Restore compatibility with old CSubNet serialization 883cea7dea

Ignore incorrectly-serialized banlist.dat entries 886be97af5

sipa force-pushed on Oct 12, 2020

kallewoof commented at 2:52 am on October 13, 2020: member

Code review ACK

Slightly prefer if this was handled with versioning instead (i.e. bump banlist version, if there is one, have a grace period for a few versions where both are supported).

I think it’s okay to eventually drop backwards compatibility for banlists, but maybe I don’t take them seriously enough.

sipa commented at 3:11 am on October 13, 2020: member

@kallewoof I agree that that would be the way to go if there was actually a feature change. There isn’t. The serialization format was just gratuitously broken, and this reverts it.

DrahtBot commented at 3:25 am on October 13, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19825 (rpc: simplify setban and consolidate BanMan functions by dhruv)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Coverage

Coverage	Change (pull 20140, 6bb49ef1ff500e332174ddbbe71aae7af3d92e29)	Reference (master, 3caee16946575e71e90ead9ac531f5a3a1259307)
Lines	+0.0037 %	90.7139 %
Functions	-0.0054 %	86.2441 %
Branches	+0.0097 %	52.0898 %

Updated at: 2020-10-15T09:31:57.171185.

practicalswift commented at 9:20 am on October 13, 2020: contributor

Concept ACK

vasild approved

vasild commented at 9:35 am on October 13, 2020: member

ACK 886be97af

Maybe we don’t need to ignore the entries that have the IPv4 mask in the first 4 bytes (second commit) because this code was not released?

I am planning to change CSubNet::netmask from uint8_t [16] to just uint8_t (CIDR netmask number) and this is tightly related. I guess I will keep the (un)serialize to use this old format of 16 bytes with IPv4 netmask at the end because introducing a version of banlist.dat does not look justified (it will save 15 bytes per entry on disk).

sipa commented at 4:00 pm on October 13, 2020: member

@vasild Agree that there is no strong requirement to deal with files created/modified using unreleased code. But as it’s perhaps a generally useful change (it guarantees that regardless of the file’s contents, you’ll never end up in a situation where there are entries in listbanned that cannot be remvoed using setban), perhaps it’s useful to keep it in?

vasild commented at 4:06 pm on October 13, 2020: member

Yes!

laanwj commented at 8:19 am on October 14, 2020: member

Thanks for the quick fix! Code review ACK 886be97af5d4aba338b23a7b20b8560be8156231

Maybe we don’t need to ignore the entries that have the IPv4 mask in the first 4 bytes (second commit) because this code was not released?

I agree about this train of thought. I’m fine with keeping that sanity check if it also serves another purpose, but also understand the argument to not clutter the code with special-purpose workarounds for corruption issues that were in the master branch only for a short time. At the least it would be good to add a comment with a timeframe for removing the work-around.

gmaxwell commented at 7:11 pm on October 14, 2020: contributor

I would suggest that it assert on any wonky-mask, except then you’d create a crash for anyone that had bans from current git master. You really don’t want those in memory, esp if later the ban lookup is changed to a patricia trie they’ll turn into memory corruption bugs (unless the author of that code remembers this possibility and inserts the sanitization there).

Perhaps an alternative is to have the sanitization and the plan for removing it is to stop storing masks and instead store prefix lengths, so that the insane values are just not representable.

sipa commented at 7:13 pm on October 14, 2020: member

@gmaxwell This deletes the wonky-mask ones immediately after loading, FWIW. The deserializer constructs them as entries with valid=false, which are then immediately pruned.

gmaxwell commented at 7:37 pm on October 14, 2020: contributor

sorry, I was intending to respond to @laanwj who seemed to not like the deleting that much!

sipa commented at 7:40 pm on October 14, 2020: member

Oh, I missed that. @laanwj I’m thinking that this sanity check should probably stay in the code. It isn’t specific to the incompatbility here - any other damage to your banlist.dat file could also mean you end up with unremovable entries, so it seems like a generic improvement to protect against that.

vasild commented at 7:37 am on October 15, 2020: member

stop storing masks and instead store prefix lengths, so that the insane values are just not representable

Just to note that even if we store prefix lengths (CIDR), that would be stored in 1 byte and it is still possible to have insane values, e.g. 150.

laanwj commented at 9:21 am on October 15, 2020: member

It’s good to ward against corrupted data files, especially relatively unimportant ones like the ban list, but at some point, disk is a trusted interface. There are tons of ways disk corruption can crash the daemon. And sometimes it should, so that users are aware of what is happening, making things easier to diagnose (weird behavior like undeletable bans is worse, but also impossible to avoid in general).

That said I’m okay with this fix as it is, and don’t think this particular change is worth bikeshedding too much. It needs to be fixed as soon as possible to avoid doing more damage.

laanwj merged this on Oct 15, 2020

laanwj closed this on Oct 15, 2020

sidhujag referenced this in commit 16a7f0f84f on Oct 16, 2020

Fabcien referenced this in commit ab15f29c5b on Feb 5, 2021

PastaPastaPasta referenced this in commit 05a4222f19 on Sep 17, 2021

PastaPastaPasta referenced this in commit 18e0bb20dd on Sep 19, 2021

PastaPastaPasta referenced this in commit 45b30c3b3a on Sep 24, 2021

kittywhiskers referenced this in commit 12afb98a99 on Oct 12, 2021

DrahtBot locked this on Feb 15, 2022

Restore compatibility with old CSubNet serialization #20140

Conflicts

Coverage