997311 - content/base/test/test_bug338583.html connects to (non-existent) hdfskjghsbg.jtiyoejowe.dafsgbhjab.com

Status: RESOLVED WORKSFORME

(Core :: General, defect)

Description

[Nathan Froyd [:froydnj]]

This test is deliberately triggering DNS lookup errors, so we get an error using an EventSource connecting to it.  Under a no-remote connections scheme, non-existent domains are still non-local domains, so we crash.

I'm not sure what the right thing to do in this case is.  If it's possible, it seems best to modify the crashing check to see if we're connecting to a real thing.

Comment 1

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #0)
> I'm not sure what the right thing to do in this case is.  If it's possible,
> it seems best to modify the crashing check to see if we're connecting to a
> real thing.

Patrick, is there an easy way to do thing in nsSocketTransport2?

Comment 2

[Nathan Froyd [:froydnj]]

Looking at NSPR logging, it looks like the nsresult seen by nsSocketTransport::OnSocketEvent for the hostname resolution is successful, even though the name is completely bogus.  That seems...odd.  Is that a result of using proxies?

Comment 3

[Patrick McManus [:mcmanus]]

I use some variant of the patch below to test for external connections.. does it do the trick? (I stay away from DNS results). Obviously it needs to be based on "enforce this" pref/env-var/something

diff --git a/netwerk/base/src/nsSocketTransport2.cpp b/netwerk/base/src/nsSocketTransport2.cpp
index 58431d4..25d9ae3 100644
--- a/netwerk/base/src/nsSocketTransport2.cpp
+++ b/netwerk/base/src/nsSocketTransport2.cpp
@@ -1177,12 +1177,19 @@ nsSocketTransport::InitiateSocket()

     nsresult rv;

-    if (gIOService->IsOffline()) {
+    if (gIOService->IsOffline() || 1) {
         bool isLocal;

         IsLocal(&isLocal);
-        if (!isLocal)
-            return NS_ERROR_OFFLINE;
+        if (!isLocal) {
+            if (gIOService->IsOffline()) {
+                return NS_ERROR_OFFLINE;
+            }
+            if (NS_SUCCEEDED(mCondition) && !IsIPAddrAny(&mNetAddr)) {
+                fprintf(stderr,"BAD CONNECT: connecting to %s %x\n", mHost.get(), mCondition);
+                MOZ_CRASH("connecting off localhost in test context");
+            }
+        }
     }

     // Hosts/Proxy Hosts that are Local IP Literals should not be speculatively
diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.cpp b/netwerk/protocol/http/nsHttpConnectionMgr.cpp
index 46704d6..417e8a5 100644
--- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -376,6 +376,7 @@ nsHttpConnectionMgr::SpeculativeConnect(nsHttpConnectionInfo *ci,

     LOG(("nsHttpConnectionMgr::SpeculativeConnect [ci=%s]\n",
          ci->HashKey().get()));
+    return NS_OK;

     // Hosts that are Local IP Literals should not be speculatively
     // connected - Bug 853423.
/home/mcmanus/src/mozilla2/wd/gecko-dev>

Comment 4

[Nathan Froyd [:froydnj]]

That doesn't seem to work; I have the equivalent block in my nsSocketTransport2.cpp:

--- a/netwerk/base/src/nsSocketTransport2.cpp
+++ b/netwerk/base/src/nsSocketTransport2.cpp
@@ -1,3 +1,4 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /* vim:set ts=4 sw=4 et cindent: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -1175,14 +1176,22 @@ nsSocketTransport::InitiateSocket()
 {
     SOCKET_LOG(("nsSocketTransport::InitiateSocket [this=%p]\n", this));
 
+    static bool crashOnNonLocalConnections = !!getenv("MOZ_DISABLE_NONLOCAL_CONNECTIONS");
+
     nsresult rv;
+    bool isLocal;
+    IsLocal(&isLocal);
 
     if (gIOService->IsOffline()) {
-        bool isLocal;
-
-        IsLocal(&isLocal);
         if (!isLocal)
             return NS_ERROR_OFFLINE;
+    } else if (!isLocal) {
+        if (NS_SUCCEEDED(mCondition) &&
+            !IsIPAddrAny(&mNetAddr) &&
+            crashOnNonLocalConnections) {
+            fprintf(stderr,"BAD CONNECT: connecting to %s %x\n", mHost.get(), mCondition);
+            MOZ_CRASH("Attempting to connect to non-local address!");
+        }
     }
 
     // Hosts/Proxy Hosts that are Local IP Literals should not be speculatively
	Modified   netwerk/dns/nsHostResolver.cpp

and I still see:

 0:10.46 BAD CONNECT: connecting to hdfskjghsbg.jtiyoejowe.dafsgbhjab.com 0
 0:10.46 Hit MOZ_CRASH(Attempting to connect to non-local address!) at /home/froydnj/src/gecko-dev.git/netwerk/base/src/nsSocketTransport2.cpp:1193

Comment 5

[Patrick McManus [:mcmanus]]

what is printf("%X\n", mNetAddr.inet.ip) ?

I would have expected 0.0.0.0 (IpAddrAny) there..

Comment 6

[Nathan Froyd [:froydnj]]

It is completely bogus:

 fprintf(stderr,"BAD CONNECT: connecting to %s %x %d %X\n", mHost.get(), mCondition, (int)mNetAddrIsSet, mNetAddr.inet.ip);

gives:

 0:10.38 BAD CONNECT: connecting to hdfskjghsbg.jtiyoejowe.dafsgbhjab.com 0 0 8441D743

I see this:

    // mNetAddr is valid from GetPeerAddr() once we have
    // reached STATE_TRANSFERRING. It must not change after that.
    mozilla::net::NetAddr   mNetAddr;
    bool                    mNetAddrIsSet;

and I see that mNetAddr is not initialized in the constructor.  Then again, IsLocal is accessing mNetAddr, so...

Comment 7

[Patrick McManus [:mcmanus]]

I need to find some time to look at this particular test.. it feels like there might be an underlying necko bug in there somewhere - but maybe I'm missing something.

That comment about GetPeerAddr() is just because mNetAddr could be changing due to failover and whatnot and that's technically a free threaded interface. So after mNetAddrIsSet it will be constant and safe to access.. so that's not necessarily the problem. 

probably won't get to it today.

Comment 8

[Nathan Froyd [:froydnj]]

I looked into this some, and I think this might be my own machine; I was using OpenDNS and OpenDNS happily hands out its own domains for failed DNS requests.  IP address 8441D743 is 67.215.65.132, which belongs to opendns.com.

Changing my nameservers in resolv.conf gets me the expected results.  And the test works on try.

We are going to need to note this somewhere, so if people run tests on their own machine and crash, they know they should be running on try...