Closed Bug 1395914 Opened 2 years ago Closed 7 months ago

Network id: attempt to identify IPv6 network

Categories

(Core :: Networking, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
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.
Assignee: nobody → daniel
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
First out, the Linux version.

(I've worked out and tested the logic with this command-line tool: https://github.com/bagder/gw-mac)
Attachment #8904168 - Flags: review?(valentin.gosu)
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?
Attachment #8904168 - Flags: review?(valentin.gosu) → review+
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.
Attachment #8904168 - Attachment is obsolete: true
Attachment #8904528 - Flags: review+
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
Attachment #8906561 - Flags: review?(valentin.gosu)
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?
Attachment #8906561 - Flags: review?(valentin.gosu) → review+
Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Priority: P1 → P2
Assignee: daniel → nobody
Status: ASSIGNED → NEW

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?

Flags: needinfo?(daniel)

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.

Flags: needinfo?(daniel) → needinfo?(valentin.gosu)

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?

Flags: needinfo?(daniel)
Flags: needinfo?(daniel)
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-active] → [necko-triaged]
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
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
Flags: needinfo?(valentin.gosu)
You need to log in before you can comment on or make changes to this bug.