net: signed-integer-overflow in LocalServiceInfo #24049

1. 
   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.
2. Crypt-iQ added the label Bug on Jan 12, 2022
3. MarcoFalke added the label P2P on Jan 12, 2022
4. 
   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?
5. 
   Crypt-iQ commented at 4:39 pm on August 8, 2022: contributor

   I won’t have time to work on this unfortunately
6. fanquake added the label Up for grabs on Aug 9, 2022
7. 
   fanquake commented at 9:20 am on August 9, 2022: member

   Ok. Labelling this “Up for grabs” for now.
8. 
   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

   0++reinterpret_cast <unsigned&> (it->second.nScore);
   

   It’s ugly, I know… But it leaves everything else in this repo untouched.

9. ViralTaco referenced this in commit 474504fa4a on Oct 27, 2022
10. 
    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.

11. 
    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.
12. DevAgrawal1112 referenced this in commit e348e4e18e on Apr 1, 2023
13. fanquake removed the label Up for grabs on Apr 3, 2023

Contributors
Crypt-iQ fanquake ViralTaco DevAgrawal1112

Labels
Bug P2P