Closed Bug 1264470 Opened 4 years ago Closed 4 years ago

a=identity attribute is truncated, duplicated

Categories

(Core :: WebRTC, defect, P1)

45 Branch
Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: fluffy, Assigned: mt)

Details

Attachments

(1 file)

In WebRTC when using an identity provider, the UA seems to generate two a=identity lines in the SDP but I am only expecting one. The second line seems correct but the first line seems wrong.  The first one seems to be a copy of the same assertion that is the second a=identity line but truncated to 256 characters. 

Test case reproduces it every time and seems to be the same in FF44 and 45.0.2. It's a little hard for me to extract a simple test case from code that causes this but if folks have problems reproducing this, I can make some example code that shows the problem and share it.
I can confirm this.  https://mozilla.github.io/webrtc-landing/pc_test.html doesn't show the problem, but opening the dev tools (f12) and typing `pc1.localDescription.sdp` shows two a=identity lines.  I have a theory and will investigate.
Assignee: nobody → martin.thomson
Byron, I know exactly what the problem is here, but I'd like your thoughts on how best to fix it.

The problem is a combination of the fact that sipcc doesn't parse strings longer than 256 characters and that we a patching in the a=identity attribute in the localDescription getter.  For a number of reasons (https://github.com/w3c/webrtc-pc/pull/574 being one of them) I would like to move all of this logic into JsepSessionImpl and sdparta.  There is no reason that we should need to patch in a=identity in PeerConnection.js; I definitely want to drop that little wart.

However, that means one of several things: loosen the restriction on string length somehow in sipcc (and probably allocate strings on the heap and deal with all the memory management fallout), or add a new attribute reader and union point for a=identity specifically.  I'm leaning toward the latter.

That means I would add a new option to the attr union on sdp_attr_t that has a type of char *.  Then I would have an extended attribute reader just for identity.  Then I would have to add code to sdp_free_attr to handle that, but we already have special free code for X-cap attributes (yay).
Flags: needinfo?(docfaraday)
I kinda want to avoid adding more attribute management/parse code to sipcc, but I guess I could go either way on this one.
Flags: needinfo?(docfaraday)
Rank: 17
Priority: -- → P1
Comment on attachment 8742545 [details]
MozReview Request: Bug 1264470 - a=identity is a long attribute, r?bwc

https://reviewboard.mozilla.org/r/47271/#review44161

::: media/webrtc/signaling/test/sdp_unittests.cpp:1339
(Diff revision 1)
>    ParseSdp(kBasicAudioVideoOffer);
>    ASSERT_TRUE(!!mSdp) << "Parse failed: " << GetParseErrors();
>    ASSERT_TRUE(mSdp->GetAttributeList().HasAttribute(
>          SdpAttribute::kIdentityAttribute));
>    auto identity = mSdp->GetAttributeList().GetIdentity();
> -  ASSERT_EQ("blahblahblah", identity) << "Wrong identity assertion";
> +  ASSERT_EQ(LONG_IDENTITY, identity) << "Wrong identity assertion";

I'm a little confused; how was the extension stuff even working? Does it work now?
Attachment #8742545 - Flags: review?(docfaraday)
The problem was that sipcc only stores 256 characters.  Anything more is thrown away.  I was patching over that by splicing in an extra a=identity in the pc.localDescription getter.
Summary: WebRTC Identity Assertions in SDP seem wrong → a=identity attribute is truncated, duplicated
Ping for Byron, who cleared the review flag.
Flags: needinfo?(docfaraday)
So the test SDP used to have this in it:

"a=identity:blahblahblah foo;bar" CRLF

And we tested for this:

ASSERT_EQ("blahblahblah", identity) << "Wrong identity assertion";

If the sipcc internal representation is just a string, what happened to the " foo;bar"? It seems that it must have been eaten somehow. Is this still being eaten after this patch?
Flags: needinfo?(docfaraday)
Oh, that.  I believe that the code I have eats the entire line rather than the first attribute.  So " foo;bar" would have been parsed in.  The splitting didn't seem like a feature worth keeping.  WDYT?
Oh, to be clear, the old code ate the " foo;bar", the new code would keep it.
I suspect we're required to ignore that stuff when we don't understand it, so I worry that we'd run into problems someday if we don't trim it off. I'd be happy with us just discarding everything past the " " inside our wrapper code.
The code that processes the attribute does ignore the ' ' and everything past it:

https://dxr.mozilla.org/mozilla-central/source/dom/media/PeerConnectionIdp.jsm#32

The question is what we save.  I saved the entire line, since that lets us use the stuff after the ' ' if we have to without touching sipcc again.
Comment on attachment 8742545 [details]
MozReview Request: Bug 1264470 - a=identity is a long attribute, r?bwc

https://reviewboard.mozilla.org/r/47271/#review46653

Ok, I can live with that.
Attachment #8742545 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d24ae84a5b49
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.