791270 - WebRTC crash [@sipcc::PeerConnectionImpl::AddStream]

Status: VERIFIED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

The parameter of addStream() is not properly validated.

Comment 1

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: Protect AddStream from NULL input

Comment 3

[Ethan Hugg [:ehugg]]

Comment on attachment 669728 [details] [diff] [review]
Protect AddStream from NULL input


Change assert on null input to AddStream to be a runtime failure.

Test case should be changed to mozRTCPeerConnection to test on latest.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 669728 [details] [diff] [review]
Protect AddStream from NULL input

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

Since xpconnect already throws an exception when we return an error here, r+

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ae33bc85be5

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Has this been landed for Firefox 18? Please set the TM accordingly. Thanks.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Landed for 19, and we should consider for uplift to 18

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7ae33bc85be5

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Crashtest v1

If this patch fixes the crash completely the following try run should be successful: https://tbpl.mozilla.org/?tree=Try&rev=da75e3bd42d9

Once the patch has been reviewed and we are green we are good to land on m-c.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Created attachment 671657 [details] [diff] [review]
> Crashtest v1
> 
> If this patch fixes the crash completely the following try run should be
> successful: https://tbpl.mozilla.org/?tree=Try&rev=da75e3bd42d9
> 
> Once the patch has been reviewed and we are green we are good to land on m-c.

If the try run passes, can mark this as verified?

Comment 12

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #11)
> (In reply to Henrik Skupin (:whimboo) from comment #10)
> > Created attachment 671657 [details] [diff] [review]
> > Crashtest v1
> > 
> > If this patch fixes the crash completely the following try run should be
> > successful: https://tbpl.mozilla.org/?tree=Try&rev=da75e3bd42d9
> > 
> > Once the patch has been reviewed and we are green we are good to land on m-c.
> 
> If the try run passes, can mark this as verified?

Actually, change that. I just ran the attached test case and it looked alright.

Comment 13

[Jason Smith [:jsmith]]

Wait a second, didn't test this right.

Comment 14

[Jason Smith [:jsmith]]

Now I did this right this time. Looks good.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d27abf64482

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d27abf64482