Closed Bug 908740 Opened 9 years ago Closed 9 years ago

Reject obviously bogus STUN and TURN candidates?


(Core :: WebRTC: Networking, defect)

Not set





(Reporter: ekr, Assigned: bwc)



(1 file, 2 obsolete files)

One of the TURN servers I am using is having a bad hair day and decided to return candidates of the form We happily accept these and treat them as the default candidate (per RFC 5245 S 4.1.4 ( However, when we try to setremote, it treats that as their being no media on the m-line and you get a negotiation failure.

Question: how aggressive should we be about checking for bogus candidates? Just this one? 1918 addresses?
Assignee: nobody → docfaraday
Initially, I think it makes sense to check for either or port 0 in isolation is invalid. Going to bake this into the stun code in nICEr, since stuff like this is invalid from a protocol perspective.
Checking for loopback since we already had a function for checking this laying around. Can remove if unpalatable.
Attached patch Return R_BAD_DATA instead of 1. (obsolete) — Splinter Review
Attachment #802480 - Attachment is obsolete: true
Comment on attachment 802491 [details] [diff] [review]
Return R_BAD_DATA instead of 1.

Review of attachment 802491 [details] [diff] [review]:

lgtm with one nit.

Please report on testing against Google.

::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c
@@ +407,5 @@
> +int nr_stun_transport_addr_check(nr_transport_addr* addr)
> +  {
> +    int r;
> +    if((r=nr_transport_addr_is_wildcard(addr)))
> +      return R_BAD_DATA;

Don't assign against r, since we're not using it. Here and below.
Attachment #802491 - Flags: review+
Attachment #802491 - Attachment is obsolete: true
Pushed to try earlier today:
Whiteboard: [checkin-needed]

Please make sure your patches have usable commit messages in the future that describe what the patch is actually doing (not just restating the bug summary). Also, checkin-needed is a bug keyword, not a whiteboard status :)
Whiteboard: [checkin-needed]

This actually is a pretty fair summary of the change. Byron could have written "localhost and wildcard" instead of "obviously bogus", I guess....
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Note: the r+ wasn't moved forward to the "a little more tweaking" patch (nit-fixed patch), so the sheriff checked in the patch with r+.  This isn't a problem for uplift monday; it will at most add a warning for an unused var.  Please throw up a patch to get rid of the unused-var warnings.  If this gets uplifted, propose the "a little more tweaking" patch for uplift.
Resolution: FIXED → ---
Comment on attachment 802501 [details] [diff] [review]
A little more tweaking.

Carry forward r+
Attachment #802501 - Flags: review+
I'm not seeing the difference between the final patch (with nit fixes) and either of the patches applied to inbound and central...
Re-closing; the code on mc looks like this landed fine.
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.