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
promag cross-referenced this on May 13, 2021 from issue refactor: Replace memset calls with array initialization by promag
laanwj referenced this in commit b34bf2b42c on May 13, 2021
sidhujag referenced this in commit 3b43f88056 on May 13, 2021
barton2526 cross-referenced this on Nov 22, 2021 from issue net: initialize nMessageSize to uint32_t max by barton2526
barton2526 cross-referenced this on Mar 3, 2022 from issue refactor: Replace memset calls with array initialization by barton2526
gwillen referenced this in commit a7702d6ab5 on Jun 1, 2022
bitcoin locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#21939 refactor: Replace memset calls with array initialization