722197 - Add a preference to map some domain names to localhost

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

The patch add a preference called 'network.dns.localDomains' that allow to map the resolution of some domains to locahost.

Basically I would like to avoid https://wiki.mozilla.org/Gaia/Hacking#Map_.24.7BDOMAIN_NAME.7D_to_localhost and altering all the host system while configuring B2G desktop.

My main question about this code is security. Does it seems to unsecure to have a pref that let the user redirect domain.tld to localhost-only or is it ok?

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I could also add a #ifdef B2G or something similar if that's too scarying.

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 592551 [details] [diff] [review]
Patch

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

Hi Vivien - thanks for the patch!

I'm fine with this in principle. As a matter of fact I want a more general static host mechanism.. would you be willing to change it to be a list of tuples like hostfoo.bar:127.0.0.1, hostbar.foo:192.168.16.1 ?

If you don't have the time to do that, just set r? again and I'll review the code you have in detail. Also, a positive xpcshell test would be good.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Patrick McManus [:mcmanus] from comment #2)
> Comment on attachment 592551 [details] [diff] [review]
> Patch
> 
> Review of attachment 592551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Vivien - thanks for the patch!
> 
> I'm fine with this in principle. As a matter of fact I want a more general
> static host mechanism.. would you be willing to change it to be a list of
> tuples like hostfoo.bar:127.0.0.1, hostbar.foo:192.168.16.1 ?
> 
> If you don't have the time to do that, just set r? again and I'll review the
> code you have in detail. Also, a positive xpcshell test would be good.

I will be a bit short in time to play with tuples but I will try to do an xpcshell test if that can help this feature to land. 
Do you have any suggestions/clues about how I can test that?

Comment 4

[Patrick McManus [:mcmanus]]

> Do you have any suggestions/clues about how I can test that?

nothing special.. get the pref service (prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);), set your pref with a name like localhost.justkidding.invalid and then maybe extend test_dns_service.js to lookup that name and see that it maps correctly? that would make me happy.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2 + test

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 595002 [details] [diff] [review]
Patch v0.2 + test

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

There are some threading problems here. There are multiple parallel resolving threads in play.
 
on reset (change of a pref) mLocalDomains is cleared on the main thread and is unlocked against those resolving threads which are accessing it. Look at how the other member variables are assigned under lock in Init().

similarly the resolving threads access the new localdomains hash without holding the lock (which you'll presumably want to use to clear it in init()). instead of clearing it on change-event, you might make mLocalDomains a refptr to a hashtable so it can be handled like the other member items by just adding a refernece under the lock on the lookup thread and replacing it with a fresh object in init.

other than that, I'm cool with this. go b2g.

Comment 7

[Mike Hommey [:glandium]]

Stupid question: why not simply use a proxy like we do in our test harnesses?

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3 + test

I have tried to use a nsRefPtr<nsTHasttable<nsCStringHashKey> > but it complains at compile time about AddRef/Release. So this patch still use nsTHasttable<nsCStringHashKey> instead but make sure it is accessed under the mutex on all call sites.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Mike Hommey [:glandium] from comment #7)
> Stupid question: why not simply use a proxy like we do in our test harnesses?

It will probably works but I would like something super simple to set up, and just changing a pref sounds the easiest way to it while reducing external dependencies.

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 611852 [details] [diff] [review]
Patch v0.3 + test

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

looks better. Thanks!

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa132abac5a

Comment 12

[Matt Brubeck (:mbrubeck)]

Sorry, this push was backed out on inbound because one of the changesets caused a leak:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92cc2053db2f

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 611852 [details] [diff] [review]
Patch v0.3 + test

I fixed the leak and pushed the patch.

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/3fc635337386

Comment 15

[mathieu]

with the help of http://xip.io/

we can build gaia with:
GAIA_DOMAIN=gaia.127.0.0.1.xip.io

and configure apache/lighttpd/etc accordingly

xip.io custum dns resolve all *.<ip>.xip.io as <ip>


in a local network, for testing from another device, you can also do something like:
GAIA_DOMAIN=gaia.192.168.0.100.xip.io

Comment 16

[[e-mail]]

Comment on attachment 611852 [details] [diff] [review]
Patch v0.3 + test

>+        mLocalDomains.Clear();
>+        if (localDomains) {
>+            nsAdoptingString domains;
>+            domains.AssignASCII(nsDependentCString(localDomains).get());
>+            nsCharSeparatedTokenizer tokenizer(domains, ',',
>+                                               nsCharSeparatedTokenizerTemplate<>::SEPARATOR_OPTIONAL);
>+ 
>+            while (tokenizer.hasMoreTokens()) {
>+                const nsSubstring& domain = tokenizer.nextToken();
>+                mLocalDomains.PutEntry(nsDependentCString(NS_ConvertUTF16toUTF8(domain).get()));
>+            }
>+        }

I stumbled over this because I was looking for unusual nsDependentCString constructors.
I imagine that the patch author was unfamiliar with our string code, which is not unusual, indeed I have only a passing familiarity with nsCharSeparatedTokenizer.
The main flaw here appears to be the use of a Unicode tokeniser on an ASCII string. This allowed the author to come up with some creative conversions.
At some point I'll remember to file a bug to clean this up.

Comment 17

[[e-mail]]

Comment on attachment 611852 [details] [diff] [review]
Patch v0.3 + test

>+    static const nsCString localhost((const char *) "localhost");
Hmm, I thought static instances were frowned upon.

Comment 18

[[e-mail]]

(In reply to comment #17)
> (From update of attachment 611852 [details] [diff] [review])
> >+    static const nsCString localhost((const char *) "localhost");
> Hmm, I thought static instances were frowned upon.
Ah, this explains comment #13 "I fixed the leak".
(And the fix was still wrong, hence bug 1026656.)