test_speculative_connections.js makes non-local connections

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: sworkman)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox31 fixed, firefox32 fixed, firefox33 fixed, firefox-esr24 unaffected, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

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?
Patrick, do you have an ETA on when this work will be scheduled?
Flags: needinfo?(mcmanus)
Flags: needinfo?(sworkman)
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?
Flags: needinfo?(sworkman)
Flags: needinfo?(mcmanus)
(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.
Flags: needinfo?(sworkman)
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..
(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.
Flags: needinfo?(sworkman)
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.
Attachment #8435244 - Flags: review?(mcmanus)
Attachment #8435244 - Flags: feedback?(nfroyd)
Flags: needinfo?(nfroyd)
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?
Attachment #8435244 - Flags: feedback?(nfroyd) → feedback+
Flags: needinfo?(nfroyd)
Thanks for verifying it works Nathan! Apologies about the other patch bits - thought I had switched queues cleanly, sorry.
Attachment #8435244 - Attachment is obsolete: true
Attachment #8435244 - Flags: review?(mcmanus)
Attachment #8435380 - Flags: review?(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?
Attachment #8435380 - Flags: review?(mcmanus) → review-
(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 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.
Attachment #8435380 - Flags: review- → review+
No worries :) Thanks for r+!
Should have done this earlier. try run: https://tbpl.mozilla.org/?tree=Try&rev=08a36a46cfc2
https://hg.mozilla.org/mozilla-central/rev/cd94a4d8be6f
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.