Closed Bug 1189198 Opened 9 years ago Closed 9 years ago

UDP stun client sends messages to TCP STUN server

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: drno, Assigned: drno)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

      No description provided.
Rank: 20
Bug 1189198: don't start STUN transactions with a protocol mis-match
Attachment #8640957 - Flags: review?(docfaraday)
https://reviewboard.mozilla.org/r/14405/#review13019

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:556
(Diff revision 1)
>          if(cand->stun_server->type == NR_ICE_STUN_SERVER_TYPE_ADDR) {
>            if(cand->base.ip_version != cand->stun_server->u.addr.ip_version) {
>              r_log(LOG_ICE, LOG_INFO, "ICE-CANDIDATE(%s): Skipping srflx/relayed candidate with different IP version (%u) than STUN/TURN server (%u).", cand->label,cand->base.ip_version,cand->stun_server->u.addr.ip_version);
>              ABORT(R_NOT_FOUND); /* Same error code when DNS lookup fails */
>            }
>  
> +          if(cand->base.protocol != cand->stun_server->u.addr.protocol) {
> +            r_log(LOG_ICE, LOG_INFO, "ICE-CANDIDATE(%s): Skipping srflx/relayed candidate with different transport (%u) than STUN/TURN server (%u).", cand->label,cand->base.protocol,cand->stun_server->u.addr.protocol);
> +            ABORT(R_NOT_FOUND); /* Same error code when DNS lookup fails */
> +          }
> +

Is there a reason not to use nr_transport_addr_cmp() here and above, rather than reaching into the structs? That's kind of the intention.
Comment on attachment 8640957 [details]
MozReview Request: Bug 1189198: don't start STUN transactions with a protocol mis-match

https://reviewboard.mozilla.org/r/14405/#review13037

Looks good to me, although using nr_transport_addr_cmp is a good idea.
Attachment #8640957 - Flags: review?(docfaraday) → review+
Comment on attachment 8640957 [details]
MozReview Request: Bug 1189198: don't start STUN transactions with a protocol mis-match

Bug 1189198: don't start STUN transactions with a protocol mis-match
(In reply to Eric Rescorla (:ekr) from comment #2)
> Is there a reason not to use nr_transport_addr_cmp() here and above, rather
> than reaching into the structs? That's kind of the intention.

No other excuse then being lazy and copying the if condition from before. Replaced both with a single call to nr_transport_addr_cmp().
https://reviewboard.mozilla.org/r/14405/#review13113

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:557
(Diff revision 2)
> -          if(cand->base.ip_version != cand->stun_server->u.addr.ip_version) {
> -            r_log(LOG_ICE, LOG_INFO, "ICE-CANDIDATE(%s): Skipping srflx/relayed candidate with different IP version (%u) than STUN/TURN server (%u).", cand->label,cand->base.ip_version,cand->stun_server->u.addr.ip_version);
> +          if(nr_transport_addr_cmp(&cand->base,&cand->stun_server->u.addr,NR_TRANSPORT_ADDR_CMP_MODE_PROTOCOL)) {
> +            r_log(LOG_ICE, LOG_INFO, "ICE-CANDIDATE(%s): Skipping srflx/relayed candidate because of IP version/transport mis-match with STUN/TURN server (%u/%u - %u/%u).", cand->label,cand->base.ip_version,cand->base.protocol,cand->stun_server->u.addr.ip_version,cand->stun_server->u.addr.protocol);

It would be nicer if this message indicated the protocols by string rather than by number, but this is acceptable as-is.

Also, I wonder if this should be at DEBUG?
Comment on attachment 8640957 [details]
MozReview Request: Bug 1189198: don't start STUN transactions with a protocol mis-match

Bug 1189198: don't start STUN transactions with a protocol mis-match
Try run looks green.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/20e92855a2e7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: