Do the SOCKS5 handshake reliably #28649

pull vasild wants to merge 2 commits into bitcoin:master from vasild:reliable_socks5_handshake changing 6 files +120 −112
  1. vasild commented at 2:35 pm on October 13, 2023: contributor

    The Socks5() function which does the SOCKS5 handshake with the SOCKS5 proxy sends bytes to the socket without retrying partial writes.

    send(2) may write only part of the provided data and return. In this case the caller is responsible for retrying the operation with the remaining data. Change Socks5() to do that. There is already a method Sock::SendComplete() which does exactly that, so use it in Socks5().

    A minor complication for this PR is that Sock::SendComplete() takes std::string argument whereas Socks5() has std::vector<uint8_t>. Thus the necessity for the first commit. It is possible to do also in other ways - convert the data in Socks5() to std::string or have just one Sock::SendComplete() that takes void* and change the callers to pass str.data(), str.size() or vec.data(), vec.size().

    This came up while testing #27375.

  2. DrahtBot commented at 2:35 pm on October 13, 2023: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK jonatack, pinheadmz, achow101
    Concept ACK laanwj

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28690 (build: Introduce internal kernel library by TheCharlatan)

    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.

  3. pinheadmz commented at 8:12 pm on October 16, 2023: member

    concept ACK 608f8aa1c472da81f0c2b0db37be7dbcc932582a

    Reviewed all code changes and built locally. I don’t see anything wrong with this simple update, it makes a lot of sense and is implemented cleanly. Later this week I’ll try to test it if I can, I know its covering a sort of intermittent failure but there might be a way to see it work in action, maybe in combination with #27375

  4. jonatack commented at 8:34 pm on October 16, 2023: contributor
    Concept ACK. In 70917bdb18035530b would it make sense to use Span?
  5. in src/util/sock.h:247 in 608f8aa1c4 outdated
    242                               CThreadInterrupt& interrupt) const;
    243 
    244+    /**
    245+     * Convenience method, equivalent to `SendComplete(data.data(), data.size(), timeout, interrupt)`.
    246+     */
    247+    virtual void SendComplete(const std::vector<uint8_t>& data,
    


    vasild commented at 9:53 am on October 17, 2023:

    moving a discussion from the main thread here

    would it make sense to use Span?

    Maybe. What would be the purpose? To unite the two SendComplete(const std::string& and SendComplete(const std::vector<uint8_t>& into one method?


    jonatack commented at 3:31 am on October 25, 2023:

    Yes, possibly, when I see range-like objects with data and size attributes, it reminds me of Span and this section from src/span.h:

    0 * - Due to Span's automatic creation from range-like objects (arrays, and data
    1 *   types that expose a data() and size() member function), functions that
    2 *   accept a Span as input parameter can be called with any compatible
    3 *   range-like object. For example, this works:
    

    Feel free to ignore that and the following:

     0--- a/src/test/fuzz/socks5.cpp
     1+++ b/src/test/fuzz/socks5.cpp
     2@@ -2,13 +2,12 @@
     3 // Distributed under the MIT software license, see the accompanying
     4 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
     5 
     6-#include <netaddress.h>
     7 #include <netbase.h>
     8 #include <test/fuzz/FuzzedDataProvider.h>
     9 #include <test/fuzz/fuzz.h>
    10-#include <test/fuzz/util.h>
    11 #include <test/fuzz/util/net.h>
    12 #include <test/util/setup_common.h>
    13+#include <util/threadinterrupt.h>
    14
    15--- a/src/util/sock.cpp
    16+++ b/src/util/sock.cpp
    17@@ -2,7 +2,6 @@
    18 
    19-#include <common/system.h>
    20 #include <compat/compat.h>
    21
    22--- a/src/util/sock.h
    23+++ b/src/util/sock.h
    24@@ -6,7 +6,6 @@
    25 
    26 #include <compat/compat.h>
    27-#include <util/threadinterrupt.h>
    28 #include <util/time.h>
    29 
    30 @ -14,6 +13,8 @@
    31 #include <unordered_map>
    32 
    33+class CThreadInterrupt;
    34+
    
  6. jonatack commented at 3:48 am on October 25, 2023: contributor
    ACK 608f8aa1c472da81f0c2b0db37be7dbcc932582a
  7. laanwj commented at 2:43 pm on October 31, 2023: member
    Concept ACK. Good catch!
  8. laanwj added the label P2P on Oct 31, 2023
  9. sock: change Sock::SendComplete() to take Span
    This would make it easier to pass other than `std::string` types,
    to be used in the `Socks5()` function.
    1b19d1117c
  10. netbase: use reliable send() during SOCKS5 handshake
    `send(2)` can be interrupted or for another reason it may not fully
    complete sending all the bytes. We should be ready to retry the send
    with the remaining bytes. This is what `Sock::SendComplete()` does,
    thus use it in `Socks5()`.
    
    Since `Sock::SendComplete()` takes a `CThreadInterrupt` argument,
    change also the recv part of `Socks5()` to use `CThreadInterrupt`
    instead of a boolean.
    
    Easier reviewed with `git show -b` (ignore white-space changes).
    af0fca530e
  11. vasild force-pushed on Oct 31, 2023
  12. vasild commented at 5:27 pm on October 31, 2023: contributor
    608f8aa1c4...af0fca530e: use a Span for Sock::SendComplete(), looks better, thanks for the suggestion.
  13. in src/util/sock.h:240 in af0fca530e
    236+    /**
    237+     * Convenience method, equivalent to `SendComplete(MakeUCharSpan(data), timeout, interrupt)`.
    238+     */
    239+    virtual void SendComplete(Span<const char> data,
    240                               std::chrono::milliseconds timeout,
    241                               CThreadInterrupt& interrupt) const;
    


    jonatack commented at 7:48 pm on October 31, 2023:
     0@@ -283,13 +283,6 @@ void Sock::SendComplete(Span<const unsigned char> data,
     1     }
     2 }
     3 
     4-void Sock::SendComplete(Span<const char> data,
     5-                        std::chrono::milliseconds timeout,
     6-                        CThreadInterrupt& interrupt) const
     7-{
     8-    SendComplete(MakeUCharSpan(data), timeout, interrupt);
     9-}
    10-
    11 std::string Sock::RecvUntilTerminator(uint8_t terminator,
    12                                       std::chrono::milliseconds timeout,
    13                                       CThreadInterrupt& interrupt,
    14diff --git a/src/util/sock.h b/src/util/sock.h
    15index 65e7ffc1650..17bab33feaa 100644
    16--- a/src/util/sock.h
    17+++ b/src/util/sock.h
    18@@ -228,16 +228,11 @@ public:
    19      * [@throws](/bitcoin-bitcoin/contributor/throws/) std::runtime_error if the operation cannot be completed. In this case only some of
    20      * the data will be written to the socket.
    21      */
    22-    virtual void SendComplete(Span<const unsigned char> data,
    23-                              std::chrono::milliseconds timeout,
    24-                              CThreadInterrupt& interrupt) const;
    25-
    26-    /**
    27-     * Convenience method, equivalent to `SendComplete(MakeUCharSpan(data), timeout, interrupt)`.
    28-     */
    29-    virtual void SendComplete(Span<const char> data,
    30-                              std::chrono::milliseconds timeout,
    31-                              CThreadInterrupt& interrupt) const;
    32+    virtual void SendComplete(Span<const unsigned char> data, std::chrono::milliseconds timeout, CThreadInterrupt& interrupt) const;
    33+    virtual void SendComplete(Span<const char> data, std::chrono::milliseconds timeout, CThreadInterrupt& interrupt) const
    34+    {
    35+        SendComplete(MakeUCharSpan(data), timeout, interrupt);
    36+    }
    

    vasild commented at 11:59 am on November 1, 2023:
    I prefer to keep all implementation in one place and to keep the interface separate from the implementation.
  14. jonatack commented at 7:50 pm on October 31, 2023: contributor
    ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78
  15. DrahtBot requested review from laanwj on Oct 31, 2023
  16. DrahtBot requested review from pinheadmz on Oct 31, 2023
  17. pinheadmz approved
  18. pinheadmz commented at 10:33 pm on November 1, 2023: member

    ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78

    Built and ran tests locally. Confirmed the changes since my last ACK are using Span instead of data and len

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA256
     2
     3ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmVC0dcACgkQ5+KYS2KJ
     7yToWvw/+JIyhSZGWsQz2aeiSo/NPEFUShQLyI6MB/dNiotx/WvbzwR5PkUWzg067
     8hJnFgdo3sS89WTLDoqpewspXgXsDcWyhaKBep8jifnxAWow+jw+DdjZO9UqnJZwN
     9g4T/URcXWVjYyIQY4C9OftUxfuRmIK8cUk6BDGSDNg2dxhdcaoez3DjcUHJuFFRa
    10vL/b96E9wuT3qHBXlCMvjvVE/hVOeSXk7sjGuGZMgtPsYl2TdH9VF1WSGh6rIAD6
    11Tf70Gf3smCj8samcs+TWvifqD4DTS4186Pu6F3P9WdVeyI+nZxirU2hizeQTRzmN
    12a8f4YcGDHNuHHSnX2es7ZelDWqfl0DfyrOVSiYr5Llac/R3Jwpb05lLw9tCktby6
    13AOW1Z8y7i3BAQfK6fLqJubGksr0JPiYs7HO0g3Funeu8mId7LZsSqXd6LRdgfmRj
    14/C8b+SlDFPd2VNa2r/X46hlBiIw+UbQ6CxO/IEkH6KbMrM7ybi3GsSrknclh9pZ3
    15ToUdhEkZJLjiO/O0gxQ+W+5aSJI6m79ReSndFxnbC0WHJgAz2osyxaDgr/Fljdyp
    16IHY0D4UjPMUVLAHwGRgbbGeePgYyO8CshI+78rIxTbbAoBS7QU0O+nDhWtd3Bfv4
    17+rFQnZQ8cB/J5fiINYj+T6DNmNso8IU2US4bVLvpVCyxc7rsIis=
    18=WJhV
    19-----END PGP SIGNATURE-----
    

    pinheadmz’s public key is on keybase

  19. achow101 commented at 7:11 pm on November 7, 2023: member
    ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78
  20. achow101 merged this on Nov 7, 2023
  21. achow101 closed this on Nov 7, 2023

  22. vasild deleted the branch on Apr 19, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me