Fixes #22613
The fix is pretty simple – it’s legal to memcpy between these types, you just can’t dereference members after a reinterpret_cast. The memcpys will be optimised away in any half decent compiler.
I chose to switch GetSockAddr/SetSockAddr to accept sockaddr_storage rather than sockaddr so we can be sure we’re never reading/writing out of bounds. Essentially, struct sockaddr* is old junk that we never want to deal with. Only hand those to the C APIs.
Can we kick the CI again?
I think you can do it yourself, git commit --amend and then git push --force.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
memcpy is notoriously dangerous though, this will have to be carefully checked to not introduce any subtle bugs, especially to make sure the right lengths are used everywhere.
In terms of lengths, and for more general context on this issue so that reviewers can avoid duplicating work:
struct sockaddr is an old 1980’s generic socket address type, (made with IPv4 in mind). It’s like a base class.struct sockaddr_in (“internet”) is the IPv4 “subclass.”
sockaddr and its layout is such that the two can be cast back & forth. In C. In C++ it’s UB to manipulate aliased unrelated types like that.struct sockaddr_in6 is the IPv6 “subclass.”
sockaddrsockaddr*. It’s up to you and the API to cast the pointer to the appropriate “subclass” once you know which protocol you’re dealing with.struct sockaddr_storage was added along with sockaddr_in6 as a “catch-all” opaque blob that is large enough to hold either one. The only field defined on the struct is the ss_family field (i.e. is this IPv4 or IPv6?). That field is guaranteed to map to the field in all the other versions so it’s “sort of” castable.So, in this PR here’s the strategy:
sockaddr_storage instead of sockaddr. Makes sure we have a big enough chunk of memory to work with, and it removes some internal casting.sockaddr_storage*, first we zero all the memory of the out-variable to get a clean slate, then create a protocol-specific local variable e.g. sockaddr_in and populate it, then memcpy the smaller subclass variable into the zero’d storage variable.sockaddr* -> e.g. sockaddr_in* with creating a local variable of the appropriate type and memcpy’ing using the “subclass” length. This looks like a read out of bounds but it’s no more out-of-bounds than casting the smaller struct pointer to the larger one was. It’s the same thing really.
mostly-ACK 77f0208cf912389c88415e8efc80af06ec7616e1 (jamesob/ackr/24827.1.Adlai-Holler.net_fix_undefined_behavi)
Built, tested locally.
I’m still spotting some reinterpret_cast<struct sockaddr*> calls in e.g.
ConnectSocketDirectlyTorControlConnection::ConnectGetBindAddressCConman::AcceptConnectionrandomenv.cpp:AddSockaddr (not sure if this one matters much)so I’m not sure this change resolves all of the UB related to sockaddr use.
Casting from sockaddr_storage* -> sockaddr* seems precarious, since they’re
different sizes, but is there some reason you avoided touching those here as
well?
All memcpy invocations look safe to me. Obviously the code will require care when modified though. I wonder if we could consolidate the memcpy calls to a (templated?) function to cut down the risk.
Longterm it may make sense to move to some kind of polymorphic union that can
encompass both sockaddr_in and sockaddr_in6 and doesn’t require all the
ip4/6 case handling, but that’s speculative and in any case the avoidance of UB
seems like a strict improvement here.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 77f0208cf912389c88415e8efc80af06ec7616e1 ([`jamesob/ackr/24827.1.Adlai-Holler.net_fix_undefined_behavi`](https://github.com/jamesob/bitcoin/tree/ackr/24827.1.Adlai-Holler.net_fix_undefined_behavi))
4
5Built, tested locally.
6
7I'm still spotting some `reinterpret_cast<struct sockaddr*>` calls in e.g.
8
9- - [ ] `ConnectSocketDirectly`
10- - [ ] `TorControlConnection::Connect`
11- - [ ] `GetBindAddress`
12- - [ ] `CConman::AcceptConnection`
13- - [ ] [some others in net.cpp...]
14- - [ ] `randomenv.cpp:AddSockaddr` (not sure if this one matters much)
15
16so I'm not sure this change resolves all of the UB related to sockaddr use.
17Casting from `sockaddr_storage*` -> `sockaddr*` seems precarious, since they're
18different sizes, but is there some reason you avoided touching those here as
19well?
20
21All memcpy invocations look safe to me. Obviously the code will require care
22when modified though. I wonder if we could consolidate the memcpy calls to a
23(templated?) function to cut down the risk.
24
25Longterm it may make sense to move to some kind of polymorphic union that can
26encompass both `sockaddr_in` and `sockaddr_in6` and doesn't require all the
27ip4/6 case handling, but that's speculative and in any case the avoidance of UB
28seems like a strict improvement here.
29-----BEGIN PGP SIGNATURE-----
30
31iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmJYlAUACgkQepNdrbLE
32TwVGxg/+NwnQ9FbaWInxv54qFF+X4QWYiOA1JAzPpGBv/Rf0KMbSz/enSsAax0j6
33YxwXyXXOMkL0CR//PzCMZqEBpqydOZcSnAE3elINlhySWMhpz0iXxz0SNuCyTnSp
34fMl/5RKTvkPvWqYCgHCqM0HmUPwdkSEFi+BHT+B8+kZBZcNgegbXEPiRcQqG6JEJ
35qs9Ro0dP9J3oH9khpxfucTkRerHSY3QmL7ZZIbzSazQnhmo/yUP1Q5zZHBsT3hZP
36RcDzp0T6RX94/vhT2gSaGRKCYasUbiJ07puEj6q/lfVWwzCBewvR/PseMS3Q5GWF
37Cj6WpTRiCetq6yeKDkGId4tgGqLEHk1RHpbtzOLbzoJMMR0sclbQjPEkHQy2/qXJ
38Asrvh39zWhCZU83KMTxY5tzP66XGtxgXcCmtFjxBjwBa4YcgGezpKYylUVwje7uT
39Hw2Z+MAZJTgjCyYIhdITs89fcJSbdoSuEekuPXtIgKAAvGmmFGFfGGoM60GonWVY
4000PHDw8eKluGMe7cG1PvQI58jWAForZEu9rqfyspBZcbG/l+VVOoKK/EfYnmDcQ1
41oFJppPouXvX/btgk5uRuNVCrzETlaxCcyRAekdKXh4IC6JmyV9pbS1f+brvOejLx
42k+5uV4d8FzkXFQkoOMcrfukQzLq063XROF9oyN/jKww/ojSnSjE=
43=kbVB
44-----END PGP SIGNATURE-----
0Tested on Linux-5.10.0-11-amd64-x86_64-with-glibc2.31
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default
3
4Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4.1 -msse4.2 -msse4 -msha i
5
6Compiler version: Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63
Casting from sockaddr_storage* -> sockaddr* seems precarious, since they’re different sizes, but is there some reason you avoided touching those here as well?
I guess since we’re essentially “upcasting” in this case (i.e. the mem allocation referenced by sockaddr_storage is larger than sockaddr, so the cast pointer will never run into unclaimed memory) it’s okay to do this cast. Is that right?
The remaining casts to sockaddr* are unavoidable and safe because we are calling into the external C APIs which expect us to do that cast.
Good call on some wrapper functions. I wrote them and I’m a lot happier with the result. Templating didn’t work out because (1) in the load functions I ended up needing to check the protocol type for safety and (2) in general I didn’t want to be memcpy’ing into templated types.
We’ll see how the CI does but I’m really happy with the new state of the PR, think we’re in good shape.
memcpy’s out for function wrappers so it’s more of a drop-in replacement.
Looking good, nice changes. I think the new functions you’ve introduced should help to reduce @laanwj’s concerns about memset. We could play some code-golf with the patch below to cut down on the number of separate memsets, but I think what you have right now in HEAD is a big improvement on the status quo with or without the patch.
0diff --git a/src/netaddress.cpp b/src/netaddress.cpp
1index 2569e9e71f..11929f5394 100644
2--- a/src/netaddress.cpp
3+++ b/src/netaddress.cpp
4@@ -1241,52 +1241,34 @@ bool operator<(const CSubNet& a, const CSubNet& b)
5 return (a.network < b.network || (a.network == b.network && memcmp(a.netmask, b.netmask, 16) < 0));
6 }
7
8-sockaddr_in LoadSockaddrIPv4(const sockaddr& src) {
9- assert(src.sa_family == AF_INET);
10+template <typename T>
11+static sockaddr_in LoadSockaddrIPv4Internal(const T& src) {
12 sockaddr_in s4;
13 std::memcpy(&s4, &src, sizeof(s4));
14 return s4;
15 }
16+
17+sockaddr_in LoadSockaddrIPv4(const sockaddr& src) {
18+ assert(src.sa_family == AF_INET);
19+ return LoadSockaddrIPv4Internal(src);
20+}
21 sockaddr_in LoadSockaddrIPv4(const sockaddr_storage& src) {
22 assert(src.ss_family == AF_INET);
23- sockaddr_in s4;
24- std::memcpy(&s4, &src, sizeof(s4));
25- return s4;
26+ return LoadSockaddrIPv4Internal(src);
27 }
28
29-sockaddr_in6 LoadSockaddrIPv6(const sockaddr& src) {
30- assert(src.sa_family == AF_INET6);
31- sockaddr_in6 s6;
32- std::memcpy(&s6, &src, sizeof(s6));
33- return s6;
34-}
35-sockaddr_in6 LoadSockaddrIPv6(const sockaddr_storage& src) {
36- assert(src.ss_family == AF_INET6);
37+template <typename T>
38+sockaddr_in6 LoadSockaddrIPv6Internal(const T& src) {
39 sockaddr_in6 s6;
40 std::memcpy(&s6, &src, sizeof(s6));
41 return s6;
42 }
43
44-void StoreSockaddrIPv4(const sockaddr_in& src, sockaddr* ptr, socklen_t *addrlen) {
45- constexpr size_t sz4 = sizeof(sockaddr_in);
46- assert(*addrlen >= sz4);
47- *addrlen = sz4;
48- std::memcpy(ptr, &src, sz4);
49-}
50-void StoreSockaddrIPv4(const sockaddr_in& src, sockaddr_storage* ptr, socklen_t *addrlen) {
51- constexpr size_t sz4 = sizeof(sockaddr_in);
52- *addrlen = sz4;
53- std::memcpy(ptr, &src, sz4);
54-}
55-
56-void StoreSockaddrIPv6(const sockaddr_in6& src, sockaddr* ptr, socklen_t *addrlen) {
57- constexpr size_t sz6 = sizeof(sockaddr_in6);
58- assert(*addrlen >= sz6);
59- *addrlen = sz6;
60- std::memcpy(ptr, &src, sz6);
61+sockaddr_in6 LoadSockaddrIPv6(const sockaddr& src) {
62+ assert(src.sa_family == AF_INET6);
63+ return LoadSockaddrIPv6Internal(src);
64 }
65-void StoreSockaddrIPv6(const sockaddr_in6& src, sockaddr_storage* ptr, socklen_t *addrlen) {
66- constexpr size_t sz6 = sizeof(sockaddr_in6);
67- *addrlen = sz6;
68- std::memcpy(ptr, &src, sz6);
69+sockaddr_in6 LoadSockaddrIPv6(const sockaddr_storage& src) {
70+ assert(src.ss_family == AF_INET6);
71+ return LoadSockaddrIPv6Internal(src);
72 }
73diff --git a/src/netaddress.h b/src/netaddress.h
74index 171653bdbc..254c92e86f 100644
75--- a/src/netaddress.h
76+++ b/src/netaddress.h
77@@ -596,9 +596,25 @@ sockaddr_in6 LoadSockaddrIPv6(const sockaddr_storage& src);
78 * Overwrite the provided generic sockaddr struct safely. Use these
79 * instead of casting the src and dereferencing it.
80 */
81-void StoreSockaddrIPv4(const sockaddr_in& src, sockaddr* ptr, socklen_t *addrlen);
82-void StoreSockaddrIPv4(const sockaddr_in& src, sockaddr_storage* ptr, socklen_t *addrlen);
83-void StoreSockaddrIPv6(const sockaddr_in6& src, sockaddr* ptr, socklen_t *addrlen);
84-void StoreSockaddrIPv6(const sockaddr_in6& src, sockaddr_storage* ptr, socklen_t *addrlen);
85+template <typename T>
86+void StoreSockaddrIPv4(const sockaddr_in& src, T* ptr, socklen_t *addrlen) {
87+ constexpr size_t sz4 = sizeof(sockaddr_in);
88+
89+ if (typeid(T) == typeid(sockaddr)) {
90+ assert(*addrlen >= sz4);
91+ }
92+ *addrlen = sz4;
93+ std::memcpy(ptr, &src, sz4);
94+}
95+
96+template <typename T>
97+void StoreSockaddrIPv6(const sockaddr_in6& src, T* ptr, socklen_t *addrlen) {
98+ constexpr size_t sz6 = sizeof(sockaddr_in6);
99+ if (typeid(T) == typeid(sockaddr)) {
100+ assert(*addrlen >= sz6);
101+ }
102+ *addrlen = sz6;
103+ std::memcpy(ptr, &src, sz6);
104+}
105
106 #endif // BITCOIN_NETADDRESS_H
Fixes #22613
The fix is pretty simple – it's legal to memcpy between these types, you just can't reference members after a reinterpret_cast. The memcpys will be optimised away in any half decent compiler.
I chose to switch GetSockAddr/SetSockAddr to work with sockaddr_storage so we can be sure we're never reading/writing out of bounds. Essentially, `struct sockaddr*` is old junk that we never want to deal with. Only hand those to the C APIs.
74 }
75 if (ai_trav->ai_family == AF_INET6) {
76 assert(ai_trav->ai_addrlen >= sizeof(sockaddr_in6));
77- const sockaddr_in6* s6{reinterpret_cast<sockaddr_in6*>(ai_trav->ai_addr)};
78- resolved_addresses.emplace_back(s6->sin6_addr, s6->sin6_scope_id);
79+ struct sockaddr_in6 s6 = LoadSockaddrIPv6(*ai_trav->ai_addr);
ACK 0b3ad721462ed060fae991d8ad1510cd92a69bfb (jamesob/ackr/24827.3.Adlai-Holler.net_fix_undefined_behavi)
Reviewed, ran functional tests locally. Seems like a clear win to avoid UB, and probably improves readability too.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 0b3ad721462ed060fae991d8ad1510cd92a69bfb ([`jamesob/ackr/24827.3.Adlai-Holler.net_fix_undefined_behavi`](https://github.com/jamesob/bitcoin/tree/ackr/24827.3.Adlai-Holler.net_fix_undefined_behavi))
4
5Reviewed, ran functional tests locally. Seems like a clear win to avoid UB, and probably readability too.
6-----BEGIN PGP SIGNATURE-----
7
8iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmKM4SgACgkQepNdrbLE
9TwUcqw/7BsjxUtnuJOQm3Lrev+KaKWgPVkPEgCammTzG9j+DwHiRnByEGxIKnWHF
10GwhqiK3asXhokUH6wtmEyCdoAFzad3IYkbMOGIL6vX+2RR3mAPlP6arCcZ4goHz6
11cP0m/IIHhhoMVL2wT72puQgWXH5bQJPxRxq/W6GWIs+xHt5BJCPmCg9EAPfchWNu
12iJMy1nJGHrktKAMQXNAxhMUy33rhyfang1s9LgGmOJt3NV1QGW5rCope0YgyWyQX
13ZZTAJpp1F56SYfldDbSwxoZ7HyIsn0GBxQe+kUIyA8P37QKWx8seQo4mF7MT7c+g
14itZePca2ztFVg7PXRqyAeV2qCBIDXJFlCTWFWKP+0Qv9daRMHuuLqywM6/njinJN
15tQ1Dr6zaeCytvO+B3TSF98uq/GnGdzBSpEWzywGYEiL8xI4O9s/63QASUr/Fs3tE
16xwwenHpASN0mCBkndmnBs5Ec3+rE5RYRdxh2XPhH0N5A0y7E2L76os+tlbstXxeX
17/GFaP5ebxfPzpEU+CEzScmSWL/vVI5H3oS6LTHr8pZucYmrD2WO9YBOx74oE1asP
18OT0IOsLmhpaEDcq62IarscW36S+nMYBx77T1wF18l0jgX9kr467P7Z7o1DFMYrJI
19Xvyi3GzFx5Dh4IKDca34K3lXlFIo60VQqWEwpNW3byvxWyRzyOk=
20=NNys
21-----END PGP SIGNATURE-----
0Tested on Linux-5.10.0-11-amd64-x86_64-with-glibc2.31
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/local/bin/clang++ CC=/usr/local/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4.1 -msse4.2 -msse4 -msha i
5
6Compiler version: Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
What about using placement new to change the dynamic type of the object?
Correct me if I am wrong, if we have:
0struct S1 {
1 int x;
2};
3
4struct S2 {
5 int x;
6};
7
8S1 s1;
9S2* s2_ptr = reinterpret_cast<S2*>(&s1);
10s2_ptr->x = 5; // UB because the S1 object is accessed as S2
but the following is ok because it changes the dynamic type of the object to S2:
0S1 s1;
1S2* s2_ptr = new (&s1) S2;
2s2_ptr->x = 5; // ok
Here is an alternative patch that uses that approach. It is much smaller because it just replaces e.g. (sockaddr_in*)foo with SockAddrCast<sockaddr_in>(foo):
0diff --git a/src/net.cpp b/src/net.cpp
1index c37d90519c..b3265dc5dc 100644
2--- a/src/net.cpp
3+++ b/src/net.cpp
4@@ -427,13 +427,13 @@ static CAddress GetBindAddress(const Sock& sock)
5 {
6 CAddress addr_bind;
7 struct sockaddr_storage sockaddr_bind;
8 socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
9 if (sock.Get() != INVALID_SOCKET) {
10 if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
11- addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind);
12+ addr_bind.SetSockAddr(sockaddr_bind);
13 } else {
14 LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
15 }
16 }
17 return addr_bind;
18 }
19@@ -920,13 +920,13 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
20 if (nErr != WSAEWOULDBLOCK) {
21 LogPrintf("socket error accept failed: %s\n", NetworkErrorString(nErr));
22 }
23 return;
24 }
25
26- if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) {
27+ if (!addr.SetSockAddr(sockaddr)) {
28 LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Unknown socket family\n");
29 } else {
30 addr = CAddress{MaybeFlipIPv6toCJDNS(addr), NODE_NONE};
31 }
32
33 const CAddress addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock)), NODE_NONE};
34@@ -2056,13 +2056,13 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError,
35 {
36 int nOne = 1;
37
38 // Create socket for listening for incoming connections
39 struct sockaddr_storage sockaddr;
40 socklen_t len = sizeof(sockaddr);
41- if (!addrBind.GetSockAddr((struct sockaddr*)&sockaddr, &len))
42+ if (!addrBind.GetSockAddr(&sockaddr, &len))
43 {
44 strError = strprintf(Untranslated("Bind address family for %s not supported"), addrBind.ToString());
45 LogPrintLevel(BCLog::NET, BCLog::Level::Error, "%s\n", strError.original);
46 return false;
47 }
48
49@@ -2151,21 +2151,19 @@ void Discover()
50 if (ifa->ifa_addr == nullptr) continue;
51 if ((ifa->ifa_flags & IFF_UP) == 0) continue;
52 if (strcmp(ifa->ifa_name, "lo") == 0) continue;
53 if (strcmp(ifa->ifa_name, "lo0") == 0) continue;
54 if (ifa->ifa_addr->sa_family == AF_INET)
55 {
56- struct sockaddr_in* s4 = (struct sockaddr_in*)(ifa->ifa_addr);
57- CNetAddr addr(s4->sin_addr);
58+ CNetAddr addr(SockAddrCast<sockaddr_in>(ifa->ifa_addr)->sin_addr);
59 if (AddLocal(addr, LOCAL_IF))
60 LogPrintf("%s: IPv4 %s: %s\n", __func__, ifa->ifa_name, addr.ToString());
61 }
62 else if (ifa->ifa_addr->sa_family == AF_INET6)
63 {
64- struct sockaddr_in6* s6 = (struct sockaddr_in6*)(ifa->ifa_addr);
65- CNetAddr addr(s6->sin6_addr);
66+ CNetAddr addr(SockAddrCast<sockaddr_in6>(ifa->ifa_addr)->sin6_addr);
67 if (AddLocal(addr, LOCAL_IF))
68 LogPrintf("%s: IPv6 %s: %s\n", __func__, ifa->ifa_name, addr.ToString());
69 }
70 }
71 freeifaddrs(myaddrs);
72 }
73diff --git a/src/netaddress.cpp b/src/netaddress.cpp
74index ca148bfa51..b997dc6ee9 100644
75--- a/src/netaddress.cpp
76+++ b/src/netaddress.cpp
77@@ -831,20 +831,20 @@ CService::CService(const struct sockaddr_in& addr) : CNetAddr(addr.sin_addr), po
78
79 CService::CService(const struct sockaddr_in6 &addr) : CNetAddr(addr.sin6_addr, addr.sin6_scope_id), port(ntohs(addr.sin6_port))
80 {
81 assert(addr.sin6_family == AF_INET6);
82 }
83
84-bool CService::SetSockAddr(const struct sockaddr *paddr)
85+bool CService::SetSockAddr(const sockaddr_storage& addr)
86 {
87- switch (paddr->sa_family) {
88+ switch (addr.ss_family) {
89 case AF_INET:
90- *this = CService(*(const struct sockaddr_in*)paddr);
91+ *this = CService(*SockAddrCast<sockaddr_in>(&addr));
92 return true;
93 case AF_INET6:
94- *this = CService(*(const struct sockaddr_in6*)paddr);
95+ *this = CService(*SockAddrCast<sockaddr_in6>(&addr));
96 return true;
97 default:
98 return false;
99 }
100 }
101
102@@ -872,32 +872,32 @@ bool operator<(const CService& a, const CService& b)
103 * parameter might change after calling this function if
104 * the size of the corresponding address structure
105 * changed.
106 *
107 * [@returns](/bitcoin-bitcoin/contributor/returns/) Whether or not the operation was successful.
108 */
109-bool CService::GetSockAddr(struct sockaddr* paddr, socklen_t *addrlen) const
110+bool CService::GetSockAddr(struct sockaddr_storage* paddr, socklen_t* addrlen) const
111 {
112+ memset(paddr, 0x0, *addrlen);
113+
114 if (IsIPv4()) {
115 if (*addrlen < (socklen_t)sizeof(struct sockaddr_in))
116 return false;
117 *addrlen = sizeof(struct sockaddr_in);
118- struct sockaddr_in *paddrin = (struct sockaddr_in*)paddr;
119- memset(paddrin, 0, *addrlen);
120+ sockaddr_in* paddrin = SockAddrCast<sockaddr_in>(paddr);
121 if (!GetInAddr(&paddrin->sin_addr))
122 return false;
123 paddrin->sin_family = AF_INET;
124 paddrin->sin_port = htons(port);
125 return true;
126 }
127 if (IsIPv6() || IsCJDNS()) {
128 if (*addrlen < (socklen_t)sizeof(struct sockaddr_in6))
129 return false;
130 *addrlen = sizeof(struct sockaddr_in6);
131- struct sockaddr_in6 *paddrin6 = (struct sockaddr_in6*)paddr;
132- memset(paddrin6, 0, *addrlen);
133+ sockaddr_in6* paddrin6 = SockAddrCast<sockaddr_in6>(paddr);
134 if (!GetIn6Addr(&paddrin6->sin6_addr))
135 return false;
136 paddrin6->sin6_scope_id = m_scope_id;
137 paddrin6->sin6_family = AF_INET6;
138 paddrin6->sin6_port = htons(port);
139 return true;
140diff --git a/src/netaddress.h b/src/netaddress.h
141index 47ba045334..e286b93a41 100644
142--- a/src/netaddress.h
143+++ b/src/netaddress.h
144@@ -525,14 +525,14 @@ protected:
145 public:
146 CService();
147 CService(const CNetAddr& ip, uint16_t port);
148 CService(const struct in_addr& ipv4Addr, uint16_t port);
149 explicit CService(const struct sockaddr_in& addr);
150 uint16_t GetPort() const;
151- bool GetSockAddr(struct sockaddr* paddr, socklen_t* addrlen) const;
152- bool SetSockAddr(const struct sockaddr* paddr);
153+ bool GetSockAddr(sockaddr_storage* paddr, socklen_t* addrlen) const;
154+ bool SetSockAddr(const sockaddr_storage& addr);
155 friend bool operator==(const CService& a, const CService& b);
156 friend bool operator!=(const CService& a, const CService& b) { return !(a == b); }
157 friend bool operator<(const CService& a, const CService& b);
158 std::vector<unsigned char> GetKey() const;
159 std::string ToString() const;
160 std::string ToStringPort() const;
161@@ -573,7 +573,36 @@ public:
162
163 private:
164 const uint64_t m_salt_k0;
165 const uint64_t m_salt_k1;
166 };
167
168+/**
169+ * Cast sockaddr or sockaddr_storage to sockaddr_in or sockaddr_in6.
170+ * This is as safe as the traditional typecast:
171+ * [@code](/bitcoin-bitcoin/contributor/code/){.cpp}
172+ * sockaddr_storage from;
173+ * sockaddr_in* to_ptr = (sockaddr_in*)&from;
174+ * to_ptr->sin_family = ...
175+ * [@endcode](/bitcoin-bitcoin/contributor/endcode/)
176+ * except that it avoids the undefined behavior of the last line above by not
177+ * breaking the strict aliasing rules.
178+ * Placement new changes the dynamic type of the object to `To`, so that it is
179+ * safe to access it as `To` afterwards.
180+ */
181+template <typename To, typename From>
182+static inline To* SockAddrCast(const From* ptr)
183+{
184+ // Make sure nobody abuses this function for other types.
185+ static_assert(
186+ std::is_same<From, sockaddr>::value ||
187+ std::is_same<From, sockaddr_storage>::value);
188+ static_assert(
189+ std::is_same<To, sockaddr_in>::value ||
190+ std::is_same<To, sockaddr_in6>::value);
191+ // Make sure that the placement new will not modify the memory pointed by `ptr`.
192+ static_assert(std::is_trivially_default_constructible<To>::value);
193+
194+ return new (const_cast<From*>(ptr)) To;
195+}
196+
197 #endif // BITCOIN_NETADDRESS_H
198diff --git a/src/netbase.cpp b/src/netbase.cpp
199index 030f462ed9..ca59230034 100644
200--- a/src/netbase.cpp
201+++ b/src/netbase.cpp
202@@ -65,17 +65,17 @@ std::vector<CNetAddr> WrappedGetAddrInfo(const std::string& name, bool allow_loo
203 // Traverse the linked list starting with ai_trav.
204 addrinfo* ai_trav{ai_res};
205 std::vector<CNetAddr> resolved_addresses;
206 while (ai_trav != nullptr) {
207 if (ai_trav->ai_family == AF_INET) {
208 assert(ai_trav->ai_addrlen >= sizeof(sockaddr_in));
209- resolved_addresses.emplace_back(reinterpret_cast<sockaddr_in*>(ai_trav->ai_addr)->sin_addr);
210+ resolved_addresses.emplace_back(SockAddrCast<sockaddr_in>(ai_trav->ai_addr)->sin_addr);
211 }
212 if (ai_trav->ai_family == AF_INET6) {
213 assert(ai_trav->ai_addrlen >= sizeof(sockaddr_in6));
214- const sockaddr_in6* s6{reinterpret_cast<sockaddr_in6*>(ai_trav->ai_addr)};
215+ const sockaddr_in6* s6 = SockAddrCast<sockaddr_in6>(ai_trav->ai_addr);
216 resolved_addresses.emplace_back(s6->sin6_addr, s6->sin6_scope_id);
217 }
218 ai_trav = ai_trav->ai_next;
219 }
220 freeaddrinfo(ai_res);
221
222@@ -485,13 +485,13 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a
223
224 std::unique_ptr<Sock> CreateSockTCP(const CService& address_family)
225 {
226 // Create a sockaddr from the specified service.
227 struct sockaddr_storage sockaddr;
228 socklen_t len = sizeof(sockaddr);
229- if (!address_family.GetSockAddr((struct sockaddr*)&sockaddr, &len)) {
230+ if (!address_family.GetSockAddr(&sockaddr, &len)) {
231 LogPrintf("Cannot create socket for %s: unsupported network\n", address_family.ToString());
232 return nullptr;
233 }
234
235 // Create a TCP socket in the address family of the specified service.
236 SOCKET hSocket = socket(((struct sockaddr*)&sockaddr)->sa_family, SOCK_STREAM, IPPROTO_TCP);
237@@ -550,13 +550,13 @@ bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nT
238 struct sockaddr_storage sockaddr;
239 socklen_t len = sizeof(sockaddr);
240 if (sock.Get() == INVALID_SOCKET) {
241 LogPrintf("Cannot connect to %s: invalid socket\n", addrConnect.ToString());
242 return false;
243 }
244- if (!addrConnect.GetSockAddr((struct sockaddr*)&sockaddr, &len)) {
245+ if (!addrConnect.GetSockAddr(&sockaddr, &len)) {
246 LogPrintf("Cannot connect to %s: unsupported network\n", addrConnect.ToString());
247 return false;
248 }
249
250 // Connect to the addrConnect service on the hSocket socket.
251 if (sock.Connect(reinterpret_cast<struct sockaddr*>(&sockaddr), len) == SOCKET_ERROR) {
252diff --git a/src/test/util/net.h b/src/test/util/net.h
253index 34ab9958c6..eb4c283cf8 100644
254--- a/src/test/util/net.h
255+++ b/src/test/util/net.h
256@@ -132,13 +132,13 @@ public:
257 if (addr != nullptr) {
258 // Pretend all connections come from 5.5.5.5:6789
259 memset(addr, 0x00, *addr_len);
260 const socklen_t write_len = static_cast<socklen_t>(sizeof(sockaddr_in));
261 if (*addr_len >= write_len) {
262 *addr_len = write_len;
263- sockaddr_in* addr_in = reinterpret_cast<sockaddr_in*>(addr);
264+ sockaddr_in* addr_in = SockAddrCast<sockaddr_in>(addr);
265 addr_in->sin_family = AF_INET;
266 memset(&addr_in->sin_addr, 0x05, sizeof(addr_in->sin_addr));
267 addr_in->sin_port = htons(6789);
268 }
269 }
270 return std::make_unique<StaticContentsSock>("");
271diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
272index d6e792a55f..c9b6fc289f 100644
273--- a/src/torcontrol.cpp
274+++ b/src/torcontrol.cpp
275@@ -137,13 +137,13 @@ bool TorControlConnection::Connect(const std::string& tor_control_center, const
276 LogPrintf("tor: Failed to look up control center %s\n", tor_control_center);
277 return false;
278 }
279
280 struct sockaddr_storage control_address;
281 socklen_t control_address_len = sizeof(control_address);
282- if (!control_service.GetSockAddr(reinterpret_cast<struct sockaddr*>(&control_address), &control_address_len)) {
283+ if (!control_service.GetSockAddr(&control_address, &control_address_len)) {
284 LogPrintf("tor: Error parsing socket address %s\n", tor_control_center);
285 return false;
286 }
287
288 // Create a new socket, set up callbacks and enable notification bits
289 b_conn = bufferevent_socket_new(base, -1, BEV_OPT_CLOSE_ON_FREE);
The remaining casts to
sockaddr*are unavoidable and safe because we are calling into the external C APIs which expect us to do that cast.
I understand the “unavoidable” but I don’t understand the “safe”. Do you mean that it is safe just because it goes to external C API? I made the following experiment which shows the opposite (both with gcc 12 and clang 15):
s.h0struct S1 {
1 int x;
2};
3
4struct S2 {
5 int x;
6};
f.c0#include "s.h"
1
2int f(struct S1* s1, struct S2* s2)
3{
4 s1->x = 5;
5 s2->x = 6;
6 return s1->x;
7}
main.cc0#include "s.h"
1
2extern "C" int f(S1*, S2*);
3
4int main(int, char**)
5{
6 S1 s1;
7 std::cout << f(&s1, (S2*)&s1) << std::endl;
8 return 0;
9}
Compile with -O3 and 5 is printed. Change the type of the second argument to S1 and 6 is printed.
I think the bind(2) API, strictly speaking implies UB, but it is safe because the bind() function never does something like the above, nor we do before passing it the argument.
There is an example in bind(2) which interprets sockaddr_un as sockaddr - that is also UB according to the standard of strict aliasing rules1 2, right? What about memcpy() where we cast anything to void*, also UB?
