From 3383b790493cc9822d070dec84a4d05672f02915 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 8 Mar 2022 16:48:18 +0100 Subject: [PATCH] Merge bitcoin/bitcoin#24312: addrman: Log too low compat value fa097d074bc1afcc2a52976796bb618f7c6a68b3 addrman: Log too low compat value (MarcoFalke) Pull request description: Before this patch, when writing a negative `lowest_compatible` value, it would be read as a positive value. For example `-32` will be read as `224`. There is generally nothing wrong with that. Though, similarly there shouldn't be anything wrong with refusing to read a negative value. I find the code after this patch more logical than before. Also, this allows dropping a file-wide sanitizer suppression. In practice none of this should ever happen. Bitcoin Core would never write a negative `lowest_compatible` in normal operation, unless the file storage is later corrupted by external influence. ACKs for top commit: mzumsande: re-ACK fa097d074bc1afcc2a52976796bb618f7c6a68b3 Tree-SHA512: 9aae7b8fe666f52f667f149667025e0160cef1a793cc4d392e36608f65c2bee8096da429235118f40a3368f327aabe30f3732ae78c5874648ea6f423f2687b65 --- src/addrman.cpp | 8 +++++++- test/functional/feature_addrman.py | 11 +++++++++++ test/sanitizer_suppressions/ubsan | 1 - 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 835b9aa6fe..52d36cd145 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -244,12 +244,18 @@ void AddrManImpl::Unserialize(Stream& s_) uint8_t compat; s >> compat; + if (compat < INCOMPATIBILITY_BASE) { + throw std::ios_base::failure(strprintf( + "Corrupted addrman database: The compat value (%u) " + "is lower than the expected minimum value %u.", + compat, INCOMPATIBILITY_BASE)); + } const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE; if (lowest_compatible > FILE_FORMAT) { throw InvalidAddrManVersionError(strprintf( "Unsupported format of addrman database: %u. It is compatible with formats >=%u, " "but the maximum supported by this version of %s is %u.", - uint8_t{format}, uint8_t{lowest_compatible}, PACKAGE_NAME, uint8_t{FILE_FORMAT})); + uint8_t{format}, lowest_compatible, PACKAGE_NAME, uint8_t{FILE_FORMAT})); } s >> nKey; diff --git a/test/functional/feature_addrman.py b/test/functional/feature_addrman.py index 2d00e26966..51203f8d49 100755 --- a/test/functional/feature_addrman.py +++ b/test/functional/feature_addrman.py @@ -67,6 +67,17 @@ class AddrmanTest(BitcoinTestFramework): self.start_node(0, extra_args=["-checkaddrman=1"]) assert_equal(self.nodes[0].getnodeaddresses(), []) + self.log.info("Check that addrman with negative lowest_compatible cannot be read") + self.stop_node(0) + write_addrman(peers_dat, lowest_compatible=-32) + self.nodes[0].assert_start_raises_init_error( + expected_msg=init_error( + "Corrupted addrman database: The compat value \\(0\\) is lower " + "than the expected minimum value 32.: (.+)" + ), + match=ErrorMatch.FULL_REGEX, + ) + self.log.info("Check that addrman from future is overwritten with new addrman") self.stop_node(0) write_addrman(peers_dat, lowest_compatible=111) diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index c5fb0d34f4..f0828c01cd 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -78,7 +78,6 @@ implicit-integer-sign-change:util/strencodings.h implicit-integer-sign-change:validation.cpp implicit-signed-integer-truncation,implicit-integer-sign-change:chain.h implicit-signed-integer-truncation,implicit-integer-sign-change:test/skiplist_tests.cpp -implicit-signed-integer-truncation:addrman.cpp implicit-signed-integer-truncation:addrman.h implicit-signed-integer-truncation:chain.h implicit-signed-integer-truncation:crypto/