1155965 - crash in mozilla::JsepSessionImpl::ValidateLocalDescription(mozilla::Sdp const&)

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Martijn Wargers (dead)]

Attachment: crash2.htm

This bug was filed from the Socorro interface and is 
report bp-fdd59d43-a292-45a8-8ef3-8efb92150418.
=============================================================
0 	XUL 	mozilla::JsepSessionImpl::ValidateLocalDescription(mozilla::Sdp const&) 	media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
1 	XUL 	mozilla::JsepSessionImpl::SetLocalDescription(mozilla::JsepSdpType, std::string const&) 	media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
2 	XUL 	mozilla::PeerConnectionImpl::SetLocalDescription(int, char const*) 	media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
3 	XUL 	mozilla::dom::PeerConnectionImplBinding::setLocalDescription 	media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
4 	XUL 	mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp
5 		@0x109ee598d 	
6 		@0x123893c6f 	
7 		@0x109a8b810 	
8 	XUL 	EnterBaseline 	js/src/jit/BaselineJIT.cpp
9 	XUL 	js::jit::EnterBaselineMethod(JSContext*, js::RunState&) 	js/src/jit/BaselineJIT.cpp
10 	XUL 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp

I get this crash after a while, with the attached testcase. You need to have the specialpowers extension installed to see the crash:
http://people.mozilla.org/~mwargers/extensions/specialpowers/specialpowers.htm

Comment 1

[Martijn Wargers (dead)]

Regression from bug 1091242?

Comment 2

[Byron Campen [:bwc]]

It seems that somehow you're calling setLocal without calling CreateOffer...

Comment 3

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://1155965/bwc

/r/7257 - Bug 1155965: Verify that CreateOffer/Answer has been called when SetLocal is called.

Pull down this commit:

hg pull -r ecbbe2d52972f8d2d24db595bf27187376844516 https://reviewboard-hg.mozilla.org/gecko/

Comment 4

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=357dd57c3ce2

Comment 5

[Eric Rescorla (:ekr)]

Attachment: MozReview Request: bz://1155965/ekr

/r/7261 - Test for bug 1155965

Pull down this commit:

hg pull -r d684aa6d4fdef8f4cdbe19db61e51ddf7e9ada54 https://reviewboard-hg.mozilla.org/gecko/

Comment 6

[Eric Rescorla (:ekr)]

I can verify that the signaling tests crash with my test without your patch but do not with your patch.

Comment 7

[Byron Campen [:bwc]]

I will write a unit-test for this tomorrow.

Comment 8

[Eric Rescorla (:ekr)]

