Do the SOCKS5 handshake reliably #28649

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.

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.

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

jonatack commented at 8:34 PM on October 16, 2023: member

Concept ACK. In 70917bdb18035530b would it make sense to use Span?

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:

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

Feel free to ignore that and the following:

<details><summary>could update the headers a little</summary><p>

--- a/src/test/fuzz/socks5.cpp
+++ b/src/test/fuzz/socks5.cpp
@@ -2,13 +2,12 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <netaddress.h>
 #include <netbase.h>
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
-#include <test/fuzz/util.h>
 #include <test/fuzz/util/net.h>
 #include <test/util/setup_common.h>
+#include <util/threadinterrupt.h>

--- a/src/util/sock.cpp
+++ b/src/util/sock.cpp
@@ -2,7 +2,6 @@
 
-#include <common/system.h>
 #include <compat/compat.h>

--- a/src/util/sock.h
+++ b/src/util/sock.h
@@ -6,7 +6,6 @@
 
 #include <compat/compat.h>
-#include <util/threadinterrupt.h>
 #include <util/time.h>
 
 @ -14,6 +13,8 @@
 #include <unordered_map>
 
+class CThreadInterrupt;
+

</p></details>

jonatack commented at 3:48 AM on October 25, 2023: member

ACK 608f8aa1c472da81f0c2b0db37be7dbcc932582a

laanwj commented at 2:43 PM on October 31, 2023: member

Concept ACK. Good catch!

laanwj added the label P2P on Oct 31, 2023

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

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

vasild force-pushed on Oct 31, 2023

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.

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:

<details><summary>The first commit could be smaller and simpler/clearer (feel free to ignore).</summary><p>

@@ -283,13 +283,6 @@ void Sock::SendComplete(Span<const unsigned char> data,
     }
 }
 
-void Sock::SendComplete(Span<const char> data,
-                        std::chrono::milliseconds timeout,
-                        CThreadInterrupt& interrupt) const
-{
-    SendComplete(MakeUCharSpan(data), timeout, interrupt);
-}
-
 std::string Sock::RecvUntilTerminator(uint8_t terminator,
                                       std::chrono::milliseconds timeout,
                                       CThreadInterrupt& interrupt,
diff --git a/src/util/sock.h b/src/util/sock.h
index 65e7ffc1650..17bab33feaa 100644
--- a/src/util/sock.h
+++ b/src/util/sock.h
@@ -228,16 +228,11 @@ public:
      * [@throws](/bitcoin-bitcoin/contributor/throws/) std::runtime_error if the operation cannot be completed. In this case only some of
      * the data will be written to the socket.
      */
-    virtual void SendComplete(Span<const unsigned char> data,
-                              std::chrono::milliseconds timeout,
-                              CThreadInterrupt& interrupt) const;
-
-    /**
-     * Convenience method, equivalent to `SendComplete(MakeUCharSpan(data), timeout, interrupt)`.
-     */
-    virtual void SendComplete(Span<const char> data,
-                              std::chrono::milliseconds timeout,
-                              CThreadInterrupt& interrupt) const;
+    virtual void SendComplete(Span<const unsigned char> data, std::chrono::milliseconds timeout, CThreadInterrupt& interrupt) const;
+    virtual void SendComplete(Span<const char> data, std::chrono::milliseconds timeout, CThreadInterrupt& interrupt) const
+    {
+        SendComplete(MakeUCharSpan(data), timeout, interrupt);
+    }

</p></details>

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.

jonatack commented at 7:50 PM on October 31, 2023: member

ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78

DrahtBot requested review from laanwj on Oct 31, 2023

DrahtBot requested review from pinheadmz on Oct 31, 2023

pinheadmz approved

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

<details><summary>Show Signature</summary>

pinheadmz's public key is on keybase

</details>

achow101 commented at 7:11 PM on November 7, 2023: member

ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78

achow101 merged this on Nov 7, 2023

achow101 closed this on Nov 7, 2023

vasild deleted the branch on Apr 19, 2024

luke-jr referenced this in commit 8ad7de4776 on Apr 24, 2024

luke-jr referenced this in commit 929796b230 on Apr 24, 2024

bitcoin locked this on Apr 19, 2025