347307 - Need a way to determine the best local IP address for PAC files to use

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The PAC utility function myIPAddress() just resolves the local hostname to some IP address and returns that. In some configurations this will return something like 127.0.0.1 which is fairly useless. We would prefer to return the address of a non-loopback interface if one is available, and we would prefer the address of a VPN interface over a wired or wireless interface.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Linux-only hack

This does it for Linux, but we really need a configure test here to see if getifaddrs is available. If it isn't, we should fall back to the current approach.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

have you considered using a list of IP addresses instead, sorted by preference or something? (note: there are some suggested PAC extensions that would need a list)

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Existing PAC files use myIPAddress(), and we don't want to change them, so I'm not sure what your suggestion means.

Comment 4

[Darin Fisher]

This appears to be what IE7 will support:
http://blogs.msdn.com/wndp/articles/IPV6_PAC_Extensions_v0_9.aspx

Also, if you need these kinds of capabilities in FF 1.5+, it is possible to configure proxies via an extension (see nsIProtocolProxyFilter).

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What I am trying to do is get more useful behaviour for existing PAC scripts. New APIs are not relevant.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

I didn't suggest changing what the PAC API exposes; I just meant that the nsPIDNSService change should maybe use an array to be more future-proof.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, OK, sure.

Comment 8

[Darin Fisher]

Can you return a nsIDNSRecord, which has support for enumeration?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That would make sense.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: real patch

Okay, here's a real patch. There's a configure test to detect getifaddrs (although we only use it in XP_UNIX). We build an nsIDNSRecord. To avoid a new implementation of nsIDNSRecord I modified the existing one to keep the list of addresses internally.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems to me that with this, all those IE extensions would be easy to implement in nsProxyAutoConfig.js

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 239308 [details] [diff] [review]
real patch

