Do the SOCKS5 handshake reliably #28649

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

Contributors
vasild DrahtBot pinheadmz jonatack laanwj achow101

Review Requested
laanwj

Labels
P2P