Closed Bug 347307 Opened 14 years ago Closed 7 years ago

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

Categories

(Core :: Networking, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: roc, Assigned: mcmanus)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Linux-only hack (obsolete) — Splinter Review
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.
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)
Existing PAC files use myIPAddress(), and we don't want to change them, so I'm not sure what your suggestion means.
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).
What I am trying to do is get more useful behaviour for existing PAC scripts. New APIs are not relevant.
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.
Can you return a nsIDNSRecord, which has support for enumeration?
That would make sense.
Attached patch real patch (obsolete) — Splinter Review
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.
Attachment #239308 - Flags: superreview?
Attachment #239308 - Flags: review?
Attachment #239308 - Flags: superreview?(cbiesinger)
Attachment #239308 - Flags: superreview?
Attachment #239308 - Flags: review?(cbiesinger)
Attachment #239308 - Flags: review?
Seems to me that with this, all those IE extensions would be easy to implement in nsProxyAutoConfig.js
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?
Attachment #239308 - Flags: superreview?(cbiesinger)
Attachment #239308 - Flags: superreview-
Attachment #239308 - Flags: review?(cbiesinger)
Attachment #239308 - Flags: review-
(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.
(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.
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.
Assignee: roc → nobody
roc: Do you forgot this patch ?
Yes, and I stopped caring about it.
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.
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.
see bug 558253
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.
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
No longer blocks: 231115, 242337, 304822, 336589
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.
Assignee: nobody → mcmanus
Duplicate of this bug: 559664
Duplicate of this bug: 629666
Duplicate of this bug: 641134
Duplicate of this bug: 683345
Attached patch patch 0 (obsolete) — Splinter Review
A potential patch.. needs some cross platform testing before r?
Attachment #232076 - Attachment is obsolete: true
Attachment #239308 - Attachment is obsolete: true
that patch is depenedent on bug 769764
Depends on: 769764
Duplicate of this bug: 231115
Duplicate of this bug: 304822
Duplicate of this bug: 336589
Duplicate of this bug: 336235
Duplicate of this bug: 400309
Attachment #658873 - Flags: review?(cbiesinger)
Attached patch patch 1Splinter Review
I've just updated this for bitrot against its 769764 dependency.
Attachment #658873 - Attachment is obsolete: true
Attachment #658873 - Flags: review?(cbiesinger)
Attachment #660226 - Flags: review?(cbiesinger)
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 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
Attachment #660226 - Flags: review?(cbiesinger) → review+
(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.
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 :)
Push backed out for m-oth permaorange in browser_463205.js:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59b9b887c25
https://hg.mozilla.org/mozilla-central/rev/6d71ff5b4b36
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla18 → ---
https://hg.mozilla.org/mozilla-central/rev/345ebb6f3f3e
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 841489
You need to log in before you can comment on or make changes to this bug.