1189961 - AAAA resolution of STUN servers fails

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Nils Ohlmeier [:drno]]

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp?from=nr_netaddr_to_transport_addr&case=true#417

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1189961: added DNS AAAA convertion to nICEr transport addr. r?bwc

Bug 1189961: added DNS AAAA convertion to nICEr transport addr

Comment 2

[Eric Rescorla (:ekr)]

https://reviewboard.mozilla.org/r/14671/#review13233

This seems like it needs a test.

::: media/mtransport/nr_socket_prsock.cpp:418
(Diff revision 1)
> -        ABORT(R_BAD_ARGS);
> +        memcpy(&ip6.s6_addr, &netaddr->inet6.ip, sizeof(ip6.s6_addr));
> +        if ((r = nr_ip6_port_to_transport_addr(&ip6,
> +                                               ntohs(netaddr->inet6.port),
> +                                               protocol, addr)))
> +          ABORT(r);
> +        break;

Is there a reason you don't cast this?

Also, I think I would be happier if you addressed the interior of the union rather than the union.

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8641972 [details]
MozReview Request: Bug 1189961: added DNS AAAA convertion to nICEr transport addr. r?bwc

https://reviewboard.mozilla.org/r/14671/#review13281

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8641972 [details]
MozReview Request: Bug 1189961: added DNS AAAA convertion to nICEr transport addr. r?bwc

https://reviewboard.mozilla.org/r/14671/#review13287

r+ with the fix to the copy.

Comment 5

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/14671/#review13233

> Is there a reason you don't cast this?
> 
> Also, I think I would be happier if you addressed the interior of the union rather than the union.

Yeah, if we're copying to s6_addr, we probably also want to copy from inet6.ip.u8 to be consistent.

Comment 6

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/14671/#review13233

Not sure how a test would look like. I think we only have the ICE unit tests "TestGatherDNSStunServerHostname*" which essentially test end-to-end from DNS name for a STUN server to generating SRV RFLX candidates. To test this code the same way we would need a STUN server with an AAAA name only entry somewhere. Or does someone how another/better idea how to test this?

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8641972 [details]
MozReview Request: Bug 1189961: added DNS AAAA convertion to nICEr transport addr. r?bwc

Bug 1189961: added DNS AAAA convertion to nICEr transport addr. r?bwc

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Does this affect anything before 42?

Comment 9

[Nils Ohlmeier [:drno]]

Yes this has an ABORT() in all previous releases. But as IPv6 support for nICEr only landed in 42 it only makes sense to fix this in 42.

Comment 10

[Nils Ohlmeier [:drno]]

Green try run.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0599674e8edf

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0599674e8edf