Allow the callers of CreateSock() to pass all 3 arguments to the socket(2) syscall. This makes it possible to create sockets of any domain/type/protocol. In addition to extending arguments, some extra safety checks were put in place.
The need for this came up during the discussion in #30043 (review)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See 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.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
-Wundef by fanquake)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.
3028 | @@ -3029,7 +3029,7 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError, 3029 | return false; 3030 | } 3031 | 3032 | - std::unique_ptr<Sock> sock = CreateSock(addrBind.GetSAFamily()); 3033 | + std::unique_ptr<Sock> sock = CreateSock(addrBind.GetSAFamily(), SOCK_STREAM, IPPROTO_TCP);
I'm assuming changing GetSAFamily()'s return type was avoided to keep the PR scoped (and since the change doesn't seem necessary).
Since GetSAFamily() returns sa_family_t, looks like this is a simple promotion from an unsigned short (sa_family_t) to int. This seems fine (it aligns with the typical way socket() args are typically used).
Returning sa_family_t is slightly more self-documenting, imo, would prefer to keep it as-is.
But yes it's simply a type-def'd integer type not a real enum, i suppose because it's "open", as in, new OS headers could potentially define their own address family without having to update a central list.
491 | // Not IPv4, IPv6 or UNIX 492 | - if (address_family == AF_UNSPEC) return nullptr; 493 | - 494 | - int protocol{IPPROTO_TCP}; 495 | -#if HAVE_SOCKADDR_UN 496 | - if (address_family == AF_UNIX) protocol = 0;
Seems ok to remove, since protocol is now an argument.
504 | return nullptr;
505 | }
506 |
507 | auto sock = std::make_unique<Sock>(hSocket);
508 |
509 | + if (domain != AF_INET && domain != AF_INET6 && domain != AF_UNIX) {
Seems good to add this sanity check to prevent setting non-pertinent sockopts.
nit: Might be worthwhile to mention in the PR description that in addition to extending arguments, some extra safety checks were put in place.
Done
544 | + 545 | return sock; 546 | } 547 | 548 | -std::function<std::unique_ptr<Sock>(const sa_family_t&)> CreateSock = CreateSockOS; 549 | +std::function<std::unique_ptr<Sock>(int, int, int)> CreateSock = CreateSockOS;
Since these are simple ints passed by value, seems ok to change from the previous const reference approach.
ACK for 77e34ded543e19ba27cab8d0cfc464af27f04bbf
Thanks. Seems like a reasonable change.
Sanity checks performed:
Left some minor notes (just thinking out loud, so no need to answer/comment unless you feel it's necessary).
261 | @@ -262,16 +262,18 @@ CService LookupNumeric(const std::string& name, uint16_t portDefault = 0, DNSLoo 262 | CSubNet LookupSubNet(const std::string& subnet_str); 263 | 264 | /** 265 | - * Create a TCP or UNIX socket in the given address family. 266 | - * @param[in] address_family to use for the socket. 267 | + * Create a real socket from the operating system. 268 | + * @param[in] domain Communications domain, first argument to the socket(2) syscall. 269 | + * @param[in] type Type of the socket, second argument to the socket(2) syscall. 270 | + * @param[in] protocol The particular protocol to be used with the socke, third argument to the socket(2) syscall.
typo nit:
* [@param](/bitcoin-bitcoin/contributor/param/)[in] protocol The particular protocol to be used with the socket, third argument to the socket(2) syscall.
Oh, it is usually either s, sock or socket. socke is something new, that is extra genuine ;-)
Fixed, invalidating 3 ACKs but should be trivial to re-review.
Thanks!
Concept and code-review ACK 77e34ded543e19ba27cab8d0cfc464af27f04bbf
77e34ded54...5f549c35d9: fix typo in the comment: s/socke/socket/
re-ACK 5f549c35d9a75c7019fe8a96963b988df5498eed 🧦
re-ACK 5f549c35d9a75c7019fe8a96963b988df5498eed
ACK 5f549c35d9a75c7019fe8a96963b988df5498eed
Silent merge conflict
../../../src/test/fuzz/i2p.cpp:30:16: error: no viable overloaded '='
30 | CreateSock = [&fuzzed_data_provider](const sa_family_t&) {
| ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
31 | return std::make_unique<FuzzedSock>(fuzzed_data_provider);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
32 | };
| ~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/std_function.h:469:7: note: candidate function not viable: no known conversion from '(lambda at ../../../src/test/fuzz/i2p.cpp:30:18)' to 'const function<unique_ptr<Sock> (int, int, int)>' for 1st argument
469 | operator=(const function& __x)
| ^ ~~~~~~~~~~~~~~~~~~~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/std_function.h:487:7: note: candidate function not viable: no known conversion from '(lambda at ../../../src/test/fuzz/i2p.cpp:30:18)' to 'function<unique_ptr<Sock> (int, int, int)>' for 1st argument
487 | operator=(function&& __x) noexcept
| ^ ~~~~~~~~~~~~~~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/std_function.h:501:7: note: candidate function not viable: no known conversion from '(lambda at ../../../src/test/fuzz/i2p.cpp:30:18)' to 'nullptr_t' (aka 'std::nullptr_t') for 1st argument
501 | operator=(nullptr_t) noexcept
| ^ ~~~~~~~~~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/std_function.h:531:2: note: candidate template ignored: requirement '_Callable<(lambda at ../../../src/test/fuzz/i2p.cpp:30:18), (lambda at ../../../src/test/fuzz/i2p.cpp:30:18), std::__invoke_result<(lambda at ../../../src/test/fuzz/i2p.cpp:30:18) &, int, int, int>>::value' was not satisfied [with _Functor = (lambda at ../../../src/test/fuzz/i2p.cpp:30:18)]
531 | operator=(_Functor&& __f)
| ^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/std_function.h:541:2: note: candidate template ignored: could not match 'reference_wrapper<_Functor>' against '(lambda at ../../../src/test/fuzz/i2p.cpp:30:18)'
541 | operator=(reference_wrapper<_Functor> __f) noexcept
| ^
1 error generated.
make[2]: *** [Makefile:17184: test/fuzz/fuzz-i2p.o] Error 1
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/26113679661</sub>
Allow the callers of `CreateSock()` to pass all 3 arguments to the
`socket(2)` syscall. This makes it possible to create sockets of
any domain/type/protocol.
5f549c35d9...1245d1388b: rebase due to (silent) merge conflict
re ACK 1245d1388b003c46092937def7041917aecec8de
re-ACK 1245d1388b003c46092937def7041917aecec8de
ACK 1245d1388b003c46092937def7041917aecec8de