Closed Bug 1264470 Opened 4 years ago Closed 4 years ago
a=identity attribute is truncated, duplicated
58 bytes, text/x-review-board-request
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).
I kinda want to avoid adding more attribute management/parse code to sipcc, but I guess I could go either way on this one.
Review commit: https://reviewboard.mozilla.org/r/47271/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47271/
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?
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.
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?
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+
You need to log in before you can comment on or make changes to this bug.