Closed Bug 829757 Opened 7 years ago Closed 7 years ago

Accept a=candidate ICE candidates (for now)

Categories

(Core :: WebRTC: Signaling, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: ekr, Assigned: ekr)

Details

(Whiteboard: [webrtc][blocking-webrtc+][qa-])

Attachments

(2 files, 4 obsolete files)

Google emits candidates with "a=...\r\n"

The spec says not to do this but Justin wants to discuss at the meeting.

In the interim, let's accept it.
Assignee: nobody → ekr
Priority: -- → P2
Summary: Accept Google's a=candidate ICE candidates → Accept a=candidate ICE candidates (for now)
Whiteboard: [webrtc][blocking-webrtc+]
Attachment #703356 - Attachment is obsolete: true
Attachment #703424 - Attachment is obsolete: true
Attachment #703425 - Flags: review?(ethanhugg)
Comment on attachment 703425 [details] [diff] [review]
Strip a= from trickle ICE candidates

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

Looks good to me.  Built and ran on Linux, no new warnings.
Attachment #703425 - Flags: review?(ethanhugg) → review+
https://hg.mozilla.org/mozilla-central/rev/7d6a8a229453
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Attached patch Move stripping to SIPCC (obsolete) — Splinter Review
Unfortunately, putting this where we put it doesn't work for candidates received before we CreateAnswer because they are processed via the SDP. See patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #704273 - Flags: review?(adam)
Comment on attachment 704273 [details] [diff] [review]
Move  stripping to SIPCC

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

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3646,3 @@
>      level = msg->data.candidate.level;
>      gsmsdp_set_ice_attribute (SDP_ATTR_ICE_CANDIDATE, level,
> +      dcb->sdp->dest_sdp, candidate);

I don't *think* this is quite right. In the case of "a=<candidate-attribute>\r\n", you're stripping the "a=", but leaving the "\r\n"

If you follow the call chain down through gsmsdp_set_ice_attribute -> sdp_attr_set_ice_attribute, you'll see that we do a straight sstrncpy into attr_p->ice_attr -- including the \r and the \n

If I examine what these strings are supposed to look like, the debugger has:

    ice_attr = "candidate:0 1 UDP 2113601791 192.168.0.240 57162 typ host", '\0' <repeats 199 times>, 


Without the \r\n. I think you need to add logic that searches for a '\r' and replaces it with a '\0' before passing the buffer in to gsmsdp_set_ice_attribute; otherwise it is pretty likely that we'll end up spitting the SDP out with "\r\n\r\n" in it. (Sorry, I've run out of time to verify, but it should be possible to double-check this by adding an ice candidate with a \r\n at the end, and then examining the remote description in the PeerConnectionImpl -- it's certainly possible that I'm wrong here).
I certainly agree that i'm not stripping the \r\n, but I'm not following why this is a problem: the parser is OK with having a \r\n  on the end because it's OK with anything trailing.

WRT the last graf, wouldn't we emit \r\na=\r\n?
Oh, never mind.  I see your point about the SDP generator. I had forgotten we needed that to work as well. I'll add code to strip it.
Attached patch Move stripping to SIPCC (obsolete) — Splinter Review
Attachment #704273 - Attachment is obsolete: true
Attachment #704273 - Flags: review?(adam)
Attachment #704582 - Flags: review?(adam)
Comment on attachment 704582 [details] [diff] [review]
Move  stripping to SIPCC

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

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3634,5 @@
>      /* Update remote SDP with new candidate information */
> +    candidate = (char *)msg->data.candidate.candidate;
> +    if (strlen(candidate) < 2) {
> +      FSM_DEBUG_SM(DEB_F_PREFIX"bogus candidate value.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> +      return SM_RC_CLEANUP;

I think we want SM_RC_END here. I believe that returning SM_RC_CLEANUP would result in the whole call going away, which is a bit severe for this kind of syntax error (at least, that's my understanding -- we might want to run this by Ethan to be sure).

The need to perform this length check is also not obvious. If we actually need it, please add a comment indicating why.
OK, I can live with END. The length check is an abundance of caution. I could ok with removing it.
That has the nice side effect of ignoring the error check question.
Attachment #704582 - Attachment is obsolete: true
Attachment #704582 - Flags: review?(adam)
Comment on attachment 704605 [details] [diff] [review]
Move  stripping to SIPCC

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

one more time
Attachment #704605 - Flags: review?(adam)
Comment on attachment 704605 [details] [diff] [review]
Move  stripping to SIPCC

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

Looks good to me.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3630,5 @@
>      /* comment until mid is properly updated
>      cause = gsmsdp_find_level_from_mid(dcb, (const char *)msg->data.candidate.mid, &level);
>      */
>  
> +    /* XXX: Need to sanity check that this is actually a valid candidate */

This comment would be better if it contained a bug number -- I agree that this check is a necessary improvement; we should make sure it's tracked.
Attachment #704605 - Flags: review?(adam) → review+
https://hg.mozilla.org/mozilla-central/rev/77802f9c2b7f
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.