Add a preference to map some domain names to localhost

RESOLVED FIXED in mozilla15

Status

()

Core
Networking
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 592551 [details] [diff] [review]
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?
Attachment #592551 - Flags: review?(mcmanus)
I could also add a #ifdef B2G or something similar if that's too scarying.
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.
Attachment #592551 - Flags: review?(mcmanus)
(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?
> 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.
Created attachment 595002 [details] [diff] [review]
Patch v0.2 + test
Attachment #592551 - Attachment is obsolete: true
Attachment #595002 - Flags: review?(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.
Attachment #595002 - Flags: review?(mcmanus) → review-
Stupid question: why not simply use a proxy like we do in our test harnesses?
Created attachment 611852 [details] [diff] [review]
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.
Attachment #595002 - Attachment is obsolete: true
Attachment #611852 - Flags: review?(mcmanus)
(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 on attachment 611852 [details] [diff] [review]
Patch v0.3 + test

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

looks better. Thanks!
Attachment #611852 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa132abac5a
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla14
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
Target Milestone: mozilla14 → ---
Comment on attachment 611852 [details] [diff] [review]
Patch v0.3 + test

I fixed the leak and pushed the patch.
Attachment #611852 - Flags: checkin+
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/3fc635337386
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 15

5 years ago
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 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 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.
(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.)
You need to log in before you can comment on or make changes to this bug.