P2P: Don't fill with zeros twice #15199

pull DesWurstes wants to merge 1 commits into bitcoin:master from DesWurstes:patch-8 changing 1 files +1 −2

DesWurstes commented at 10:31 AM on January 18, 2019: contributor

destination is padded with zeros until a total of num characters have been written to it.

from http://www.cplusplus.com/reference/cstring/strncpy/

Closes #15151

Protocol: Don't fill with zeros twice

and use COMMAND_SIZE rather than calculating it via sizeof.

dc2dcf8750

fanquake added the label P2P on Jan 18, 2019
sdaftuar commented at 11:42 AM on January 18, 2019: member

I prefer the readability and robustness of the existing code -- it seems error-prone to rely on behavior of strncpy that I think many developers may not be familiar with, in particular if that code were changed I could imagine reviewers overlooking the memset() issue.

I think my concern could be addressed by adding a comment above the call to strncpy and ensuring we have a test that would fail if this behavior were broken (is that already the case?).
laanwj commented at 2:40 PM on January 18, 2019: member

NACK.

I prefer the current code, too. I see no need to change this. See, this concerns a very small buffer, if ti'd be megabytes of memory and a potential performance impact it might be different.

Edit: note that I've actually seen issues in the wild where clients didn't properly clear the command field and it leaked a bit of memory. Better to be sure than sorry here.
DesWurstes commented at 2:44 PM on January 18, 2019: contributor

May I replace strncpy with memcpy then? That'd silence a -pedantic warning.
laanwj commented at 3:13 PM on January 18, 2019: member

Nah, I'd say, if you contribute, please make changes that fix user-facing issues. This just isn't worth the overhead IMO.
DesWurstes closed this on Jan 18, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels