net: add RAII socket and use it instead of bare SOCKET #20788

vasild · 2020-12-28T16:12:30Z

Introduce a class to manage the lifetime of a socket - when the object
that contains the socket goes out of scope, the underlying socket will
be closed.

In addition, the new Sock class has a Send(), Recv() and Wait()
methods that can be overridden by unit tests to mock the socket
operations.

The Wait() method also hides the
#ifdef USE_POLL poll() #else select() #endif technique from higher
level code.

vasild · 2020-12-28T16:18:16Z

The following two PRs would benefit from this:

net: Add regression fuzz harness for CVE-2017-18350. Add FuzzedSocket. #19203 would only need to add the fuzzing test and the mocked/fuzzed Sock implementation (last commit from that PR).
Add I2P support using I2P SAM #20685 would become smaller. Sock originated there and that PR contains a copy of it. So if this PR gets merged, then the size of Add I2P support using I2P SAM #20685 will be reduced.

maflcko

left a style comment, no other review. Feel free to ignore.

src/sock.cpp

jonatack

Concept ACK, was unable to build this pull @ 99b8e88

build error output

$ gcc --version
gcc (Debian 10.2.1-1) 10.2.1 20201207

.../...

  AR       qt/libbitcoinqt.a
/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-netbase.o): in function `std::_MakeUniq<Sock>::__single_object std::make_unique<Sock, unsigned int&>(unsigned int&)':
/usr/include/c++/10/bits/unique_ptr.h:962: undefined reference to `Sock::Sock(unsigned int)'
  CXXLD    qt/bitcoin-qt
  CXXLD    bitcoin-gui
/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-netbase.o): in function `std::_MakeUniq<Sock>::__single_object std::make_unique<Sock, unsigned int&>(unsigned int&)':
/usr/include/c++/10/bits/unique_ptr.h:962: undefined reference to `Sock::Sock(unsigned int)'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:5503: bitcoin-node] Error 1
make[2]: *** Waiting for unfinished jobs....
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:5515: bitcoind] Error 1
/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-netbase.o): in function `std::_MakeUniq<Sock>::__single_object std::make_unique<Sock, unsigned int&>(unsigned int&)':
/usr/include/c++/10/bits/unique_ptr.h:962: undefined reference to `Sock::Sock(unsigned int)'
/usr/bin/ld: libbitcoin_common.a(libbitcoin_common_a-netbase.o): in function `std::_MakeUniq<Sock>::__single_object std::make_unique<Sock, unsigned int&>(unsigned int&)':
/usr/include/c++/10/bits/unique_ptr.h:962: undefined reference to `Sock::Sock(unsigned int)'
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:5521: qt/bitcoin-qt] Error 1
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:5499: bitcoin-gui] Error 1
make[2]: Leaving directory '/home/jon/projects/bitcoin/bitcoin/src'
make[1]: *** [Makefile:14882: all-recursive] Error 1
make[1]: Leaving directory '/home/jon/projects/bitcoin/bitcoin/src'
make: *** [Makefile:806: all-recursive] Error 1

DrahtBot · 2020-12-28T17:07:35Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Make all of net_processing (and some of net) use std::chrono types #21015 (Make all of net_processing (and some of net) use std::chrono types by dhruv)
Add I2P support using I2P SAM #20685 (Add I2P support using I2P SAM by vasild)
net: Make DNS lookup mockable, add fuzzing harness #19415 (net: Make DNS lookup mockable, add fuzzing harness by practicalswift)
net: Add regression fuzz harness for CVE-2017-18350. Add FuzzedSocket. #19203 (net: Add regression fuzz harness for CVE-2017-18350. Add FuzzedSocket. Add thin SOCKET wrapper. by practicalswift)

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.

src/netbase.cpp

practicalswift · 2020-12-28T20:57:34Z

Strong concept ACK

This abstraction would be extremely useful to have when fuzzing low-level networking code.

vasild · 2020-12-30T11:37:19Z

99b8e88...408a947: moved the Sock class inside netbase.cpp to resolve a circular dependency.

