net: signed-integer-overflow in LocalServiceInfo #24049

issue Crypt-iQ opened this issue on January 12, 2022

Crypt-iQ commented at 4:12 PM on January 12, 2022: contributor

LocalServiceInfo is defined as: https://github.com/bitcoin/bitcoin/blob/16781e1bc9f8ffc721ebea73434e0066957bc959/src/net.h#L228-L231 nScore is of type int which is signed and is 4 bytes wide on my machine (clang-12). nScore is incremented by SeenLocal during the version handshake here: https://github.com/bitcoin/bitcoin/blob/16781e1bc9f8ffc721ebea73434e0066957bc959/src/net_processing.cpp#L2603-L2606 So I believe this UB is peer-triggerable (I don't think the compiler can detect this), but I don't think it's a big deal unless there's a node out there compiled with UBSAN or ftrapv. It seems like nScore can just be changed to int64_t.
Crypt-iQ added the label Bug on Jan 12, 2022
MarcoFalke added the label P2P on Jan 12, 2022
fanquake commented at 4:37 PM on August 8, 2022: member

#24090 was closed with no comment. @Crypt-iQ do you want to follow up here?
Crypt-iQ commented at 4:39 PM on August 8, 2022: contributor

I won't have time to work on this unfortunately
fanquake added the label Up for grabs on Aug 9, 2022
fanquake commented at 9:20 AM on August 9, 2022: member

Ok. Labelling this "Up for grabs" for now.
ViralTaco commented at 7:49 PM on September 29, 2022: none
I have a potential fix… It's ugly, though. And it looks like it should be UB, but I can't find a reason why.
The function which may invoke UB is (currently) defined like so: https://github.com/bitcoin/bitcoin/blob/5b6f0f31fa6ce85db3fb7f9823b1bbb06161ae32/src/net.cpp#L344-L352
```
++reinterpret_cast <unsigned&> (it->second.nScore);
```
It's ugly, I know… But it leaves everything else in this repo untouched.
ViralTaco referenced this in commit 474504fa4a on Oct 27, 2022
ViralTaco commented at 3:23 PM on November 24, 2022: none

Replying to #26399 (comment):

The burden to […] motivate a change […] is on the author.

The patch makes the behavior of the code defined. (See: [intro.compliance.general], [intro.abstract]) It should do so, without any conversions.

I spent over a hundred hours trying to prove it couldn't be done. First, I asked more senior engineers, chatted llvm devs to know it could [be an asan bug], then finally posted on stackoverflow.

You […] ignored previous discussions, ignored two review comments

I did? I'll have to look into that. I agree with the rest of your comment, though.

I have very little free time, and this isn't how I want to spend it. Please don't mistake my frustration for arrogance.
DevAgrawal1112 commented at 5:23 AM on April 1, 2023: none

Since I am new to open source, I would like to know if I need to change int to int64_t for nScore or if I have to work on something else.
DevAgrawal1112 referenced this in commit e348e4e18e on Apr 1, 2023
fanquake removed the label Up for grabs on Apr 3, 2023

Contributors

Labels

Linked (view graph)

#24090 net/p2p: change nScore and nBestScore data types to int64_t #26399 Fix #24049: signed integer overflow in `SeenLocal`#33072 [p2p] Fix signed integer overflow in LocalServiceInfo::nScore #34028 p2p: Prevent integer overflow in LocalServiceInfo::nScore