configure.in
+AC_CACHE_VAL(ac_cv_getifaddrs,[

OK, I suppose there's a reason why you don't just use:
  AC_CHECK_FUNCS(getifaddrs)
? That'd also be nicer for cross-compiling.

netwerk/dns/src/nsDNSService2.cpp
+nsDNSRecord::Init()
+{
+    if (!mHostRecord)
+        return NS_OK;

when would this be null?

+nsDNSRecord::Init()

it would be good to add a comment why you don't need to do the addr/addr_info check inside the loop (because the addr case is only hit for literal IP addresses (and the bug 290190 case), right?)

+                rv = rec->Init();
+                if (NS_SUCCEEDED(rv)) {
+                    NS_ADDREF(*result = rec);

you need to delete rec if that failed

+    if (strchr(aInterface->ifa_name, ':')) {
+        // guess that it's probably a VPN address

why's that? it might be good to put an example name here.

+    return PRNetAddr(*NS_REINTERPRET_CAST(PRNetAddr*, aAddr));

hm, the documentation says:
"PRNetAddr is binary-compatible with the socket address structures in the familiar Berkeley socket interface, although this fact should not be relied upon"
(http://developer.mozilla.org/en/docs/PRNetAddr#Description)

I guess you don't care? :)

+nsDNSService::GetInterfaceAddresses(nsIDNSRecord** aResult)
+{
+    nsRefPtr<nsDNSRecord> rec = new nsDNSRecord(nsnull);

is there a reason not to allocate this closer to where it is first used?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #12)
> (From update of attachment 239308 [details] [diff] [review] [edit])
> configure.in
> +AC_CACHE_VAL(ac_cv_getifaddrs,[
> 
> OK, I suppose there's a reason why you don't just use:
>   AC_CHECK_FUNCS(getifaddrs)
> ? That'd also be nicer for cross-compiling.

Because I didn't know it existed. I'll do that.

> netwerk/dns/src/nsDNSService2.cpp
> +nsDNSRecord::Init()
> +{
> +    if (!mHostRecord)
> +        return NS_OK;
> 
> when would this be null?

Here:
+nsDNSService::GetInterfaceAddresses(nsIDNSRecord** aResult)
+{
+    nsRefPtr<nsDNSRecord> rec = new nsDNSRecord(nsnull);

i.e. when we populate the record with explicit AddAddress calls.

> +nsDNSRecord::Init()
> 
> it would be good to add a comment why you don't need to do the addr/addr_info
> check inside the loop (because the addr case is only hit for literal IP
> addresses (and the bug 290190 case), right?)

I don't think this needs commenting. addr_info is a collection of addresses, so we have to iterate through it. addr is a single address. This should be clear from the types of those variables.

> +                rv = rec->Init();
> +                if (NS_SUCCEEDED(rv)) {
> +                    NS_ADDREF(*result = rec);
> 
> you need to delete rec if that failed

OK

> +    if (strchr(aInterface->ifa_name, ':')) {
> +        // guess that it's probably a VPN address
> 
> why's that? it might be good to put an example name here.

OK. Some VPNs create psuedointerfaces such as "eth0:1".

> +    return PRNetAddr(*NS_REINTERPRET_CAST(PRNetAddr*, aAddr));
> 
> hm, the documentation says:
> "PRNetAddr is binary-compatible with the socket address structures in the
> familiar Berkeley socket interface, although this fact should not be relied
> upon"
> (http://developer.mozilla.org/en/docs/PRNetAddr#Description)
> 
> I guess you don't care? :)

Not so much. Can you suggest a better approach?

> +nsDNSService::GetInterfaceAddresses(nsIDNSRecord** aResult)
> +{
> +    nsRefPtr<nsDNSRecord> rec = new nsDNSRecord(nsnull);
> 
> is there a reason not to allocate this closer to where it is first used?

No, I'll do that.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #13)
> > when would this be null?
> 
> Here:
> +nsDNSService::GetInterfaceAddresses(nsIDNSRecord** aResult)
> +{
> +    nsRefPtr<nsDNSRecord> rec = new nsDNSRecord(nsnull);
> 
> i.e. when we populate the record with explicit AddAddress calls.

No, that doesn't call Init().

> I don't think this needs commenting. addr_info is a collection of addresses, so
> we have to iterate through it. addr is a single address. This should be clear
> from the types of those variables.

Oh, indeed. You're right.

> OK. Some VPNs create psuedointerfaces such as "eth0:1".

OK. I wonder if we should also do something special with ppp0 (dialup or PPTP VPN).

> Not so much. Can you suggest a better approach?

No, I can't really think of a better one. Converting to a string and then using PR_StringToNetAddr is probably not worth it. I guess you could make a new nsIDNSRecord implementation that uses the result of getifaddrs directly, and inet_ntoa (does that work for IPv6?).

But I don't mind if you leave it as-is.

Comment 15

[Dan Mellem]

I mentioned this on bug 231115 for MacOS, but it applies here, too:

If there's more than one IP address, why not use the one that's associated with the default route (netstat -r)?  E.g., if Default points to en0 (or eth0 for Linux), return the first IP address on en0/eth0.

Comment 16

[Matthias Versen [:Matti]]

roc: Do you forgot this patch ?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, and I stopped caring about it.

Comment 18

[Matthieu Patou]

Any news on this patch or another one ? 
This bug is reported also in ubuntu:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/228850

In a corporate environment it's really annoying so if it could be fixed it would be very good.

For the record I have a simplier patch that fix the problem just for Linux by avoiding returning of 127.0.0.1 if any other address is available.

Comment 19

[David von Oheimb]

I just found a very helpful hint at http://ubuntuforums.org/showthread.php?t=826495
Simply by removing in /etc/hosts the line "127.0.1.1 hostname david-laptop"
stopped myIPAddress() from returning the useless address 127.0.1.1,
and now I get the IP address that I'd expect, e.g. assigned by DHCP :-)
So it seems to me that it's not really a Mozilla bug but a widespread system misconfiguration on Linux.

Comment 20

[Jo Hermans]

see bug 558253

Comment 21

[Andi Kleen]

So what's up with the patches? This is still as broken as ever. The original patches in 304822 were available in 2006 and still no progress ?!? Can the obvious patches from 304822 please merged?

And no changing /etc/hosts is not a solution, this breaks all kinds of other programs when the system is disconnected.

You guys make my corporate IT department appear efficient.

Comment 22

[Matthias Versen [:Matti]]

The patches got a negative review and the patch author doesn't intend to update them.
Someone else have to update the patch and request a new review.
Note: There are other issues with myIPAddress() like returning ::1 or link-local

Comment 23

[Patrick McManus [:mcmanus]]

I'm going to write a patch that takes a different approach to fixing this:

first, try to 'connect' a UDP socket to the host in the url that is causing the pac execution and pull the local address out of the socket. UDP doesn't have a handshake so this won't create any network traffic or firewall problems - but it will cause the OS to consult its routing tables and figure out for us which of its interfaces it would want to use (vpn, multiple nics, etc..) Nice and portable without any OS specific implementaitons and better than what we have now even when it doesn't return localhost.

That will work well for most situations. There still exists the outside possibility of a host without DNS access (normally provided by the proxy) or even without a default route.

So if using the hostname in the url fails, retry the udp check using a common internet pre-resolved address - 8.8.8.8 (the google anycast dns server) seems reasonable. (again, no actual packets are created)

If we've still got nothing, use the current algorithm and return the result if it is not localhost (v4 or v6).

if we're still stuck try the udp approach using a couple differnet rfc 1918 addresses as targets.

if we're still stuck return localhost

its not perfect, but its a lot better than what we've got.

Comment 28

[Patrick McManus [:mcmanus]]

Attachment: patch 0

A potential patch.. needs some cross platform testing before r?

Comment 29

[Patrick McManus [:mcmanus]]

that patch is depenedent on bug 769764

Comment 35

[Patrick McManus [:mcmanus]]

Attachment: patch 1

I've just updated this for bitrot against its 769764 dependency.

Comment 36

[fieldhouse]

Comment on attachment 660226 [details] [diff] [review]
patch 1

Review of attachment 660226 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me - 1 suggestion & 2 nits.

::: netwerk/base/src/ProxyAutoConfig.cpp
@@ +743,5 @@
> +  if (MyIPAddressTryHost(mRunningHost, kTimeout, vp))
> +    return true;
> +
> +  // next, look for a route to a public internet address that doesn't need DNS
> +  remoteDottedDecimal.Assign("8.8.8.8");

Even if anyone who's working on this should recognise the address 8.8.8.8, perhaps there should be a comment saying what it is (and therefore why it's more durable than eg a mozilla.* IP address). As it's a sort of external dependency, maybe make it a ProxyAutoConfig private const static member in the .h with the comment there? Eg in the header

// a public IPv4 address expected to be reachable "forever"
static const char szKnownPublicIP[] = "8.8.8.8";   // rely on Google Anycast DNS

and then here ...

remoteDottedDecimal.Assign(szKnownPublicIP);

::: netwerk/base/src/ProxyAutoConfig.h
@@ +16,5 @@
>  namespace mozilla { namespace net {
>  
>  class JSRuntimeWrapper;
>  
>  // The ProxyAutoConfig class is meant to be created an run on a

an -> and

@@ +80,5 @@
>  
>  private:
> +  const static unsigned int kTimeout = 250; // ms to allow for myipaddress dns queries
> +
> +  // used to compizle the PAC file and setup the execution context

compizle -> compile

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 660226 [details] [diff] [review]
patch 1

Review of attachment 660226 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/ProxyAutoConfig.cpp
@@ +630,5 @@
>    JSString *hostString =
>      JS_NewStringCopyZ(mJSRuntime->Context(), aTestHost.get());
>    jsval argv[2] = { STRING_TO_JSVAL(uriString), STRING_TO_JSVAL(hostString) };
>    jsval rval;
> +  bool ok = JS_CallFunctionName(mJSRuntime->Context(), mJSRuntime->Global(),

Shouldn't this stay a JSBool?

@@ +706,5 @@
> +
> +  return true;
> +}
> +
> +// hostName is run through a dns lookup and then a udp socket is connected

It makes me a little uncomfortable that we leak host names to the local DNS server whenever a PAC file uses myIpAddress(), but maybe that's OK - you need to trust your PAC author anyway. And this is the only way to handle IPv6 "correctly".

@@ +715,5 @@
> +ProxyAutoConfig::MyIPAddressTryHost(const nsCString &hostName,
> +                                    unsigned int timeout,
> +                                    jsval *vp)
> +{
> +  PRNetAddr     remoteAddress;

you should either line up all three variables here, or none of them. I vote for none.

::: netwerk/base/src/ProxyAutoConfig.h
@@ +78,5 @@
>                            const nsCString &aTestHost,
>                            nsACString &result);
>  
>  private:
> +  const static unsigned int kTimeout = 250; // ms to allow for myipaddress dns queries

That seems really short. I'd increase this to one second

Comment 38

[Patrick McManus [:mcmanus]]

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #37)
> >  private:
> > +  const static unsigned int kTimeout = 250; // ms to allow for myipaddress dns queries
> 
> That seems really short. I'd increase this to one second

my concern here was making pac unusable if you don't have working DNS even if you're not explicitly calling a dns resolution function.. but if we want this to work reliably for v6, a larger value is necessary and 250 is probably still too big to make anything work well when everything is timing out anyhow.

so 1000 it is.

if it proves to be a problem we can add some kind of heuristic to just skip step 1 if we've seen a run of timeouts.

Comment 39

[Patrick McManus [:mcmanus]]

Dirk, thanks for reading the code! Hope to see more of you around these parts.

(In reply to fieldhouse from comment #36)
> Comment on attachment 660226 [details] [diff] [review]

> > +  // used to compizle the PAC file and setup the execution context
> 
> compizle -> compile

oops! It's really more like compuzzle anyhow :)

Comment 40

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3182f9d08c2d

Comment 41

[Ed Morley [:emorley]]

Push backed out for m-oth permaorange in browser_463205.js:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59b9b887c25

Comment 42

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d71ff5b4b36

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d71ff5b4b36

Comment 44

[Ed Morley [:emorley]]

I've had to back this and two other changesets in the same push out, for extremely frequent OS X 10.8 mochitest-other leaks of the form found in bug 773255:
https://tbpl.mozilla.org/php/getParsedLog.php?id=15272317&tree=Mozilla-Inbound

http://brasstacks.mozilla.com/orangefactor/?display=Bug&tree=trunk&startday=2012-09-01&endday=2012-09-17&bugid=773255

https://hg.mozilla.org/integration/mozilla-inbound/rev/161847c0ac46

Comment 45

[Ed Morley [:emorley]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/161847c0ac46

Comment 46

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/345ebb6f3f3e

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/345ebb6f3f3e