Another option would be to keep the Sock class in its own file src/sock.cpp or src/util/sock.cpp. Since it uses MillisToTimeval() and CloseSocket() they would have to be moved out of netbase.cpp.

jonatack

Tested approach ACK 408a947 (modulo missing test coverage?)

moving the Sock class to its own file and the common deps to a utility seems appealing, at least in principle, not sure in practice
review of the diff would be easier and nicer, and there would be less unnecessary churn and risk of overlooking a regression, if unneeded formatting and line break changes were not added when only an argument in the function signature is being updated
do you plan to add unit tests for the Sock class?

src/netbase.h

lontivero

I've finished the review. Only two comments.

src/net.cpp

src/netbase.cpp

jnewbery · 2020-12-31T14:08:47Z

Concept ACK. Is there any reason not to use the Sock object in CNode?

vasild · 2020-12-31T14:14:38Z

408a947...8fc63e3: fixed a typo and removed unused parameter

vasild · 2020-12-31T14:35:28Z

@jonatack

moving the Sock class to its own file and the common deps to a utility seems appealing

I will try this. Opinions, anybody? That would mean:

Move Sock from netbase.cpp to util/sock.cpp
Move MillisToTimeval() from netbase.cpp to util/where?
Move CloseSocket() from netbase.cpp to util/where? (it uses LogPrintf())

do you plan to add unit tests for the Sock class?

I did not but any ideas are welcome. I think it may be required to create a TCP server and client and do some communication between them to check that send() is indeed being called by the Send() method. May be an overkill, given that the Send() method is 1 line.

I don't know how to reliably check if the socket was closed by the destructor, given that the same file descriptor number could be assigned to a newly created socket (or opened file) after we close it.

@jnewbery

Is there any reason not to use the Sock object in CNode?

No, other than minimizing the size of this PR. See also #20788 (comment) :-)

lontivero · 2020-12-31T16:23:14Z

I agree with moving it to its own file and with adding UTs for it. I am not sure a real socket is necessary for testing this abstraction, could it be enough to simply mock send, recv and select? Something it could be useful is to make sure the ownership of the SOCKET is always correct (copy ctor, Release, operator =)

btw, utACK 8fc63e3

vasild · 2021-01-08T13:24:36Z

8fc63e3...3fa2a11:

rebased due to conflicts
moved Sock to its dedicated src/util/sock.{cpp,h}
added unit tests

…SOCKET

This fixes: ```bash In file included from test/sock_tests.cpp:10: In file included from /usr/local/include/boost/test/unit_test.hpp:18: In file included from /usr/local/include/boost/test/test_tools.hpp:46: /usr/local/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Wsign-compare] return left == right; ~~~~ ^ ~~~~~ ``` which was introduced in bitcoin#20788.

practicalswift · 2021-02-12T09:36:14Z

@vasild Thanks for making our socket handling mockable. This will be extremely useful when fuzzing!

Similar to what you've done for TCP connections it would be really useful to be able to globally mock out DNS lookup to be able to have the responses be generated by a FuzzedDataProvider driven class instead of the ordinary lookup facility provided by the OS.

If you want to address this mocking need too you can find some inspiration in #19415 :)

vasild · 2021-02-13T13:11:01Z

Just for reference: previous incarnations of this PR had the Sock::Wait() method report to the caller whether one of the requested events happened or a timeout occurred. This was removed because none of the callers needed it.

However this is needed in #20685, so that PR contains a tiny commit to re-introduce that functionality.

9cc8e30 test: fix sign comparison warning in socket tests (fanquake) Pull request description: This fixes: ```bash In file included from test/sock_tests.cpp:10: In file included from /usr/local/include/boost/test/unit_test.hpp:18: In file included from /usr/local/include/boost/test/test_tools.hpp:46: /usr/local/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Wsign-compare] return left == right; ~~~~ ^ ~~~~~ ``` which was introduced in #20788. ACKs for top commit: practicalswift: cr ACK 9cc8e30 vasild: ACK 9cc8e30 Tree-SHA512: 7069a4fde5cec01be03f8477fe396e53658f170efbf1d9ef3339d553bb90a2be9f4acd6b348127b14cd2f91426e0cd1fc35d2d3c9f201cf748c0cf50f47e46a5

