Reachable from either place where SetIP is used when all of:
In that case, SetIP turns an IPv4 address into an IPv6 address without setting the scopeId, which is subsequently read in GetSockAddr during CNetAddr::ToStringIP and passed to getnameinfo. Fix by ensuring every constructor initializes the scopeId field with something.
Nice catch. Can you add a test that tests for the old behavior?
69 | @@ -70,6 +70,7 @@ bool CNetAddr::SetSpecial(const std::string &strName) 70 | CNetAddr::CNetAddr(const struct in_addr& ipv4Addr) 71 | { 72 | SetRaw(NET_IPV4, (const uint8_t*)&ipv4Addr); 73 | + scopeId = 0;
I'd go a step further and move scopeId = 0 to static initialization instead:
uint32_t scopeId{0}; // for scoped/link-local ipv6 addresses
Good point! Switched to in-class initialization.
utACK 9151f78.
Excellent catch and thanks for another great contribution!
utACK 9151f783ca53cde718a3895324bd6389e5de6318 modulo @promag's nit
How did you find this issue?
I can't believe I didn't catch this issue as part of my routine static analysis runs :-\ I checked now and I had this specific case in multiple analysis reports, but it appears the signal drowned in the noise.
Would be nice to have the offending code path executed in a test to allow for catching it via dynamic analysis too.
utACK 04ae6807bc09d52c4738836243cb3dad8845b405
I was looking at the code that relates to a recent bug report. I didn't see the cause of the bug, but I did see this.
I added a test that successfully fails under -fsanitize=memory without this patch.
221 | + pnode->SetAddrLocal(addrLocal); 222 | + 223 | + // before patch, this causes undefined behavior detectable with clang's -fsanitize=memory 224 | + AdvertiseLocal(&*pnode); 225 | + 226 | + // suppress no-checks-run warning; if this test fails, it's by triggering triggering a sanitizer
Double "triggering"? :-)
utACK d0b673176b3214e16e4df50dcb65352fbd07969f modulo typo fix :-)
Reachable from either place where SetIP is used when our best-guess
addrLocal for a peer is IPv4, but the peer tells us it's reaching us at
an IPv6 address.
In that case, SetIP turns an IPv4 address into an IPv6 address without
setting the scopeId, which is subsequently read in GetSockAddr during
CNetAddr::ToStringIP and passed to getnameinfo. Fix by ensuring every
constructor initializes the scopeId field with something.
utACK b7b36de.
utACK 8ebbef016928811756e46b9086067d1c826797a8
utACK b7b36decaf878a8c1dcfdb4a27196c730043474b
Makes the code adhere to our developer cpp style guide and also fixes the warning that valgrind yells at me when running valgrind ./src/test/test_bitcoin -t net_tests/ipv4_peer_with_ipv6_addrMe_test.
I would suggest applying the same change to the ip array unsigned char ip[16] = {0}; and remove the memset call from the constructor. This leaves an empty constructor which can then be marked default.
re-utACK 8ebbef0
You're not acking the top commit (b7b36decaf878a8c1dcfdb4a27196c730043474b) is this intentional? (I guess not)
@laanwj Oh sorry I got the ordering wrong. I meant to re-utACK the latest commit :-)
