net: initialize nMessageSize to uint32_t max #21905
Conversation
src/protocol.cpp
Outdated
| memset(pchMessageStart, 0, MESSAGE_START_SIZE); | ||
| memset(pchCommand, 0, sizeof(pchCommand)); | ||
| nMessageSize = -1; | ||
| nMessageSize = 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think I can tackle this in another PR, thanks for the suggestion
|
Please remove the suppression if the goal of this pull is to fix it |
|
Concept ACK Last time I checked (early April) this was the only remaining To limit the scope of this PR and to guarantee preservation of current behaviour I would suggest keeping As laanwj mentioned in-class member initialization would be nice. |
nMessageSize is uint32_t and is set to -1. This will warn with -fsanitize=implicit-integer-sign-change.
Crypt-iQ
force-pushed
the
cmessageheader_signchange_05102021
branch
from
1abc466 to
9c891b6
Compare
Crypt-iQ
changed the title
Crypt-iQ
force-pushed
the
cmessageheader_signchange_05102021
branch
2 times, most recently
from
1abc466 to
9c891b6
Compare
|
Code review ACK 9c891b6
Agree, would be nice to do this, though I think this PR is good (and self-contained) as-is. |
1c9255c refactor: Replace memset calls with array initialization (João Barbosa) Pull request description: Follow up to #21905 (review). ACKs for top commit: laanwj: re-ACK 1c9255c Crypt-iQ: Code review ACK 1c9255c Tree-SHA512: 4b61dec2094f4781ef1c0427ee3bda3cfea12111274eebc7bc40a84f261d9c1681dd0860c57200bea2456588e44e8e0aecd18545c25f1f1250dd331ab7d05f28
…lization 1c9255c refactor: Replace memset calls with array initialization (João Barbosa) Pull request description: Follow up to bitcoin#21905 (review). ACKs for top commit: laanwj: re-ACK 1c9255c Crypt-iQ: Code review ACK 1c9255c Tree-SHA512: 4b61dec2094f4781ef1c0427ee3bda3cfea12111274eebc7bc40a84f261d9c1681dd0860c57200bea2456588e44e8e0aecd18545c25f1f1250dd331ab7d05f28
6 participants
nMessageSize is uint32_t and is set to -1. This will warn with
-fsanitize=implicit-integer-sign-changewhen V1TransportDeserializer calls into the ctor. This pull initializes nMessageSize tonumeric_limits<uint32_t>::max()instead and removes the ubsan suppression.