9cc8e30 test: fix sign comparison warning in socket tests (fanquake) Pull request description: This fixes: ```bash In file included from test/sock_tests.cpp:10: In file included from /usr/local/include/boost/test/unit_test.hpp:18: In file included from /usr/local/include/boost/test/test_tools.hpp:46: /usr/local/include/boost/test/tools/old/impl.hpp:107:17: warning: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Wsign-compare] return left == right; ~~~~ ^ ~~~~~ ``` which was introduced in bitcoin#20788. ACKs for top commit: practicalswift: cr ACK 9cc8e30 vasild: ACK 9cc8e30 Tree-SHA512: 7069a4fde5cec01be03f8477fe396e53658f170efbf1d9ef3339d553bb90a2be9f4acd6b348127b14cd2f91426e0cd1fc35d2d3c9f201cf748c0cf50f47e46a5

3174425 Cleanup headers after #20788 (Hennadii Stepanov) Pull request description: This is a header cleanup after #20788. ACKs for top commit: vasild: ACK 3174425 Tree-SHA512: 1c21b1ba43841880625289174f10e5b333f6eb857f448e1e4114b1ecdf32a6044ec91c5987c1d66806c1d408a4e3d46569eb41d69a0acb8296601d7c203d9f1d

3174425 Cleanup headers after bitcoin#20788 (Hennadii Stepanov) Pull request description: This is a header cleanup after bitcoin#20788. ACKs for top commit: vasild: ACK 3174425 Tree-SHA512: 1c21b1ba43841880625289174f10e5b333f6eb857f448e1e4114b1ecdf32a6044ec91c5987c1d66806c1d408a4e3d46569eb41d69a0acb8296601d7c203d9f1d

