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

RESOLVED FIXED in mozilla18

Status

()

Core
Networking
RESOLVED FIXED
11 years ago
4 years ago

People

(Reporter: roc, Assigned: mcmanus)

Tracking

(Depends on: 1 bug)

Trunk
mozilla18
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
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.
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.

Comment 4

11 years ago
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.
Ah, OK, sure.

Comment 8

11 years ago
Can you return a nsIDNSRecord, which has support for enumeration?
That would make sense.
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.
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.
Blocks: 304822
Blocks: 336589
Blocks: 231115
Blocks: 242337

Comment 15

9 years ago
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.

Comment 18

6 years ago
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

6 years ago
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

6 years ago
see bug 558253

Comment 21

5 years ago
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
(Assignee)

Comment 23

5 years ago
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)

Updated

5 years ago
Assignee: nobody → mcmanus
(Assignee)

Updated

5 years ago
Duplicate of this bug: 559664
(Assignee)

Updated

5 years ago
Duplicate of this bug: 629666
(Assignee)

Updated

5 years ago
Duplicate of this bug: 641134
(Assignee)

Updated

5 years ago
Duplicate of this bug: 683345
(Assignee)

Comment 28

5 years ago
Created attachment 658873 [details] [diff] [review]
patch 0

A potential patch.. needs some cross platform testing before r?
Attachment #232076 - Attachment is obsolete: true
Attachment #239308 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
that patch is depenedent on bug 769764
Depends on: 769764
(Assignee)

Updated

5 years ago
Duplicate of this bug: 231115
(Assignee)

Updated

5 years ago
Duplicate of this bug: 304822
(Assignee)

Updated

5 years ago
Duplicate of this bug: 336589
(Assignee)

Updated

5 years ago
Duplicate of this bug: 336235
(Assignee)

Updated

5 years ago
Duplicate of this bug: 400309
(Assignee)

Updated

5 years ago
Attachment #658873 - Flags: review?(cbiesinger)
(Assignee)

Comment 35

5 years ago
Created attachment 660226 [details] [diff] [review]
patch 1

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 36

5 years ago
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+
(Assignee)

Comment 38

5 years ago
(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.
(Assignee)

Comment 39

5 years ago
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 :)
(Assignee)

Comment 40

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3182f9d08c2d
Push backed out for m-oth permaorange in browser_463205.js:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59b9b887c25
(Assignee)

Comment 42

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d71ff5b4b36
https://hg.mozilla.org/mozilla-central/rev/6d71ff5b4b36
Status: NEW → RESOLVED
Last Resolved: 5 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 → ---
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/161847c0ac46
(Assignee)

Comment 46

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/345ebb6f3f3e
https://hg.mozilla.org/mozilla-central/rev/345ebb6f3f3e
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 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.