Closed Bug 1155965 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: WebRTC: Signaling, defect, critical)

All
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: martijn.martijn, Assigned: bwc)

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(4 files, 2 obsolete files)

Attached file 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
Regression from bug 1091242?
It seems that somehow you're calling setLocal without calling CreateOffer...
Attached file MozReview Request: bz://1155965/bwc (obsolete) —
/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/
Attached file MozReview Request: bz://1155965/ekr (obsolete) —
/r/7261 - Test for bug 1155965

Pull down this commit:

hg pull -r d684aa6d4fdef8f4cdbe19db61e51ddf7e9ada54 https://reviewboard-hg.mozilla.org/gecko/
I can verify that the signaling tests crash with my test without your patch but do not with your patch.
I will write a unit-test for this tomorrow.
(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
(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.
Sorry, I mean I attached one to this bug above and it validates that your patch removes the problem.
Assignee: nobody → docfaraday
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/
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.
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 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/
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.
Attachment #8594532 - Flags: review?(ekr)
Attachment #8594532 - Flags: review?(ekr) → review?(drno)
Comment on attachment 8594532 [details]
MozReview Request: bz://1155965/bwc

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

Ship It!
Attachment #8594532 - Flags: review?(drno) → review+
Has anyone checked that we properly reject the same thing for setRemoteDescription()?
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.
(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.
I can add that test.
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/
Attachment #8594532 - Flags: review+
Attachment #8594532 - Flags: review?(drno)
Part 1 is what needs review, I haven't changed part 2.
Comment on attachment 8594532 [details]
MozReview Request: bz://1155965/bwc

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

Ship It!
Attachment #8594532 - Flags: review?(drno) → review+
Check back on try to see if intermittents clear up.
Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e82bae379ca6
https://hg.mozilla.org/mozilla-central/rev/b57fef90c5cc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Attachment #8594536 - Attachment is obsolete: true
Attachment #8594532 - Attachment is obsolete: true
Attachment #8620093 - Flags: review+
Attachment #8620094 - Flags: review+
Byron - does this need uplift to 40, or should it be WONTFIX?
Flags: needinfo?(docfaraday)
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.
Flags: needinfo?(docfaraday)
Attachment #8620093 - Flags: approval-mozilla-beta?
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+
Attachment #8620093 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8620094 - Flags: approval-mozilla-beta+
I'm still crashing with the testcase attached to this bug, I filed bug 1182098 for it.
You need to log in before you can comment on or make changes to this bug.