Summary: Move `MillisToTimeval()` from `netbase.{h,cpp}` to `src/util/system.{h,cpp}`. This is necessary in order to use `MillisToTimeval()` from a newly introduced `src/util/sock.{h,cpp}` which cannot depend on netbase because netbase will depend on it. This is a backport of [[bitcoin/bitcoin#20788 | [[bitcoin/bitcoin#20788 | core#20788]]]] [1/5] bitcoin/bitcoin@aa17a44 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11003

Summary: Move `CloseSocket()` (and `NetworkErrorString()` which it uses) from `netbase.{h,cpp}` to newly added `src/util/sock.{h,cpp}`. This is necessary in order to use `CloseSocket()` from a newly introduced Sock class (which will live in `src/util/sock.{h,cpp}`). `sock.{h,cpp}` cannot depend on netbase because netbase will depend on it. This is a backport of [[bitcoin/bitcoin#20788 | core#20788]] [2/5] bitcoin/bitcoin@dec9b5e Depends on D11003 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11004

Summary: Use the `Sock` class instead of `SOCKET` for `InterruptibleRecv()` and `Socks5()`. This way the `Socks5()` function can be tested by giving it a mocked instance of a socket. Co-authored-by: practicalswift <practicalswift@users.noreply.github.com> Note: Apply the same changes to seeder/bitcoin.cpp This is a backport of [[bitcoin/bitcoin#20788 | core#20788]] [4/5] bitcoin/bitcoin@04ae846 Depends on D11005 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11006

Summary: In the arguments of `InterruptibleRecv()`, `Socks5()` and `ConnectThroughProxy()` the variable `hSocket` was previously of type `SOCKET`, but has been changed to `Sock`. Thus rename it to `sock` to imply its type, to distinguish from other `SOCKET` variables and to abide to the coding style wrt variables' names. This is a backport of [[bitcoin/bitcoin#20788 | core#20788]] [5/5] bitcoin/bitcoin@7bd21ce Depends on D11006 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11007

vasild mentioned this pull request Dec 28, 2020

net: Add regression fuzz harness for CVE-2017-18350. Add FuzzedSocket. #19203

Merged

maflcko reviewed Dec 28, 2020

View reviewed changes

src/sock.cpp Outdated Show resolved Hide resolved

DrahtBot added Build system P2P labels Dec 28, 2020

jonatack reviewed Dec 28, 2020

View reviewed changes

This was referenced Dec 28, 2020

Add I2P support using I2P SAM #20685

Merged

refactor: Remove nMyStartingHeight from CNode/Connman #20649

Merged

lontivero reviewed Dec 28, 2020

View reviewed changes

src/netbase.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 28, 2020

net: Make DNS lookup mockable, add fuzzing harness #19415

Merged

vasild force-pushed the sock branch from 99b8e88 to 408a947 Compare December 30, 2020 11:32

jonatack reviewed Dec 30, 2020

View reviewed changes

src/netbase.h Outdated Show resolved Hide resolved

lontivero reviewed Dec 30, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/netbase.cpp Outdated Show resolved Hide resolved

vasild force-pushed the sock branch from 408a947 to 8fc63e3 Compare December 31, 2020 14:14

DrahtBot added the Needs rebase label Jan 2, 2021

vasild force-pushed the sock branch 2 times, most recently from 0b11ea2 to 3fa2a11 Compare January 8, 2021 13:22

DrahtBot removed the Needs rebase label Jan 8, 2021

This was referenced Jan 8, 2021

Move SaltedHashers to separate file and add some new ones #19935

Merged

wallet: Migrate legacy wallets to descriptor wallets #19602

Merged

refactor: Cleanup thread ctor calls #19064

Merged

fanquake removed the Build system label Jan 10, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 11, 2021

Merge bitcoin#20788: net: add RAII socket and use it instead of bare …

11681c6

…SOCKET

fanquake mentioned this pull request Feb 12, 2021

test: fix sign comparison warning in socket tests #21159

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jul 2, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

52007c9

kwvg mentioned this pull request Jul 2, 2021

merge #16117, #18358, #17383, #21052, #14424, #15159, #14689, #14978, partial #16908, #14978, #13932: Auxillary Backports dashpay/dash#4229

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jul 4, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

41c7983

kwvg added a commit to kwvg/dash that referenced this pull request Jul 9, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

307649f

kwvg added a commit to kwvg/dash that referenced this pull request Jul 9, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

0c2fcdc

kwvg added a commit to kwvg/dash that referenced this pull request Jul 9, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

5321ddb

kwvg added a commit to kwvg/dash that referenced this pull request Jul 13, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

8a40a3a

kwvg added a commit to kwvg/dash that referenced this pull request Jul 13, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

545d45d

kwvg added a commit to kwvg/dash that referenced this pull request Jul 13, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

c7d2d54

kwvg added a commit to kwvg/dash that referenced this pull request Jul 15, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

776b2ea

kwvg added a commit to kwvg/dash that referenced this pull request Jul 15, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

d3c15d0

kwvg added a commit to kwvg/dash that referenced this pull request Jul 16, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

d905728

kwvg added a commit to kwvg/dash that referenced this pull request Aug 1, 2021

merge bitcoin#20788: add RAII socket and use it instead of bare SOCKET

f195d54

hebasto added a commit to hebasto/bitcoin that referenced this pull request Sep 11, 2021

Cleanup headers after bitcoin#20788

2ec884d

hebasto added a commit to hebasto/bitcoin that referenced this pull request Sep 11, 2021

Cleanup headers after bitcoin#20788

3174425

hebasto mentioned this pull request Sep 11, 2021

Cleanup headers after #20788 #22952

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

net: add RAII socket and use it instead of bare SOCKET #20788

net: add RAII socket and use it instead of bare SOCKET #20788

Uh oh!

Conversation

vasild commented Dec 28, 2020

Uh oh!

vasild commented Dec 28, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

practicalswift commented Dec 28, 2020

Uh oh!

vasild commented Dec 30, 2020

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lontivero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Dec 31, 2020

Uh oh!

vasild commented Dec 31, 2020

Uh oh!

vasild commented Dec 31, 2020

Uh oh!

lontivero commented Dec 31, 2020

Uh oh!

vasild commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Feb 12, 2021

Uh oh!

vasild commented Feb 13, 2021

Uh oh!

Uh oh!

DrahtBot commented Dec 28, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading

vasild commented Jan 8, 2021 •

edited

Loading