1011503 - test_speculative_connections.js makes non-local connections

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nathan Froyd [:froydnj]]

From bug 995417 comment 12, with the patches from that bug applied (I will refresh part 2 in a moment so that xpcshell tests actually get the appropriate environment variable set):

I am seeing test_speculative_connect.js xpcshell failures locally:

 7:48.00 TEST-PASS | /opt/build/froydnj/build-mc/_tests/xpcshell/netwerk/test/unit/test_speculative_connect.js | [test_hostnames_resolving_to_addresses : 204] 1 == 1
 7:48.00 BAD CONNECT: connecting to 10.0.0.1 0
 7:48.00 TEST-PASS | /opt/build/froydnj/build-mc/_tests/xpcshell/netwerk/test/unit/test_speculative_connect.js | [test_hostnames_resolving_to_addresses : 207] "object" != undefined
 7:48.00 TEST-PASS | /opt/build/froydnj/build-mc/_tests/xpcshell/netwerk/test/unit/test_speculative_connect.js | [test_hostnames_resolving_to_addresses : 212] "object" != undefined
 7:48.00 <<<<<<<
PROCESS-CRASH | /opt/build/froydnj/build-mc/_tests/xpcshell/netwerk/test/unit/test_speculative_connect.js | application crashed [Unknown top frame]

Happens on try, too:

https://tbpl.mozilla.org/php/getParsedLog.php?id=39672912&tree=Try

Should we be checking for IsIPAddrLocal in addition to IsLoopBackAddress:

http://mxr.mozilla.org/mozilla-central/source/netwerk/dns/DNS.cpp#181

?

And from bug 995417 comment 13:

(In reply to Nathan Froyd (:froydnj) from comment #12)
> I am seeing test_speculative_connect.js xpcshell failures locally:

I think that test does a bunch of off host things well beyond the rfc 1918 stuff (that we should keep around). can you file a separate bug about it, assign to :sworkman and cc me?

Comment 1

[Nathan Froyd [:froydnj]]

Patrick, do you have an ETA on when this work will be scheduled?

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Sorry for the delay here.

If I understand this correctly, I think adding a check for !IsIPAddrLocal to your branch in nsSocketTransport::InitiateSocket would be good. This should still crash on external connection attempts, but should be ok for connection attempts in the local network.

Question - is this code to be permanent in nsSocketTransport::InitiateSocket? Or is it just means to catch external connections now so that external access can be restricted/disabled at the network level?

Like Pat, I'd like to keep the external connections defined here: http://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_speculative_connect.js#46, e.g. google.com.

From what I've read of the related bugs, there was to be a restricted whitelist of external network accesses. Can the hosts used in this test be part of that? Or can you tell us which these hosts are so we can rewrite the test?

And speaking of whitelists, if you are keeping that code in nsSocketTransport::InitiateSocket, it will need to check the whitelist itself, no?

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Steve Workman [:sworkman] from comment #2)
> If I understand this correctly, I think adding a check for !IsIPAddrLocal to
> your branch in nsSocketTransport::InitiateSocket would be good. This should
> still crash on external connection attempts, but should be ok for connection
> attempts in the local network.

OK, great.

> Question - is this code to be permanent in
> nsSocketTransport::InitiateSocket? Or is it just means to catch external
> connections now so that external access can be restricted/disabled at the
> network level?

Yes, the intention is that the code in InitiateSocket would be permanent, guarded by an environment variable that our testsuites set.

> Like Pat, I'd like to keep the external connections defined here:
> http://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/
> test_speculative_connect.js#46, e.g. google.com.

OK.  Can you elaborate on the rationale for doing this?  We've spent some time removing external connections to *.google.com, *.mozilla.*, etc. elsewhere and I'd like to understand why this test should be considered different.

> From what I've read of the related bugs, there was to be a restricted
> whitelist of external network accesses. Can the hosts used in this test be
> part of that? Or can you tell us which these hosts are so we can rewrite the
> test?

Where did you see this?  I don't recall seeing a discussion of such a whitelist.  We have a proxy redirection scheme for a whitelist-like selection of origins for mochitests, but those connections are still localhost-only.

Comment 4

[Patrick McManus [:mcmanus]]

I think its ok to allow the 1918 addresses to the extent that this test is specifically testing 1918 semantcs (ther are some associated with speculative connections), but not the general internet routable ones..

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Nathan Froyd (:froydnj) from comment #3)
> (In reply to Steve Workman [:sworkman] from comment #2)
> > Like Pat, I'd like to keep the external connections ...
> 
> OK.  Can you elaborate on the rationale for doing this? ...

So, if Pat's ok with not testing the external speculative connections (comment 4), that's fine by me. I'll work up a patch and upload it shortly. It will still need !IsIPAddrLocal() to be added to the condition, but that should stop the failures.

> > From what I've read of the related bugs, there was to be a restricted
> > whitelist of external network accesses...
> 
> Where did you see this?  I don't recall seeing a discussion of such a
> whitelist.  We have a proxy redirection scheme for a whitelist-like
> selection of origins for mochitests, but those connections are still
> localhost-only.

Ah, I may have misunderstood that. No worries.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Remove external addresses from test_speculative_connect.js

Nathan, here's a patch to try out to see if it solves the problem. It removes all remote/external tests from test_speculative_connect.js.

Runs fine locally, but I don't have your patches applied here.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8435244 [details] [diff] [review]
v1.0 Remove external addresses from test_speculative_connect.js

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

This works for me if I add an IsIPAddrLocal check in InitiateSocket.  Thanks for the quick turnaround!

::: netwerk/protocol/websocket/WebSocketChannelChild.h
@@ +23,5 @@
>   public:
>    WebSocketChannelChild(bool aSecure);
>    ~WebSocketChannelChild();
>  
> +  NS_DECL_THREADSAFE_ISUPPORTS

Presumably this is part of a different patch?

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Remove external addresses from test_speculative_connect.js

Thanks for verifying it works Nathan! Apologies about the other patch bits - thought I had switched queues cleanly, sorry.

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 8435380 [details] [diff] [review]
v1.1 Remove external addresses from test_speculative_connect.js

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

this doesn't leave any positive test cases :)

I think we need to add a pref that controls whether or not we filter speculative connections to local(). (right now iirc that's true all the time right?). then we can write one test
that does sc to localhost and test it. sound good?

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Patrick McManus [:mcmanus] from comment #9)
> Comment on attachment 8435380 [details] [diff] [review]
> v1.1 Remove external addresses from test_speculative_connect.js
> 
> Review of attachment 8435380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this doesn't leave any positive test cases :)
> 
> I think we need to add a pref that controls whether or not we filter
> speculative connections to local(). (right now iirc that's true all the time
> right?). then we can write one test
> that does sc to localhost and test it. sound good?

I left the function test_speculative_connect which is a positive test case for localhost and should do what you suggest, no?

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 8435380 [details] [diff] [review]
v1.1 Remove external addresses from test_speculative_connect.js

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

steve - sorry for a moment I thought that localhost was filtered out as part of the 1918 stuff.. but assuming its not (which I now think you're right) we're good. sorry about that.

Comment 12

[Steve Workman [:sworkman] (INACTIVE)]

No worries :) Thanks for r+!

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

Should have done this earlier. try run: https://tbpl.mozilla.org/?tree=Try&rev=08a36a46cfc2

Comment 14

[Steve Workman [:sworkman] (INACTIVE)]

On inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd94a4d8be6f

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b40d5a52870b
https://hg.mozilla.org/releases/mozilla-beta/rev/8151e54f1500
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ab0f0b3787fa
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/728a5643db0f