Last Comment Bug 347307 - Need a way to determine the best local IP address for PAC files to use
: Need a way to determine the best local IP address for PAC files to use
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: x86 Linux
: -- normal with 4 votes (vote)
: mozilla18
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
: 231115 304822 336235 336589 400309 559664 629666 641134 683345 (view as bug list)
Depends on: 841489 769764
Blocks: 231115 242337 304822 336589
  Show dependency treegraph
 
Reported: 2006-08-03 22:10 PDT by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2013-05-27 14:52 PDT (History)
28 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Linux-only hack (8.79 KB, patch)
2006-08-03 22:12 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
real patch (20.82 KB, patch)
2006-09-19 22:18 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
cbiesinger: review-
cbiesinger: superreview-
Details | Diff | Splinter Review
patch 0 (18.60 KB, patch)
2012-09-06 07:26 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
patch 1 (18.68 KB, patch)
2012-09-11 14:29 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-08-03 22:10:40 PDT
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) (Exited; email my personal email if necessary) 2006-08-03 22:12:50 PDT
Created attachment 232076 [details] [diff] [review]
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) 2006-08-03 22:25:50 PDT
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) (Exited; email my personal email if necessary) 2006-08-03 22:37:50 PDT
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 2006-08-03 22:59:57 PDT
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) (Exited; email my personal email if necessary) 2006-08-06 13:40:23 PDT
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) 2006-08-06 15:26:52 PDT
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) (Exited; email my personal email if necessary) 2006-08-06 17:36:52 PDT
Ah, OK, sure.
Comment 8 Darin Fisher 2006-08-07 11:07:52 PDT
Can you return a nsIDNSRecord, which has support for enumeration?
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-08-07 16:15:19 PDT
That would make sense.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-09-19 22:18:19 PDT
Created attachment 239308 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2006-09-19 22:21:24 PDT
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) 2006-10-03 19:14:06 PDT
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) (Exited; email my personal email if necessary) 2006-10-03 20:01:44 PDT
(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) 2006-10-03 20:51:16 PDT
(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 2008-04-02 15:32:10 PDT
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] 2010-08-18 10:07:37 PDT
roc: Do you forgot this patch ?
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-18 16:46:44 PDT
Yes, and I stopped caring about it.
Comment 18 Matthieu Patou 2011-01-22 01:27:25 PST
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 2011-03-02 08:08:36 PST
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 2011-03-09 10:00:45 PST
see bug 558253
Comment 21 Andi Kleen 2012-05-20 11:32:02 PDT
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] 2012-05-20 14:12:07 PDT
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] 2012-09-02 09:27:18 PDT
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 24 Patrick McManus [:mcmanus] 2012-09-04 07:42:24 PDT
*** Bug 559664 has been marked as a duplicate of this bug. ***
Comment 25 Patrick McManus [:mcmanus] 2012-09-04 08:24:59 PDT
*** Bug 629666 has been marked as a duplicate of this bug. ***
Comment 26 Patrick McManus [:mcmanus] 2012-09-04 08:28:23 PDT
*** Bug 641134 has been marked as a duplicate of this bug. ***
Comment 27 Patrick McManus [:mcmanus] 2012-09-04 09:38:51 PDT
*** Bug 683345 has been marked as a duplicate of this bug. ***
Comment 28 Patrick McManus [:mcmanus] 2012-09-06 07:26:31 PDT
Created attachment 658873 [details] [diff] [review]
patch 0

A potential patch.. needs some cross platform testing before r?
Comment 29 Patrick McManus [:mcmanus] 2012-09-06 07:28:09 PDT
that patch is depenedent on bug 769764
Comment 30 Patrick McManus [:mcmanus] 2012-09-06 07:35:51 PDT
*** Bug 231115 has been marked as a duplicate of this bug. ***
Comment 31 Patrick McManus [:mcmanus] 2012-09-06 07:35:53 PDT
*** Bug 304822 has been marked as a duplicate of this bug. ***
Comment 32 Patrick McManus [:mcmanus] 2012-09-06 07:35:56 PDT
*** Bug 336589 has been marked as a duplicate of this bug. ***
Comment 33 Patrick McManus [:mcmanus] 2012-09-06 07:35:59 PDT
*** Bug 336235 has been marked as a duplicate of this bug. ***
Comment 34 Patrick McManus [:mcmanus] 2012-09-06 07:36:02 PDT
*** Bug 400309 has been marked as a duplicate of this bug. ***
Comment 35 Patrick McManus [:mcmanus] 2012-09-11 14:29:22 PDT
Created attachment 660226 [details] [diff] [review]
patch 1

I've just updated this for bitrot against its 769764 dependency.
Comment 36 fieldhouse 2012-09-12 06:31:16 PDT
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) 2012-09-12 13:46:17 PDT
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] 2012-09-12 14:19:46 PDT
(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] 2012-09-12 14:30:14 PDT
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] 2012-09-13 12:28:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3182f9d08c2d
Comment 41 Ed Morley [:emorley] 2012-09-13 14:48:52 PDT
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] 2012-09-14 13:51:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d71ff5b4b36
Comment 43 Ryan VanderMeulen [:RyanVM] 2012-09-14 18:15:30 PDT
https://hg.mozilla.org/mozilla-central/rev/6d71ff5b4b36
Comment 44 Ed Morley [:emorley] 2012-09-17 09:40:47 PDT
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] 2012-09-17 12:21:56 PDT
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/161847c0ac46
Comment 46 Patrick McManus [:mcmanus] 2012-09-27 10:36:44 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/345ebb6f3f3e
Comment 47 Ryan VanderMeulen [:RyanVM] 2012-09-27 20:15:50 PDT
https://hg.mozilla.org/mozilla-central/rev/345ebb6f3f3e

Note You need to log in before you can comment on or make changes to this bug.