Missing byte-order conversion in ChildDNSRecord::GetNextAddr

VERIFIED FIXED in Firefox 32

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

Trunk
mozilla34
Other
All
Points:
---

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 verified, firefox33 verified, firefox34 verified, firefox-esr24 wontfix, firefox-esr31 wontfix, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 wontfix, b2g-v1.3 wontfix, b2g-v1.3T wontfix, b2g-v1.4 affected, b2g-v2.0 verified, b2g-v2.1 fixed)

Details

(Whiteboard: [loop-ft])

Attachments

(1 attachment)

Assignee

Description

5 years ago
http://dxr.mozilla.org/mozilla-central/source/netwerk/dns/DNSRequestChild.cpp#90

|port| is host-byte-order, NetAddr.inet.port is network-byte-order.
Assignee

Updated

5 years ago
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Attachment #8461043 - Flags: review?(mcmanus)
Assignee

Updated

5 years ago
Blocks: 1042345
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.0?
Comment on attachment 8461043 [details] [diff] [review]
Add the appropriate byte-order conversion to ChildDNSRecord::GetNextAddr.

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

lgtm.. sanity checks by matching nsSocketTransport2 behavior
Attachment #8461043 - Flags: review?(mcmanus) → review+
Assignee

Comment 4

5 years ago
Let me make sure we don't need to pick up an include for htons here. Needinfo myself to check back on try push.

https://tbpl.mozilla.org/?tree=Try&rev=f91beec58461
Flags: needinfo?(docfaraday)
Assignee

Comment 5

5 years ago
Looks good, requesting checkin.
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
Comment on attachment 8461043 [details] [diff] [review]
Add the appropriate byte-order conversion to ChildDNSRecord::GetNextAddr.

Approval Request Comment
[Feature/regressing bug #]: N/A (likely always in the E10S/DNS-STUN/TURN)

[User impact if declined]: Inability to connect to STUN/TURN servers using DNS names instead of raw IP addresses.

[Describe test coverage new/current, TBPL]: Manual testing - it's hard to test STUN/TURN in the current setup; this should be testable under Steeplechase on desktop E10S.   Very hard to test on B2G in automation.

[Risks and why]: No risk

[String/UUID change made/needed]: none
Attachment #8461043 - Flags: approval-mozilla-beta?
Attachment #8461043 - Flags: approval-mozilla-aurora?
Severity: normal → critical
https://hg.mozilla.org/mozilla-central/rev/03c685fb1d75
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
See https://bugzilla.mozilla.org/show_bug.cgi?id=1042345#c8 for blocking rationale.
blocking-b2g: 2.0? → 2.0+
QA Whiteboard: [qa+]
QA Contact: anthony.s.hughes
(In reply to Randell Jesup [:jesup] from comment #7)
> [Describe test coverage new/current, TBPL]: Manual testing - it's hard to
> test STUN/TURN in the current setup; this should be testable under
> Steeplechase on desktop E10S.   Very hard to test on B2G in automation.

Nils, can you please advise if this is testable by us?
Flags: needinfo?(drno)
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #10)
> Nils, can you please advise if this is testable by us?

Please test it manually like this:

1a) On Desktop open a e10s browser window
1b) On B2G simply open the Firefox browser

2) Open this URL: http://mozilla.github.io/webrtc-landing/stun_test_ip.html
3) After about 5s you should see output under the headline. The output should have line likes this at the end:
  a=candidate:0 1 UDP 2130379007 10.0.0.9 60122 typ host
  a=candidate:1 1 UDP 1694236671 76.103.213.96 60122 typ srflx raddr 10.0.0.9 rport 60122
Important is that you should have at least one line with "typ host" at the end, and at least one line with "typ srflx" in it.
4) Now open this URL: http://mozilla.github.io/webrtc-landing/stun_test_ip.html
5) Again after about 5s you should see the similar output as in 3). If this output is missing any line with "typ srflx" (and only has "typ host" lines) in it that is test failure. This fix works if the output again like in step 3) contains "typ host" AND "typ srflx" lines.
Flags: needinfo?(drno)
Comment on attachment 8461043 [details] [diff] [review]
Add the appropriate byte-order conversion to ChildDNSRecord::GetNextAddr.

Aurora+ and Beta+

Note that this fix is already clear to land on b2g32 via the 2.0+ flag.
Attachment #8461043 - Flags: approval-mozilla-beta?
Attachment #8461043 - Flags: approval-mozilla-beta+
Attachment #8461043 - Flags: approval-mozilla-aurora?
Attachment #8461043 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/fe0621be0dc3

If you want a decision on esr31/b2g30, you should probably nominate the patch for approval so it shows up on RelMan's radar.
Duplicate of this bug: 1042345
No longer blocks: 1042345
Whiteboard: [webrtc-uplift][loop-ft] → [loop-ft]
On the latest mozilla-central (2014-08-11) I get the following output:
> Created offer: v=0
> o=Mozilla-SIPUA-34.0a1 9788 0 IN IP4 0.0.0.0
> s=SIP Call
> t=0 0
> a=ice-ufrag:c1ea9b0b
> a=ice-pwd:4608618001e85642b6cb6ac3287ad532
> a=fingerprint:sha-256 BD:8D:26:EF:34:47:14:EC:3C:18:B0:31:DF:0A:32:7E:45:D1:71:37:AF:5A:2C:A2:52:AE:86:A0:A8:95:BE:F0
> m=video 52393 RTP/SAVPF 120
> c=IN IP4 64.214.126.165
> a=rtpmap:120 VP8/90000
> a=sendrecv
> a=rtcp-fb:120 nack
> a=rtcp-fb:120 nack pli
> a=rtcp-fb:120 ccm fir
> a=setup:actpass
> a=candidate:0 1 UDP 2130379007 10.244.25.141 57711 typ host
> a=candidate:1 1 UDP 1694236671 64.214.126.165 52393 typ srflx raddr 10.244.25.141 rport 57711
> a=candidate:2 1 UDP 2129723647 192.168.172.1 60129 typ host
> a=candidate:4 1 UDP 2129264895 172.16.109.1 50386 typ host
> a=candidate:0 2 UDP 2130379006 10.244.25.141 54393 typ host
> a=candidate:1 2 UDP 1694236670 64.214.126.165 43354 typ srflx raddr 10.244.25.141 rport 54393
> a=candidate:2 2 UDP 2129723646 192.168.172.1 53863 typ host
> a=candidate:4 2 UDP 2129264894 172.16.109.1 60877 typ host
> a=rtcp-mux 

Does that look right?
Flags: needinfo?(drno)
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #16)
> On the latest mozilla-central (2014-08-11) I get the following output:
> > a=candidate:1 1 UDP 1694236671 64.214.126.165 52393 typ srflx raddr 10.244.25.141 rport 57711
> > a=candidate:1 2 UDP 1694236670 64.214.126.165 43354 typ srflx raddr 10.244.25.141 rport 54393
>
> Does that look right?

Yes. The two lines above indicate it is working.
Flags: needinfo?(drno)
Thanks Nils, I get a similar result on my Flame running yesterday's 2.0-pre build.
Status: RESOLVED → VERIFIED
Verified fixed in today's Aurora and Firefox 32.0b5.
This doesn't meet ESR landing criteria.
You need to log in before you can comment on or make changes to this bug.