1264470 - a=identity attribute is truncated, duplicated

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Cullen Jennings]

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.

Comment 1

[Martin Thomson [:mt:]]

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.

Comment 2

[Martin Thomson [:mt:]]

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).

Comment 3

[Byron Campen [:bwc]]

I kinda want to avoid adding more attribute management/parse code to sipcc, but I guess I could go either way on this one.

Comment 4

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1264470 - a=identity is a long attribute, r?bwc

Review commit: https://reviewboard.mozilla.org/r/47271/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47271/

Comment 5

[Byron Campen [:bwc]]

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?

Comment 6

[Martin Thomson [:mt:]]

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.

Comment 7

[Martin Thomson [:mt:]]

Ping for Byron, who cleared the review flag.

Comment 8

[Byron Campen [:bwc]]

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?

Comment 9

[Martin Thomson [:mt:]]

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?

Comment 10

[Martin Thomson [:mt:]]

Oh, to be clear, the old code ate the " foo;bar", the new code would keep it.

Comment 11

[Byron Campen [:bwc]]

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.

Comment 12

[Martin Thomson [:mt:]]

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 13

[Byron Campen [:bwc]]

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.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d24ae84a5b49

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d24ae84a5b49