(In reply to Byron Campen [:bwc] from comment #7)
> I will write a unit-test for this tomorrow.

You shouldn't need to. I wrote one already in signaling_unittests

Comment 9

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #8)
> (In reply to Byron Campen [:bwc] from comment #7)
> > I will write a unit-test for this tomorrow.
> 
> You shouldn't need to. I wrote one already in signaling_unittests

Hmm. I'll look over there and try to determine why it did not catch this, and move it to jsep_session_unittest.

Comment 10

[Eric Rescorla (:ekr)]

Sorry, I mean I attached one to this bug above and it validates that your patch removes the problem.

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8594532 [details]
MozReview Request: bz://1155965/bwc

/r/7257 - Bug 1155965: Verify that CreateOffer/Answer has been called when SetLocal is called.

Pull down this commit:

hg pull -r 7287940001df4c57d4660b79c4c753af69672426 https://reviewboard-hg.mozilla.org/gecko/

Comment 12

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/7255/#review6051

> This violates the WebRTC specification which requires that the offer be valid for the duration of the CreateOffer callback. In this case, if you were to do 
> CreateOffer.then(function(r) {
>   SetLocal(r);
>   SetLocal(r);
> });
> 
> This would fail

This already won't work because we don't allow SetLocal more than once, nor have we ever allowed it. Once we have rollback support we can support this kind of thing easily. I guess what we want to do is clear it out once we finish offer/answer.

Comment 13

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/7255/#review6055

> Sorry but this is wrong. The JSEP spec explicitly allows you to call SetLocal() repeatedly and this code would forbid that.
> 
> The naive thing to do is probably just to leave this value in place and then rely on the other state checks to stop you from calling SetLocal() in other wrong states.

I wrote and pushed this updated patch before I saw your earlier comment (failure after ValidateLocalDescription would require creating a new offer/answer, but failure earlier would not, which is screwy). I'm rebasing and moving this to the end of HandleNegotiatedSession now.

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8594532 [details]
MozReview Request: bz://1155965/bwc

/r/7257 - Bug 1155965: Verify that CreateOffer/Answer has been called when SetLocal is called.

Pull down this commit:

hg pull -r 701c80c461c010ddbdaee5f27f10f3377d99dc64 https://reviewboard-hg.mozilla.org/gecko/

Comment 15

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/7259/#review6063

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1526
(Diff revision 1)
> +  // wrt the 

Looks like this snuck in.

::: media/webrtc/signaling/test/signaling_unittests.cpp:3090
(Diff revision 1)
> +  agent(1)->SetLocal(TestObserver::OFFER, agent(0)->offer(),

We should check that this fails, I think. Also, maybe this test belongs in jsep_session_unittest, but not too particular about that.

Comment 16

[Nils Ohlmeier [:drno]]

Comment on attachment 8594532 [details]
MozReview Request: bz://1155965/bwc

https://reviewboard.mozilla.org/r/7255/#review6949

Ship It!

Comment 17

[Nils Ohlmeier [:drno]]

Has anyone checked that we properly reject the same thing for setRemoteDescription()?

Comment 18

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/7259/#review6951

::: media/webrtc/signaling/test/signaling_unittests.cpp:3083
(Diff revision 1)
> +TEST_F(SignalingAgentTest, SetLocal) {
> +  CreateAgent(TestStunServer::GetInstance()->addr(),
> +              TestStunServer::GetInstance()->port());
> +  CreateAgent(TestStunServer::GetInstance()->addr(),
> +              TestStunServer::GetInstance()->port());
> +  OfferOptions options;
> +  agent(0)->CreateOffer(options, OFFER_AUDIO);
> +  agent(1)->SetLocal(TestObserver::OFFER, agent(0)->offer(),
> +                     true, PCImplSignalingState::SignalingStable);
> +}
> +

Just to be safe I think we should have the same test for the answerer side.

Comment 19

[Nils Ohlmeier [:drno]]

(In reply to Nils Ohlmeier [:drno] from comment #17)
> Has anyone checked that we properly reject the same thing for
> setRemoteDescription()?

Disregard. I meant the answerer side. So sLD right after sRD, without calling CreateAnswer() in between.

Comment 20

[Byron Campen [:bwc]]

I can add that test.

Comment 21

[Byron Campen [:bwc]]

Comment on attachment 8594532 [details]
MozReview Request: bz://1155965/bwc

/r/8725 - Bug 1155965 - Part 1: Test-cases.
/r/7257 - Bug 1155965 - Part 2: Verify that CreateOffer/Answer has been called when SetLocal is called.

Pull down these commits:

hg pull -r d4ee034d788fb0f7f95cb80f447a94e4ad7d309c https://reviewboard-hg.mozilla.org/gecko/

Comment 22

[Byron Campen [:bwc]]

Part 1 is what needs review, I haven't changed part 2.

Comment 23

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/8725/#review7389

Ship It!

Comment 24

[Nils Ohlmeier [:drno]]

Comment on attachment 8594532 [details]
MozReview Request: bz://1155965/bwc

https://reviewboard.mozilla.org/r/7255/#review7391

Ship It!

Comment 25

[Byron Campen [:bwc]]

Check back on try to see if intermittents clear up.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e82bae379ca6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b57fef90c5cc

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e82bae379ca6
https://hg.mozilla.org/mozilla-central/rev/b57fef90c5cc

Comment 29

[Eric Rescorla (:ekr)]

Attachment: MozReview Request: Test for bug 1155965

Comment 31

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1155965 - Part 2: Verify that CreateOffer/Answer has been called when SetLocal is called.

Comment 32

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1155965 - Part 1: Test-cases.

Comment 33

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

Byron - does this need uplift to 40, or should it be WONTFIX?

Comment 34

[Byron Campen [:bwc]]

Comment on attachment 8620093 [details]
MozReview Request: Bug 1155965 - Part 2: Verify that CreateOffer/Answer has been called when SetLocal is called.

I guess this has high annoyance potential, so an uplift would make sense.

Approval Request Comment
[Feature/regressing bug #]:

   sdparta

[User impact if declined]:

   Null pointer crash can be caused by content at will, so there is a high annoyance potential here.

[Describe test coverage new/current, TreeHerder]:

   A unit-test was added (see part 1).

[Risks and why]: 

   Extremely low, we just added some checking.

[String/UUID change made/needed]:

   None.

Comment 35

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8620093 [details]
MozReview Request: Bug 1155965 - Part 2: Verify that CreateOffer/Answer has been called when SetLocal is called.

bwc tells me that although this crash does not currently have a high incidence rate there is a real potential for abuse. Given that and that the patch is pretty safe, we'll take this in beta3. Beta+

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fd3b313c083b
https://hg.mozilla.org/releases/mozilla-beta/rev/1332e4b5d0e1

Comment 37

[Martijn Wargers (dead)]

I'm still crashing with the testcase attached to this bug, I filed bug 1182098 for it.