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)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
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).
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;
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.
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;
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:
0 * [@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!
77e34ded54...5f549c35d9: fix typo in the comment: s/socke/socket/
Silent merge conflict
0../../../src/test/fuzz/i2p.cpp:30:16: error: no viable overloaded '='
1 30 | CreateSock = [&fuzzed_data_provider](const sa_family_t&) {
2 | ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 31 | return std::make_unique<FuzzedSock>(fuzzed_data_provider);
4 | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
5 32 | };
6 | ~
7/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
8 469 | operator=(const function& __x)
9 | ^ ~~~~~~~~~~~~~~~~~~~
10/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
11 487 | operator=(function&& __x) noexcept
12 | ^ ~~~~~~~~~~~~~~
13/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
14 501 | operator=(nullptr_t) noexcept
15 | ^ ~~~~~~~~~
16/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)]
17 531 | operator=(_Functor&& __f)
18 | ^
19/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)'
20 541 | operator=(reference_wrapper<_Functor> __f) noexcept
21 | ^
221 error generated.
23make[2]: *** [Makefile:17184: test/fuzz/fuzz-i2p.o] Error 1
🚧 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.
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