net: Use Span in ReceiveMsgBytes #20056

MarcoFalke commented at 3:57 PM on October 1, 2020: member

Pass a data pointer and a size as span in ReceiveMsgBytes to get the benefits of a span

MarcoFalke added the label Refactoring on Oct 1, 2020

MarcoFalke added the label P2P on Oct 1, 2020

MarcoFalke commented at 4:02 PM on October 1, 2020: member

Also, the doxygen comment is moved from the cpp file to the header, which can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

laanwj commented at 4:10 PM on October 1, 2020: member

Concept ACK

MarcoFalke force-pushed on Oct 1, 2020

jonatack commented at 5:39 PM on October 1, 2020: member

Concept ACK

practicalswift commented at 7:13 PM on October 1, 2020: contributor

Concept ACK

MarcoFalke force-pushed on Oct 2, 2020

in src/net.cpp:609 in faa8ad5888 outdated

 605 | @@ -606,34 +606,21 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap)
 606 |  }
 607 |  #undef X
 608 |  
 609 | -/**

laanwj commented at 1:52 PM on October 2, 2020:

+1, this comment for a method belongs in the header not in the implementation file.

in src/net.h:805 in faa8ad5888 outdated

 801 | @@ -802,9 +802,14 @@ class V1TransportDeserializer final : public TransportDeserializer
 802 |          hdrbuf.SetVersion(nVersionIn);
 803 |          vRecv.SetVersion(nVersionIn);
 804 |      }
 805 | -    int Read(const char *pch, unsigned int nBytes) override {
 806 | -        int ret = in_data ? readData(pch, nBytes) : readHeader(pch, nBytes);
 807 | -        if (ret < 0) Reset();
 808 | +    int Read(Span<const char>& msg_bytes) override

laanwj commented at 1:58 PM on October 2, 2020:

Might want to add a doc comment here that this function reads bytes from the span and advances the pointer. At least I first missed where the msg_bytes was advanced in the CNode::ReceiveMsgBytes loop.

MarcoFalke commented at 2:27 PM on October 2, 2020:

thx, done

laanwj approved

laanwj commented at 1:59 PM on October 2, 2020: member

Code review ACK faa8ad5888c82fbb636aa2bbbe722fc7d3c2b918

net: Use Span in ReceiveMsgBytes fa5ed3b4ca

MarcoFalke force-pushed on Oct 2, 2020

jonatack commented at 8:09 PM on October 10, 2020: member

ACK fa5ed3b4ca609426b2622cad235e107d33db7b30 code review, rebased to current master 12a1c3ad1a43634, debug build, unit tests, ran bitcoind/-netinfo/getpeerinfo

theStack approved

theStack commented at 3:04 PM on October 11, 2020: member

ACK fa5ed3b4ca609426b2622cad235e107d33db7b30 Reviewed the code changes and ran the tests locally. :sake:

In the future, I think it would be nice to have a MakeSpan-like helper that creates a (signed) char Span out of an unsigned char container (like MakeUCharSpan, but in the other direction). Then one wouldn't need to construct the Spans with .data()/.size() pairs in the tests. Don't know though how that fits into the C++20 std::spans and If it's not better to get rid of signed char in serializations completely.

DrahtBot commented at 12:09 PM on November 3, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18242 (Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 11:21 AM on November 19, 2020: member

In the future, I think it would be nice to have a MakeSpan-like helper that creates a (signed) char Span out of an unsigned char container (like MakeUCharSpan, but in the other direction)

Mind that char is not, necessarily, signed, this depends on the architecture's C ABI. And no matter, what, it is always seen as a separate type from both signed char and unsigned char. This difference between platforms tends to give a lot of subtle issues (including a security in the SOCKS5 code, historically) so I think it's best to use uint8_t as a byte type where remotely possible.

Then one wouldn't need to construct the Spans with .data()/.size() pairs in the tests

If it's test only I'd prefer not to introduce more wrapper functions for it. In general it should be enough to do this in one direction, to our standardized unsigned byte type.

MarcoFalke commented at 1:03 PM on November 19, 2020: member

(appveyor ci failure can be ignored)

practicalswift commented at 2:02 PM on November 19, 2020: contributor

Mind that char is not, necessarily, signed, this depends on the architecture's C ABI. And no matter, what, it is always seen as a separate type from both signed char and unsigned char. This difference between platforms tends to give a lot of subtle issues (including a security in the SOCKS5 code, historically) so I think it's best to use uint8_t as a byte type where remotely possible.

Just adding a reference: interested readers might want to check out this write-up on the SOCKS5 issue CVE-2017-18350 @laanwj mentions above.

FWIW, I agree 100% with what was said above. Or more explicitly: explicit (uint8_t) is better! :)

laanwj commented at 5:10 AM on November 20, 2020: member

re-ACK fa5ed3b4ca609426b2622cad235e107d33db7b30

laanwj merged this on Nov 20, 2020

laanwj closed this on Nov 20, 2020

MarcoFalke deleted the branch on Nov 20, 2020

sidhujag referenced this in commit 0c5249763b on Nov 20, 2020

Fabcien referenced this in commit 71149ac502 on Jan 11, 2022

DrahtBot locked this on Feb 15, 2022