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

jonatack · 2021-05-28T14:04:39Z

This patch

drops an unused nTime param from CAddrMan::Connected_() and CAddrMan::Connected()
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.)

does the same for two similar UBSan errors in CAddrMan::Add_() that were suppressed
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)

removes the four addrman UBSan suppressions
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

lsilva01

Code review ACK ffd89f1

klementtan

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

maflcko · 2021-06-01T09:12:30Z

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

maflcko · 2021-06-01T09:14:44Z

See also bitcoin/bips#967 (comment)

jonatack · 2021-06-01T09:48:32Z

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.

jonatack · 2021-06-01T10:12:15Z

Updated the PR title.

maflcko · 2021-06-01T10:13:24Z

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.

maflcko · 2021-06-01T10:14:55Z

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

vasild · 2021-07-28T11:28:23Z

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

[patch] remove nTime argument

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[in]   addr     The address of the peer we were connected to
-     * @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));

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.

jonatack · 2021-07-30T09:43:52Z

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; ~~I didn't reproduce the warning but may still look into it.~~ -> Edit: done.

src/addrman.h

vasild · 2021-07-30T12:49:11Z

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.

jonatack · 2021-07-30T13:05:21Z

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?

jonatack · 2021-07-30T15:46:06Z

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

vasild · 2021-08-02T09:24:35Z

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

jonatack · 2021-08-10T14:12:36Z

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

CAddress::nTime is declared in src/protocol.h as uint32 but in CAddrMan::Connected_() it needs to be converted to int64 for subtraction from an int64 to avoid unsigned integer overflow, and int64 values can be written to it with implicit integer truncation. Fixes two UBSan errors reported by the addrman fuzzer - a currently suppressed error addrman.cpp:594:13: runtime error: unsigned integer overflow: 513 - 100000000 cannot be represented in type 'unsigned int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior addrman.cpp:594:13 in - a non-suppressed error addrman.cpp:595: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) SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior addrman.cpp:595:22

CAddress::nTime is declared in src/protocol.h as uint32, but in CAddrMan::Add_() int64 values can be written to it with implicit integer truncation, and to avoid unsigned integer overflow errors nTime needs to be converted to int64 for int64 values to be subtracted from it. Fixes these currently suppressed UBSan errors (addrman fuzzer) - unsigned integer overflow - implicit-signed-integer-truncation

and raise if the file compat is corrupt. Found by the addrman fuzzer with UBSan: 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)

DrahtBot · 2021-08-13T17:14:35Z

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

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.

vasild

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.

vasild · 2021-08-23T08:23:46Z

src/addrman.cpp

+    const int64_t nUpdateInterval{20 * 60};
+    const int64_t now{GetAdjustedTime()};
+    // TODO: change CAddress::nTime from uint32 to int64 before the Year 2106.
+    if (now - static_cast<int64_t>(info.nTime) > nUpdateInterval) {
+        info.nTime = static_cast<uint32_t>(now);
+    }


This warrants an assert that now, when cast to uint32_t will not overflow, i.e. year 2106 be silently converted to year 1970:

assert(now <= std::numeric_limits<uint32>::max()); info.nTime = static_cast<uint32_t>(now);

vasild · 2021-08-23T08:30:39Z

src/addrman.cpp

-            pinfo->nTime = std::max((int64_t)0, addr.nTime - nTimePenalty);
-
+        if (addr.nTime && (!pinfo->nTime || pinfo->nTime < static_cast<int64_t>(addr.nTime) - nUpdateInterval - nTimePenalty)) {
+            pinfo->nTime = std::max<uint32_t>(0, static_cast<int64_t>(addr.nTime) - nTimePenalty);


This will implicitly convert the second argument of std::max from int64_t to uint32_t:

int64_t x = 10; int64_t y = 14; std::cout << std::max<uint32_t>(0, x - y) << std::endl;

produces this warning (with both gcc and clang; that particular warning switch is not turned on in Bitcoin Core):

warning: implicit conversion loses integer precision: 'long' to 'const unsigned int' [-Wshorten-64-to-32] std::cout << std::max<uint32_t>(0, x - y) << std::endl; ~~~ ~~^~~

and prints 4294967292 instead of the intended 0.

I would go for KISS:

if (addr.nTime > nTimePenalty) { pinfo->nTime = static_cast<uint32_t>(addr.nTime - nTimePenalty); } else { pinfo->nTime = 0; }

DrahtBot · 2021-09-01T03:11:56Z

🐙 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".}

maflcko · 2021-10-22T12:04:40Z

Needs rebase if still relevant

theStack · 2021-12-21T17:58:40Z

Concept ACK

fanquake · 2022-04-20T09:55:27Z

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.

jonatack · 2022-04-20T11:33:26Z

Will update or close.

fanquake · 2022-04-29T11:46:41Z

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.

fanquake added the P2P label May 28, 2021

lsilva01 approved these changes May 29, 2021

View reviewed changes

klementtan approved these changes Jun 1, 2021

View reviewed changes

maflcko added the Refactoring label Jun 1, 2021

jonatack changed the title ~~p2p: fix UBSan implicit conversion error in CAddrMan::Connected_()~~ p2p, refactor: make nTime truncation conversion explicit in CAddrMan::Connected_() Jun 1, 2021

jonatack force-pushed the fix-ubsan-implicit-conversion-in-CAddrMan_Connected branch 2 times, most recently from 6ce4c84 to 523c26d Compare July 30, 2021 06:45

jonatack changed the title ~~p2p, refactor: make nTime truncation conversion explicit in CAddrMan::Connected_()~~ p2p: fix ubsan addrman errors, make nTime truncation conversion explicit Jul 30, 2021

maflcko reviewed Jul 30, 2021

View reviewed changes

src/addrman.h Outdated Show resolved Hide resolved

jonatack force-pushed the fix-ubsan-implicit-conversion-in-CAddrMan_Connected branch from 523c26d to 1bf84f6 Compare July 30, 2021 15:46

jonatack force-pushed the fix-ubsan-implicit-conversion-in-CAddrMan_Connected branch from 1bf84f6 to 241817f Compare August 10, 2021 14:07

vasild and others added 5 commits August 10, 2021 20:20

p2p: drop unused nTime param from CAddrMan::Connected()

14119e9

fuzz: remove addrman ubsan integer suppressions

2706a9d

jonatack force-pushed the fix-ubsan-implicit-conversion-in-CAddrMan_Connected branch from 241817f to 2706a9d Compare August 10, 2021 20:29

DrahtBot mentioned this pull request Aug 13, 2021

addrman: Remove CAddrMan::Clear() function #22697

Merged

DrahtBot mentioned this pull request Aug 21, 2021

[addrman] Move serialization code to cpp #22740

Merged

vasild reviewed Aug 23, 2021

View reviewed changes

DrahtBot added the Needs rebase label Sep 1, 2021

fanquake closed this Apr 29, 2022

bitcoin locked and limited conversation to collaborators Apr 29, 2023

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

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

Uh oh!

Conversation

jonatack commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

klementtan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 1, 2021

Uh oh!

maflcko commented Jun 1, 2021

Uh oh!

jonatack commented Jun 1, 2021

Uh oh!

jonatack commented Jun 1, 2021

Uh oh!

maflcko commented Jun 1, 2021

Uh oh!

maflcko commented Jun 1, 2021

Uh oh!

vasild commented Jul 28, 2021

Uh oh!

jonatack commented Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vasild commented Jul 30, 2021

Uh oh!

jonatack commented Jul 30, 2021

Uh oh!

jonatack commented Jul 30, 2021

Uh oh!

vasild commented Aug 2, 2021

Uh oh!

jonatack commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

vasild Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Sep 1, 2021

Uh oh!

maflcko commented Oct 22, 2021

Uh oh!

theStack commented Dec 21, 2021

Uh oh!

fanquake commented Apr 20, 2022

Uh oh!

jonatack commented Apr 20, 2022

Uh oh!

fanquake commented Apr 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jonatack commented May 28, 2021 •

edited

Loading

klementtan left a comment •

edited

Loading

jonatack commented Jul 30, 2021 •

edited

Loading

jonatack commented Aug 10, 2021 •

edited

Loading

DrahtBot commented Aug 13, 2021 •

edited

Loading