p2p: fix ubsan addrman errors, make nTime truncation conversion explicit #22094

This patch

1. drops an unused nTime param from CAddrMan::Connected_() and CAddrMan::Connected()

2. makes an nTime integer truncations explicit, as CAddress::nTime is declared in src/protocol.h as uint32 but int64 values can be written to it in CAddrMan::Connected_(). This also fixes the following non-suppressed UBSan error in addrman.cpp:

SUMMARY: UndefinedBehaviorSanitizer
addrman.cpp:535:22: runtime error: implicit conversion
  from type 'int64_t' (aka 'long') of value 68719478016 (64-bit, signed)
  to type 'uint32_t' (aka 'unsigned int') changed the value to 1280 (32-bit, unsigned)

(According to https://en.cppreference.com/w/cpp/language/implicit_conversion, if the destination type is unsigned, the resulting value is the smallest unsigned value equal to the source value modulo (2 exponent n), where n is the number of bits used to represent the destination type. So it seems this is defined behavior and the signed integer is being truncated correctly. However, using a named cast makes the truncation conversion explicit and reduces the warnings by the UBSan integer sanitizer.)

3. does the same for two similar UBSan errors in CAddrMan::Add_() that were suppressed

4. fixes the following UBSan error in CAddrMan::Unserialize() that was suppressed

SUMMARY: UndefinedBehaviorSanitizer
addrman.h:320:43: runtime error: implicit conversion
  from type 'int' of value -32 (32-bit, signed)
  to type 'const uint8_t' (aka 'const unsigned char')
  changed the value to 224 (8-bit, unsigned)

5. removes the four addrman UBSan suppressions

6. adds TODO documentation for updates needed by the Year 2106

To test

$ ./autogen.sh && ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined,integer CC=clang CXX=clang++ && make clean && make
$ FUZZ=addrman src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/addrman

Code review ACK https://github.com/bitcoin/bitcoin/pull/22094/commits/ffd89f1d68fcea254565d8a4ff3e81bae078d378

ACK ffd89f1

Tested on macOs with

 ./autogen.sh && ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined,integer CC=clang CXX=clang++
make
FUZZ=addrman src/test/fuzz/fuzz ~/Desktop/codes/SSD/bitcoin_data/qa-assets`

addrman.cpp:535:22: runtime error: implicit conversion error not thrown from test

What is the goal of this change? Is it to remove a suppression? If yes, why is the suppression not removed in this pull?

Also, it seems a bit early to fuzz years larger than 2106 (80 years in the future). The network code will need to be changed anyway by then and this change doesn't seem to make it any easier (or harder).

See also https://github.com/bitcoin/bips/pull/967#issue-467275837

Using a named cast makes the truncation conversion explicit, which ISTM is preferable than implicit. Happy to close if explicit isn't seen as better or sufficient justification.

Updated the PR title.

I agree that absent any other points the explicit conversion is better than the implicit one. Though, this silences a warning, which might also be worthy to fix. Either by saying it is "invalid" (can be fixed in several decades) and ignoring it or by updating the fuzz test to not fuzz years past 2106.

Either way, the suppressions file should be updated as part of the change, if applicable.

Given that the second argument of Connected() is never used, maybe it is better to drop it:

<details> <summary>[patch] remove nTime argument</summary>

diff --git i/src/addrman.cpp w/src/addrman.cpp
index 8192b4eba6..149d929e73 100644
--- i/src/addrman.cpp
+++ w/src/addrman.cpp
@@ -570,13 +570,13 @@ void CAddrMan::GetAddr_(std::vector<CAddress>& vAddr, size_t max_addresses, size
         if (ai.IsTerrible(now)) continue;
 
         vAddr.push_back(ai);
     }
 }
 
-void CAddrMan::Connected_(const CService& addr, int64_t nTime)
+void CAddrMan::Connected_(const CService& addr)
 {
     AssertLockHeld(cs);
 
     CAddrInfo* pinfo = Find(addr);
 
     // if not found, bail out
@@ -588,14 +588,16 @@ void CAddrMan::Connected_(const CService& addr, int64_t nTime)
     // check whether we are talking about the exact same CService (including same port)
     if (info != addr)
         return;
 
     // update info
     int64_t nUpdateInterval = 20 * 60;
-    if (nTime - info.nTime > nUpdateInterval)
-        info.nTime = nTime;
+    const auto now = GetAdjustedTime();
+    if (now - info.nTime > nUpdateInterval) {
+        info.nTime = now;
+    }
 }
 
 void CAddrMan::SetServices_(const CService& addr, ServiceFlags nServices)
 {
     AssertLockHeld(cs);
 
diff --git i/src/addrman.h w/src/addrman.h
index 1fc64ac07f..28f445b071 100644
--- i/src/addrman.h
+++ w/src/addrman.h
@@ -605,18 +605,18 @@ public:
         GetAddr_(vAddr, max_addresses, max_pct, network);
         Check();
         return vAddr;
     }
 
     //! Outer function for Connected_()
-    void Connected(const CService &addr, int64_t nTime = GetAdjustedTime())
+    void Connected(const CService &addr)
         EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         Check();
-        Connected_(addr, nTime);
+        Connected_(addr);
         Check();
     }
 
     void SetServices(const CService &addr, ServiceFlags nServices)
         EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
@@ -760,15 +760,14 @@ private:
      *  this information doesn't leak topology information to network spies.
      *
      *  net_processing calls this function when it *disconnects* from a peer to
      *  not leak information about currently connected peers.
      *
      * [@param](/bitcoin-bitcoin/contributor/param/)[in]   addr     The address of the peer we were connected to
-     * [@param](/bitcoin-bitcoin/contributor/param/)[in]   nTime    The time that we were last connected to this peer
      */
-    void Connected_(const CService& addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
+    void Connected_(const CService& addr) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Update an entry's service bits.
     void SetServices_(const CService &addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Remove invalid addresses.
     void RemoveInvalid() EXCLUSIVE_LOCKS_REQUIRED(cs);
diff --git i/src/test/fuzz/addrman.cpp w/src/test/fuzz/addrman.cpp
index 92c34e74d9..38c551e0e3 100644
--- i/src/test/fuzz/addrman.cpp
+++ w/src/test/fuzz/addrman.cpp
@@ -100,13 +100,13 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman)
                     addr_man.Attempt(*opt_service, fuzzed_data_provider.ConsumeBool(), ConsumeTime(fuzzed_data_provider));
                 }
             },
             [&] {
                 const std::optional<CService> opt_service = ConsumeDeserializable<CService>(fuzzed_data_provider);
                 if (opt_service) {
-                    addr_man.Connected(*opt_service, ConsumeTime(fuzzed_data_provider));
+                    addr_man.Connected(*opt_service);
                 }
             },
             [&] {
                 const std::optional<CService> opt_service = ConsumeDeserializable<CService>(fuzzed_data_provider);
                 if (opt_service) {
                     addr_man.SetServices(*opt_service, ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS));

</details>

Either way, the suppression unsigned-integer-overflow:addrman.cpp cannot be removed because according to #21000 there is another place in addrman.cpp that has a similar problem.

Given that the second argument of Connected() is never used, maybe it is better to drop it: [patch] remove nTime argument

Either way, the suppression unsigned-integer-overflow:addrman.cpp cannot be removed because according to #21000 there is another place in addrman.cpp that has a similar problem.

Thanks! Removing the argument seems out of scope of this change, but you're right.

Rebased, added a second UBSan fix, and removed the addrman UBSan suppressions that these changes seem to allow. There is a third one at the top of #21000; <strike>I didn't reproduce the warning but may still look into it.</strike> -> Edit: done.

Removing the argument seems out of scope of this change

Why? If the purpose is to silence the warning, removing the argument will achieve that (+ remove some dead code).

With current master we have wrong behavior (e.g. treat year 2110 as year 1974) and a warning due to that. With this PR in its current form (523c26d) we have wrong behavior (same as in master), no warning now from fuzzing and no warning ever, even in year 2110.

Why? If the purpose is to silence the warning, removing the argument will achieve that (+ remove some dead code).

Would it not just replace one int64 value for another int64 being passed into a uint32?

Updated per @MarcoFalke's #22094 (review) to raise if the file compat is corrupt.

Would it not just replace one int64 value for another int64 being passed into a uint32?

Correct. But that new int64 (returned by GetAdjustedTime()) will not overflow the uint32_t range before year 2106 and after that we would get a warning as deserved.

The underlying problem is that the code is not ready for year>2106 and the fuzzer tests with such years. Given that it is difficult to fix the code, I think it would be better to make the fuzzer not test with years>2106 (removing the second argument of CAddrMan::Connected() will achieve that).

@vasild I added your proposed change at the top as a commit authored by you (though it doesn't fix the UBSan error for me) and added TODO comments regarding updates needed by the Year 2106.

Hopefully this makes the issue more explicit in the code and documentation.

Also fixed the remaining addrman issues listed in #21000 that were suppressed.

A final commit removes all four of the UBSan addrman suppressions.

Edit: hm, seeing new UBSan errors in both the CI and locally. Looking.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22740 ([addrman] Move serialization code to cpp by amitiuttarwar)

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.

I see now why removing the second argument of Connected() (which comes from the fuzzer and could contain year >2106) does not fix the warning - I was thinking that GetAdjustedTime() would return "now" and since "now" is year 2021, we would not get an overflow. However the fuzzer has set mocked time which could be year >2106, so GetAdjustedTime() could overflow uint32_t too.

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

Needs rebase if still relevant

Concept ACK

What is the state of this PR? The ubsan suppressions it set out to fix/remove no-longer exist in master (#24302), so I don't know if it's still relevant.

Will update or close.

Will update or close.

Feel free to reopen whenever you get a chance to look at this again, and/or determine if any of the changes are still relevant. Going to close for now, as this has needed rebase for 7 months, and is currently neither reviewable or mergeable.