Network id: attempt to identify IPv6 network
Categories
(Core :: Networking, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: bagder, Assigned: valentin)
Details
(Whiteboard: [necko-triaged])
Attachments
(5 files, 1 obsolete file)
The experiment to attempt to identify[*] the current IPv4 network (NETWORK_ID) on which Firefox is running has proven to have a fairly high degree of "unable to find an id" cases. To lower the amount of ID failures, we should also extract an IPv6 network id (but make it a generic single output ID). The IPv6 implementations need to be platform specific as these low level APIs are completely different between the different targets. [*] = they're identified to the level that they get a hash value out. A value that *should* be the same the next time Firefox runs on that same network.
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Reporter | ||
Comment 1•7 years ago
|
||
First out, the Linux version. (I've worked out and tested the logic with this command-line tool: https://github.com/bagder/gw-mac)
Reporter | ||
Comment 2•7 years ago
|
||
Forgot to mention the successful try-run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=769ded415f27714159eff77c9fa0a533039a835e&selectedJob=127730972
Assignee | ||
Comment 3•7 years ago
|
||
Comment on attachment 8904168 [details] [diff] [review] 0001-bug-1395914-figure-out-network-id-for-IPv6-too-Linux.patch Review of attachment 8904168 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: netwerk/system/linux/nsNotifyAddrListener_Linux.cpp @@ +189,5 @@ > + char ip6[40]; > + int devnum; > + int preflen; > + int scope; > + int flags; let's use fixed width types here - int32_t ? @@ +219,5 @@ > + ip6, &devnum, &preflen, &scope, &flags, name)) { > + if (scope == GLOBAL) { > + unsigned char id6[33]; > + int i; > + int bits; same. int32_t ? @@ +226,5 @@ > + for (bits=preflen, i=0; bits>0; bits-=8, i++) { > + char buf[3]; > + long val; > + int mask; > + int maskit[]={0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, 0xff}; same. int32_t ? @@ +231,5 @@ > + buf[0]=ip6[i*2]; > + buf[1]=ip6[i*2+1]; > + buf[2]=0; > + /* convert from hex */ > + val = strtol(buf, NULL, 16); nit: nullptr instead of NULL @@ +233,5 @@ > + buf[2]=0; > + /* convert from hex */ > + val = strtol(buf, NULL, 16); > + mask = ( bits >= 8 ) ? 0xff : maskit[bits]; > + id6[i]=(unsigned char)val&mask; nit: spacing @@ +263,5 @@ > + nsAutoCString output; > + sha1.update(addition.get(), addition.Length()); > + uint8_t digest[SHA1Sum::kHashSize]; > + sha1.finish(digest); > + nsCString newString(reinterpret_cast<char*>(digest), can we use nsAutoCString here as well?
Reporter | ||
Comment 4•7 years ago
|
||
Thanks Valentin! Some of the 'int' variables I use there are used as input arguments to sscanf() and that function is documented to take "pointers to int", so I think they should remain like that, but the rest of the remarks have been addressed in this version 2.
Reporter | ||
Comment 5•7 years ago
|
||
Here's the mac version of the same logic for ipv6 netid detection. The try build on mac is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89a1673985e230ea69a4d29540710a3f7c496da3
Assignee | ||
Comment 6•7 years ago
|
||
Comment on attachment 8906561 [details] [diff] [review] 0001-bug-1395914-figure-out-network-id-for-IPv6-too-mac-r.patch Review of attachment 8906561 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/system/mac/nsNetworkLinkService.mm @@ +299,5 @@ > + return IPV6_SCOPE_GLOBAL; > +} > + > +static int > +ipv6_prefix(void *val, int size) val doesn't really need to be void* - let's use the actual type to prevent calling it on something wrong. Also, do we need the size argument, or can we just hardcode the size of the struct? nit: aVal naming for argument @@ +319,5 @@ > + } > + } > + for (; bit != 0; bit--) { > + if (name[byte] & (1 << bit)) { > + return 0; I assume this not normally happen, right? Consider adding some comments. @@ +351,5 @@ > + !(ifa->ifa_flags & (IFF_POINTOPOINT|IFF_LOOPBACK))) { > + // only IPv6 interfaces that aren't pointtopoint or loopback > + struct sockaddr_in6 *sin = > + (struct sockaddr_in6 *)ifa->ifa_netmask; > + if (sin) { nit: having variables named sin and sin6 is a bit confusing. consider more descriptive names @@ +356,5 @@ > + char addr_buf[128]; > + int scope; > + struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)ifa->ifa_addr; > + scope = ipv6_scope(sin6); > + if (scope == IPV6_SCOPE_GLOBAL) { nit: can we reverse some of these if blocks so we get fewer levels of indentantion? if (!sin) continue; ? @@ +358,5 @@ > + struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)ifa->ifa_addr; > + scope = ipv6_scope(sin6); > + if (scope == IPV6_SCOPE_GLOBAL) { > + // only care about global scope > + inet_ntop(AF_INET6, &sin6->sin6_addr, addr_buf, nit: should we check the return value? also, you don't seem to use addr_buf for anything @@ +365,5 @@ > + sizeof(struct in6_addr)); > + if (prefix && (prefix < 128)) { > + unsigned char *p = (unsigned char *)&sin6->sin6_addr; > + int match = 0; > + /* check if prefix was already found */ nit: let's use c++ style comments everywhere. @@ +367,5 @@ > + unsigned char *p = (unsigned char *)&sin6->sin6_addr; > + int match = 0; > + /* check if prefix was already found */ > + for (int i=0; i<prefixCount; i++) { > + if (!memcmp(&prefixStore[i], p, prefix/8)) { do we need to worry about comparing non-multiple-of-8 prefixes?
Comment 7•7 years ago
|
||
Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Updated•7 years ago
|
Reporter | ||
Updated•6 years ago
|
Comment 8•5 years ago
|
||
There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:bagder, could you have a look please?
Reporter | ||
Comment 9•5 years ago
|
||
Sorry, I can't recall now why these particular ones were never merged. I have to defer to the existing necko people to decide how to deal with them (as I left the team months ago).
Network ID is still, the last time I discussed it, a dream that hasn't been fully realized and the patches here will not alone make the error rate low enough for the feature to become reliable. There needs to be more (semi-static) data input inserted into the algorithm for it to work decently, basing it on mac address and IPv6 prefix won't be enough. Possibly advertised DNS servers or other DHCP-related details can help.
I'm bouncing the NI over the valentin as a start.
Comment 10•5 years ago
|
||
There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:bagder, could you have a look please?
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
Assignee | ||
Comment 12•5 years ago
|
||
Depends on D34903
Assignee | ||
Comment 13•5 years ago
|
||
Depends on D34904
Comment 14•5 years ago
|
||
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/47f2f3e8d5f9 Figure out network id for IPv6 too (Linux) r=michal https://hg.mozilla.org/integration/autoland/rev/2b23587aa5ff Figure out network id for IPv6 too (mac) r=michal https://hg.mozilla.org/integration/autoland/rev/719b3b61b9a9 Update histogram with two new values for IPv6 network-id changes r=michal
Comment 15•5 years ago
|
||
Backed out 3 changesets for causing bustages in nsNetworkLinkService and nsNotifyAddrListener.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/8537d24d8aa8c0c2189c308d5835def2feec313c
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=c552aeab0fbfa729d61467f507b8a117da77e524&selectedJob=253898337
Failures:
- at nsNetworkLinkService: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253898307&repo=autoland&lineNumber=13913
- at nsNotifyAddrListener.cpp: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253898295&repo=autoland&lineNumber=54677
Comment 16•5 years ago
|
||
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/dd05ca553d8e Figure out network id for IPv6 too (Linux) r=michal https://hg.mozilla.org/integration/autoland/rev/5ecb89d6b4b1 Figure out network id for IPv6 too (mac) r=michal https://hg.mozilla.org/integration/autoland/rev/d3452fb68e51 Update histogram with two new values for IPv6 network-id changes r=michal
Assignee | ||
Updated•5 years ago
|
Comment 17•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dd05ca553d8e
https://hg.mozilla.org/mozilla-central/rev/5ecb89d6b4b1
https://hg.mozilla.org/mozilla-central/rev/d3452fb68e51
Description
•