971153 - Write a test for bug 751465

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Georg Koppen]

Attachment: Test idea for websocket dns leak test

We should add a test to make sure that DNS leaks in Websocket code are not happening.

Comment 1

[Georg Koppen]

No sure whether there is a better way to get out of the loop without leaking things. I had a hard time finding the chan.close() workaround. But I hope there is something I missed.

Is the place okay, (netwerk/test/unit) given that all the other websocket tests are in content? I thought the DNS leak test would make more sense in netwerk...

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8374313 [details] [diff] [review]
Test idea for websocket dns leak test

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

::: netwerk/test/unit/test_dns_leak_websocket.js
@@ +5,5 @@
> +var ioService = Cc["@mozilla.org/network/io-service;1"].
> +  getService(Ci.nsIIOService);
> +// DNS resolution locally does not work if we are offline. We try to exploit
> +// that for our test.
> +ioService.offline = true;

see below - this needs to be reset at the end of the test

@@ +13,5 @@
> +
> +var prefs = Cc["@mozilla.org/preferences-service;1"].
> +  getService(Ci.nsIPrefBranch);
> +
> +var url = "ws://dnsleak.testing.com";

dnsleak.example.com

@@ +34,5 @@
> +  onStart: function(aContext) {
> +  },
> +
> +  onStop: function(aContext, aStatusCode) {
> +    do_test_finished();

cancel the timer if its still outstanding

@@ +43,5 @@
> +  observe: function(subject, topic, data) {
> +    try {
> +      // Calling close() here as a means of getting out of the event loop
> +      // without leaking.
> +      chan.close(3999, "DNS leak!!!");

nsIWebsocketChannel.CLOSE_ABNORMAL

@@ +59,5 @@
> +  prefs.setCharPref("network.proxy.socks", "127.0.0.1");
> +  prefs.setIntPref("network.proxy.socks_port", 9000);
> +  prefs.setIntPref("network.proxy.type", 1);
> +  prefs.setBoolPref("network.proxy.socks_remote_dns", true);
> +  do_check_true(ioService.offline);

you need to push all these settings into local vars before you change them and then pop them back when your test finishes because each test does not necessarily get a new gecko instance and you don't want to screw up the subsequent tests

@@ +65,5 @@
> +    createInstance(Ci.nsIWebSocketChannel);
> +  var uri = ioService.newURI(url, null, null);
> +  chan.asyncOpen(uri, url, listener, null);
> +  // If we are still not done after 2 sec, we got stuck in the event loop:
> +  // DNS resolution was about to take place locally. Make the test fail then.

timers just don't work as heuristics. We just can't take this test into the tree like this - it will inevitably generate both false positives and negatives.

Comment 3

[Patrick McManus [:mcmanus]]

for clarity let's not call this a leak.. from a gecko standpoint that means a resource leak (ram, filedescriptors, etc..) that will lead to a crash. you're talking about something else.. "unexpected network activity" or somesuch is more clear.

(In reply to Georg Koppen from comment #1)
>
> Is the place okay, (netwerk/test/unit) given that all the other websocket
> tests are in content? I thought the DNS leak test would make more sense in
> netwerk...

that's fine..

Comment 4

[Patrick McManus [:mcmanus]]

I forgot to say thank you!

(man, geeks sometimes..)

Comment 5

[Georg Koppen]

(In reply to Patrick McManus [:mcmanus] from comment #2)
> @@ +65,5 @@
> > +    createInstance(Ci.nsIWebSocketChannel);
> > +  var uri = ioService.newURI(url, null, null);
> > +  chan.asyncOpen(uri, url, listener, null);
> > +  // If we are still not done after 2 sec, we got stuck in the event loop:
> > +  // DNS resolution was about to take place locally. Make the test fail then.
> 
> timers just don't work as heuristics. We just can't take this test into the
> tree like this - it will inevitably generate both false positives and
> negatives.

Yes, I thought so, too. The question is then: Is there a sane way to get out of the event loop without leaks I could try and that would get accepted? Or do I need to look for a totally different approach for this test?

Comment 6

[Georg Koppen]

Ah, and I forgot to say "Thank you" as well (you know, geeks sometimes...). I'll fix all the things you pointed out provided there is a good solution to avoid the timer (achieving the same aim) without the need to redesign the whole test.

Comment 7

[Patrick McManus [:mcmanus]]

I think if you want to test that dns lookups aren't happening you're going to need to run the test and then when it finishes check the dns log.

of course there is no dns log in the necko sense :) so you'll need to implement something.

I bet you could have the dns system publish an event to nsIObserver listeners for every resolution attempt it makes. obviously that needs to be off by default, but your test could flip a testing pref on and then observe dns-resolution-request or something like that.. making sure that the name isn't used in the stream of observations that it gets before the websocket is complete.

Comment 8

[Georg Koppen]

Attachment: enhanced WebSocket proxy bypass test

The updated patch implements the observer idea and hopefully addresses all the previous feedback. I made the name of the test file more generic as I can think of adding more specific proxy bypass tests now that we have the notifying observer implemented. Not sure whether the SOCKS proxy port is okay as I don't know what is running on which port on Mozilla's test machines.

Comment 9

[Georg Koppen]

Hmm... maybe it would be better to put the first call to cleanupOnFinish() inside the if-clause checking for "dnsleak.example.com" (although it doesn't seem to matter much currently)?

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8387504 [details] [diff] [review]
enhanced WebSocket proxy bypass test

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

This is great - thanks for doing this. I can think of a number of other DNS tests I want to write using the structure.

There is one problem that drives the r-.. basing cleanup on http-on-modify-request is a little sketchy.. as long as asyncOpen doesn't throw an exception the channel will call OnStopRequest and OnStartRequest.. can you bind the end of test logic to OnStopRequest?

I think with that change and the nits below we can land this! sound good?

::: netwerk/dns/nsDNSService2.cpp
@@ +398,5 @@
>  
> +class NotifyDNSResolution: public nsRunnable
> +{
> +public:
> +    NotifyDNSResolution(nsIObserverService* aObs, nsAutoCString &aHostname)

nsACtring for the parameter

@@ +629,3 @@
>  
>  NS_IMETHODIMP
>  nsDNSService::AsyncResolve(const nsACString  &hostname,

probly need this in ::resolve() as well

@@ +636,5 @@
>  {
> +    if (mNotifyResolution) {
> +        nsAutoCString host(hostname);
> +        nsCOMPtr<nsIObserverService> observerService =
> +            mozilla::services::GetObserverService();

set this up to be mObserverService in ::Init() the way mIDN is done but conditional on mNotifyResolution

Comment 11

[Georg Koppen]

(In reply to Patrick McManus [:mcmanus] from comment #10)
> Comment on attachment 8387504 [details] [diff] [review]
> enhanced WebSocket proxy bypass test
> 
> Review of attachment 8387504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is great - thanks for doing this. I can think of a number of other DNS
> tests I want to write using the structure.
> 
> There is one problem that drives the r-.. basing cleanup on
> http-on-modify-request is a little sketchy.. as long as asyncOpen doesn't
> throw an exception the channel will call OnStopRequest and OnStartRequest..
> can you bind the end of test logic to OnStopRequest?

I did that originally. Surprisingly, I encountered and still encounter at least one dns-resolution-request notification with dnsleak.example.com as hostname which breaks this test on trunk. Thus, I implemented this, well, shortcut. Seems I need to do some further investigation on why this is happening in the first place...

> I think with that change and the nits below we can land this! sound good?

Yes.
 
> ::: netwerk/dns/nsDNSService2.cpp
> @@ +398,5 @@
> >  
> > +class NotifyDNSResolution: public nsRunnable
> > +{
> > +public:
> > +    NotifyDNSResolution(nsIObserverService* aObs, nsAutoCString &aHostname)
> 
> nsACtring for the parameter

Okay.
 
> @@ +629,3 @@
> >  
> >  NS_IMETHODIMP
> >  nsDNSService::AsyncResolve(const nsACString  &hostname,
> 
> probly need this in ::resolve() as well

Well, yes. Although this is strictly speaking a different bug (which is the reason why I omitted it in this patch). I'll add it in the next one.
 
> @@ +636,5 @@
> >  {
> > +    if (mNotifyResolution) {
> > +        nsAutoCString host(hostname);
> > +        nsCOMPtr<nsIObserverService> observerService =
> > +            mozilla::services::GetObserverService();
> 
> set this up to be mObserverService in ::Init() the way mIDN is done but
> conditional on mNotifyResolution

Okay.

Comment 12

[Georg Koppen]

Attachment: WebSocket proxy bypass test -- third try

Okay, I hope I fixed everything. Let me know if something is still missing/wrong.

Comment 13

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=a7eb3e7f399a

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 8392855 [details] [diff] [review]
WebSocket proxy bypass test -- third try

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

test failed on try.

https://tbpl.mozilla.org/php/getParsedLog.php?id=36353326&tree=Try&full=1

Comment 15

[Georg Koppen]

"reference to undefined property Ci.nsIWebSocketChannel" does not make much sense to me, especially as this is a non-issue locally. Is this a bug in the test infrastructure? And any ideas on how to fix this?

Comment 16

[Patrick McManus [:mcmanus]]

I don't know off the top of my head.

Comment 17

[Patrick McManus [:mcmanus]]

you might need

var Ci = Components.interfaces;

that could depend on the version of xpcshell that is being used by the test infra.. I've heard rumors it is dated.

just a guess.

Comment 18

[Georg Koppen]

Okay, I added

  var test = Cc["@mozilla.org/network/protocol;1?name=ws"].
    createInstance();
  for each (i in Components.interfaces) {
    if (test instanceof i) {
      try {
        do_print("Supported interface: " + i + "\n");
      } catch (e) {}
    }
  }

in my test and the only difference I got from try was the missing nsIWebSocketChannel which is likely the reason for the failure. I have no clue how this can happen given that try and my local computer are supposed to compile the same code :)

Comment 19

[Georg Koppen]

Andy ideas, Patrick? (No hurry, you are probably busy with other stuff, but I still have the hope to get that patch in ESR 31...)

Comment 20

[Patrick McManus [:mcmanus]]

I don't know. If I get some time I'll try and debug it, but its not high on the list.

as a shot in the dark you can retry with the patch from 995177 (or wait for it to be merged).

Comment 21

[Georg Koppen]

(In reply to Patrick McManus [:mcmanus] from comment #20)
> as a shot in the dark you can retry with the patch from 995177 (or wait for
> it to be merged).

Woohoo! You were right. Applying the fix made the try build green. I'll apply a slightly enhanced patch on Monday for review.

Comment 22

[Georg Koppen]

I meant "attach" instead of "apply"...

Comment 23

[Georg Koppen]

Attachment: Websocket proxy bypass test -- fourth try

I enhanced the patch slightly to fix try debug build failures.

Comment 24

[Georg Koppen]

Passed on try: https://tbpl.mozilla.org/?tree=Try&rev=7c57cc558357

Comment 25

[Patrick McManus [:mcmanus]]

Comment on attachment 8406024 [details] [diff] [review]
Websocket proxy bypass test -- fourth try

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

sorry :gk - gonna need a quick update for the nsMainThreadPtrHandle issue.. I just missed it last time around.

::: netwerk/dns/nsDNSService2.cpp
@@ +402,5 @@
> +        : mObs(aObs)
> +        , mHostname(aHostname) {}
> +
> +    NS_IMETHOD Run()
> +    {

MOZ_ASSERT(NS_IsMainThread());

@@ +410,5 @@
> +        return NS_OK;
> +    }
> +
> +private:
> +    nsCOMPtr<nsIObserverService> mObs;

This needs to be a nsMainThreadPtrHandle<nsIObserverService> because it is adding and removing refs of a main thread xpcom obj off the main thread.

sorry I didn't notice that earlier. See nsHttpHandler.cpp for an example.

@@ +411,5 @@
> +    }
> +
> +private:
> +    nsCOMPtr<nsIObserverService> mObs;
> +    nsAutoCString                mHostname;

generally we only use autocstring for stack allocation.. just cstring is the right thing here.

Comment 26

[Georg Koppen]

Attachment: Websocket proxy bypass test -- fifth try

Okay. So, I fixed the issues you had with the last patch (I hope :) ). Try is green again (after fiddling a lot with nsMainThreadPtrHandle (a thing new to me)...:

https://tbpl.mozilla.org/?tree=Try&rev=782acc7de5cc

Comment 27

[Patrick McManus [:mcmanus]]

Comment on attachment 8406939 [details] [diff] [review]
Websocket proxy bypass test -- fifth try

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

a couple nits here that you can address at your discretion - r+

please get a more complete try run before landing to look for regressions.. you're changing DNS after all:)
try: -b d -p linux -u all -t none

::: netwerk/dns/nsDNSService2.cpp
@@ +662,5 @@
>          localDomain = mLocalDomains.GetEntry(hostname);
>      }
> +
> +    if (mNotifyResolution) {
> +        nsAutoCString host(hostname);

why are you doing this host(hostname) copy to the stack? Can't you just pass it to the NotifyDNSResolution ctor if you change the argument to that function to be const? Same thing in ::Resolve()

@@ +664,5 @@
> +
> +    if (mNotifyResolution) {
> +        nsAutoCString host(hostname);
> +        NS_DispatchToMainThread(new NotifyDNSResolution(mObserverService,
> +                                                            host));

indent

@@ +775,5 @@
> +
> +    if (mNotifyResolution) {
> +        nsAutoCString host(hostname);
> +        NS_DispatchToMainThread(new NotifyDNSResolution(mObserverService,
> +                                                            host));

indent

Comment 28

[Georg Koppen]

Attachment: Websocket proxy bypass test -- final patch

Comment 29

[Georg Koppen]

(In reply to Patrick McManus [:mcmanus] from comment #27)
> a couple nits here that you can address at your discretion - r+
> 
> please get a more complete try run before landing to look for regressions..
> you're changing DNS after all:)

What could possibly go wrong? ;)

> try: -b d -p linux -u all -t none

Everything is green: https://tbpl.mozilla.org/?tree=Try&rev=25e6c5b75097
 
> ::: netwerk/dns/nsDNSService2.cpp
> @@ +662,5 @@
> >          localDomain = mLocalDomains.GetEntry(hostname);
> >      }
> > +
> > +    if (mNotifyResolution) {
> > +        nsAutoCString host(hostname);
> 
> why are you doing this host(hostname) copy to the stack? Can't you just pass
> it to the NotifyDNSResolution ctor if you change the argument to that
> function to be const? Same thing in ::Resolve()

Done, good idea. Not sure why I did that in the first place. IIRC I had issues handling the string properly due to my bad Mozilla string knowledge (seems I have to read the internal string guide another 10 times :) )...
 
> @@ +664,5 @@
> > +
> > +    if (mNotifyResolution) {
> > +        nsAutoCString host(hostname);
> > +        NS_DispatchToMainThread(new NotifyDNSResolution(mObserverService,
> > +                                                            host));
> 
> indent

Fixed.
 
> @@ +775,5 @@
> > +
> > +    if (mNotifyResolution) {
> > +        nsAutoCString host(hostname);
> > +        NS_DispatchToMainThread(new NotifyDNSResolution(mObserverService,
> > +                                                            host));
> 
> indent

Fixed.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d1d9108f56

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d8d1d9108f56