Do the SOCKS5 handshake reliably #28649
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
concept ACK 608f8aa 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 |
|
Concept ACK. In 70917bd would it make sense to use |
src/util/sock.h
Outdated
| /** | ||
| * Convenience method, equivalent to `SendComplete(data.data(), data.size(), timeout, interrupt)`. | ||
| */ | ||
| virtual void SendComplete(const std::vector<uint8_t>& data, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
could update the headers a little
--- 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;
+
src/util/sock.h
Outdated
| /** | ||
| * Convenience method, equivalent to `SendComplete(data.data(), data.size(), timeout, interrupt)`. | ||
| */ | ||
| virtual void SendComplete(const std::vector<uint8_t>& data, |
There was a problem hiding this comment.
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:
could update the headers a little
--- 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;
+
|
Concept ACK. Good catch! |
This would make it easier to pass other than `std::string` types, to be used in the `Socks5()` function.
`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).
vasild
force-pushed
the
reliable_socks5_handshake
branch
from
608f8aa
to
af0fca5
Compare
|
|
| /** | ||
| * Convenience method, equivalent to `SendComplete(MakeUCharSpan(data), timeout, interrupt)`. | ||
| */ | ||
| virtual void SendComplete(Span<const char> data, | ||
| std::chrono::milliseconds timeout, | ||
| CThreadInterrupt& interrupt) const; |
There was a problem hiding this comment.
The first commit could be smaller and simpler/clearer (feel free to ignore).
@@ -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 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);
+ }There was a problem hiding this comment.
I prefer to keep all implementation in one place and to keep the interface separate from the implementation.
There was a problem hiding this comment.
ACK af0fca5
Built and ran tests locally. Confirmed the changes since my last ACK are using Span instead of data and len
Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmVC0dcACgkQ5+KYS2KJ
yToWvw/+JIyhSZGWsQz2aeiSo/NPEFUShQLyI6MB/dNiotx/WvbzwR5PkUWzg067
hJnFgdo3sS89WTLDoqpewspXgXsDcWyhaKBep8jifnxAWow+jw+DdjZO9UqnJZwN
g4T/URcXWVjYyIQY4C9OftUxfuRmIK8cUk6BDGSDNg2dxhdcaoez3DjcUHJuFFRa
vL/b96E9wuT3qHBXlCMvjvVE/hVOeSXk7sjGuGZMgtPsYl2TdH9VF1WSGh6rIAD6
Tf70Gf3smCj8samcs+TWvifqD4DTS4186Pu6F3P9WdVeyI+nZxirU2hizeQTRzmN
a8f4YcGDHNuHHSnX2es7ZelDWqfl0DfyrOVSiYr5Llac/R3Jwpb05lLw9tCktby6
AOW1Z8y7i3BAQfK6fLqJubGksr0JPiYs7HO0g3Funeu8mId7LZsSqXd6LRdgfmRj
/C8b+SlDFPd2VNa2r/X46hlBiIw+UbQ6CxO/IEkH6KbMrM7ybi3GsSrknclh9pZ3
ToUdhEkZJLjiO/O0gxQ+W+5aSJI6m79ReSndFxnbC0WHJgAz2osyxaDgr/Fljdyp
IHY0D4UjPMUVLAHwGRgbbGeePgYyO8CshI+78rIxTbbAoBS7QU0O+nDhWtd3Bfv4
+rFQnZQ8cB/J5fiINYj+T6DNmNso8IU2US4bVLvpVCyxc7rsIis=
=WJhV
-----END PGP SIGNATURE-----
pinheadmz's public key is on keybase
|
ACK af0fca5 |
This would make it easier to pass other than `std::string` types, to be used in the `Socks5()` function. Github-Pull: bitcoin#28649 Rebased-From: 1b19d11
`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). Github-Pull: bitcoin#28649 Rebased-From: af0fca5
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. ChangeSocks5()to do that. There is already a methodSock::SendComplete()which does exactly that, so use it inSocks5().A minor complication for this PR is that
Sock::SendComplete()takesstd::stringargument whereasSocks5()hasstd::vector<uint8_t>. Thus the necessity for the first commit. It is possible to do also in other ways - convert the data inSocks5()tostd::stringor have just oneSock::SendComplete()that takesvoid*and change the callers to passstr.data(), str.size()orvec.data(), vec.size().This came up while testing #27375.