net: initialize nMessageSize to uint32_t max #21905

pull Crypt-iQ wants to merge 1 commits into bitcoin:master from Crypt-iQ:cmessageheader_signchange_05102021 changing 3 files +2 −3

Crypt-iQ commented at 2:24 PM on May 10, 2021: contributor

nMessageSize is uint32_t and is set to -1. This will warn with -fsanitize=implicit-integer-sign-change when V1TransportDeserializer calls into the ctor. This pull initializes nMessageSize to numeric_limits<uint32_t>::max() instead and removes the ubsan suppression.
fanquake added the label P2P on May 10, 2021
in src/protocol.cpp:94 in 1abc466968 outdated
```
  90 | @@ -91,7 +91,7 @@ CMessageHeader::CMessageHeader()
  91 |  {
  92 |      memset(pchMessageStart, 0, MESSAGE_START_SIZE);
  93 |      memset(pchCommand, 0, sizeof(pchCommand));
  94 | -    nMessageSize = -1;
  95 | +    nMessageSize = 0;
```
laanwj commented at 2:51 PM on May 10, 2021:

Good catch. Haven't checked if changing from -1 to 0 has no impact (but I guess not), but if we're changing this anyway, maybe move initialization to the class definition

class CMessageHeader { ⋮ uint32_t nMessageSize{0}; ⋮ }

Not sure if the same is possible for the arrays but that would be even better.
Crypt-iQ commented at 2:27 PM on May 12, 2021:

I think I can tackle this in another PR, thanks for the suggestion
MarcoFalke commented at 2:52 PM on May 10, 2021: member

Please remove the suppression if the goal of this pull is to fix it
practicalswift commented at 7:31 PM on May 10, 2021: contributor

Concept ACK

Last time I checked (early April) this was the only remaining -fsanitize=integer warning in protocol.cpp. In other words you should be able to remove the suppression for protocol.cpp as part of this PR :)

To limit the scope of this PR and to guarantee preservation of current behaviour I would suggest keeping std::numeric_limits<uint32_t>::max() as the initial value. (Not claiming that changing to 0 would change behaviour: I haven't check.)

As laanwj mentioned in-class member initialization would be nice.

net: initialize nMessageSize to max uint32_t instead of -1

nMessageSize is uint32_t and is set to -1. This will warn with
-fsanitize=implicit-integer-sign-change.

9c891b64ff

Crypt-iQ force-pushed on May 11, 2021
Crypt-iQ renamed this:
~~net: set nMessageSize to 0 in CMessageHeader ctor~~
net: initialize nMessageSize to uint32_t max
on May 11, 2021
Crypt-iQ force-pushed on May 11, 2021
Crypt-iQ force-pushed on May 11, 2021
promag commented at 5:57 PM on May 11, 2021: member

Code review ACK 9c891b64ffd14bc8216dbd5eb60816043af265b6.

Here or follow up it could even drop the constructor and ditch memset calls and just intialize arrays like char pchCommand[COMMAND_SIZE]{};
MarcoFalke added the label Refactoring on May 11, 2021
laanwj commented at 12:34 PM on May 12, 2021: member

Code review ACK 9c891b64ffd14bc8216dbd5eb60816043af265b6

Here or follow up it could even drop the constructor and ditch memset calls and just intialize arrays like char pchCommand[COMMAND_SIZE]{};

Agree, would be nice to do this, though I think this PR is good (and self-contained) as-is.
laanwj merged this on May 12, 2021
laanwj closed this on May 12, 2021
Crypt-iQ deleted the branch on May 12, 2021
sidhujag referenced this in commit 33755a2b4c on May 12, 2021
laanwj referenced this in commit b34bf2b42c on May 13, 2021
sidhujag referenced this in commit 3b43f88056 on May 13, 2021
gwillen referenced this in commit a7702d6ab5 on Jun 1, 2022
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#21939 refactor: Replace memset calls with array initialization