Skip to content

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Sep 12, 2020

I'm probably missing something here but CNetAddr::scopeId seems unused. It isn't covered by tests. It was added in eda3d92 and potentially caused an uninitialized read fixed in b7b36de. Am running master on mainnet with the following asserts as a sanity check.

--- a/src/netaddress.cpp
+++ b/src/netaddress.cpp
@@ -132,6 +132,7 @@ CNetAddr::CNetAddr(const struct in_addr& ipv4Addr)
 CNetAddr::CNetAddr(const struct in6_addr& ipv6Addr, const uint32_t scope)
 {
     SetLegacyIPv6(Span<const uint8_t>(reinterpret_cast<const uint8_t*>(&ipv6Addr), sizeof(ipv6Addr)));
+    assert(scope == 0);
     scopeId = scope;
 }
 
@@ -716,6 +717,7 @@ bool CService::GetSockAddr(struct sockaddr* paddr, socklen_t *addrlen) const
+        assert(scopeId == 0);
         paddrin6->sin6_scope_id = scopeId;
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -120,6 +120,7 @@ bool static LookupIntern(const std::string& name, std::vector<CNetAddr>& vIP, un
             struct sockaddr_in6* s6 = (struct sockaddr_in6*) aiTrav->ai_addr;
+            assert(s6->sin6_scope_id == 0);
             resolved = CNetAddr(s6->sin6_addr, s6->sin6_scope_id);
         }

@fanquake fanquake added the P2P label Sep 12, 2020
@jonatack jonatack force-pushed the remove-unused-CNetAddr-scopeId branch from 606609a to 9662fad Compare September 12, 2020 10:20
@michaelfolkson
Copy link

Concept ACK. Seems convincing to me. Thanks for the clear explanation above on your process.

What are you seeking from reviewers? Replicating the above steps? Running the fuzz tests due to the minor change introduced to them? Or do you just need experienced reviewers to weigh in on whether removing this might create a problem?

@Saibato
Copy link
Contributor

Saibato commented Sep 12, 2020

tNACK with that merged we would no longer be able to connect or bind to specific scoped ipv6 addresses e.g.[::1%21].

There might be users who use scopes to tunnel traffic with same "ip" number to different adapters. with {ipv6%n]

in general https://tools.ietf.org/html/rfc4007 rfc's in the form of n-007 are relevant, imho 😸

@jonatack
Copy link
Member Author

@Saibato thanks for the info. I suspected there was something I was missing, but when no tests fail it still surprises me. Looks like we could use some.

@jonatack jonatack closed this Sep 12, 2020
@jonatack jonatack deleted the remove-unused-CNetAddr-scopeId branch September 13, 2020 10:16
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants