Closed Bug 971153 Opened 6 years ago Closed 6 years ago

Write a test for bug 751465

Categories

(Core :: Networking: WebSockets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: gk, Assigned: gk)

References

Details

Attachments

(1 file, 5 obsolete files)

We should add a test to make sure that DNS leaks in Websocket code are not happening.
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...
Depends on: 751465
Flags: needinfo?(mcmanus)
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
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.
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..
I forgot to say thank you!

(man, geeks sometimes..)
Flags: needinfo?(mcmanus)
(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?
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.
Flags: needinfo?(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.
Flags: needinfo?(mcmanus)
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.
Attachment #8374313 - Attachment is obsolete: true
Attachment #8387504 - Flags: review?(mcmanus)
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 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
Attachment #8387504 - Flags: review?(mcmanus) → review-
(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.
Okay, I hope I fixed everything. Let me know if something is still missing/wrong.
Assignee: nobody → gk
Attachment #8387504 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8392855 - Flags: review?(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
Attachment #8392855 - Flags: review?(mcmanus)
"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?
Flags: needinfo?(mcmanus)
I don't know off the top of my head.
Flags: needinfo?(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.
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 :)
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...)
Flags: needinfo?(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).
Flags: needinfo?(mcmanus)
(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.
Depends on: 995177
I meant "attach" instead of "apply"...
I enhanced the patch slightly to fix try debug build failures.
Attachment #8392855 - Attachment is obsolete: true
Attachment #8406024 - Flags: review?(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.
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
Attachment #8406024 - Attachment is obsolete: true
Attachment #8406024 - Flags: review?(mcmanus)
Attachment #8406939 - Flags: review?(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
Attachment #8406939 - Flags: review?(mcmanus) → review+
Attachment #8406939 - Attachment is obsolete: true
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d8d1d9108f56
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.