876878 - crash in webrtc::videocapturemodule::GetMaxOfFrameArray

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Jason Smith [:jsmith]]

This bug was filed from the Socorro interface and is 
report bp-0a4f8b73-e624-48f2-bbf5-dc62d2130527 .
============================================================= 

Frame 	Module 	Signature 	Source
0 	xul.dll 	webrtc::videocapturemodule::GetMaxOfFrameArray 	media/webrtc/trunk/webrtc/modules/video_capture/windows/help_functions_ds.cc:26
1 	xul.dll 	webrtc::videocapturemodule::DeviceInfoDS::CreateCapabilityMap 	media/webrtc/trunk/webrtc/modules/video_capture/windows/device_info_ds.cc:609
2 	xul.dll 	webrtc::videocapturemodule::DeviceInfoImpl::NumberOfCapabilities 	media/webrtc/trunk/webrtc/modules/video_capture/device_info_impl.cc:76
3 	xul.dll 	webrtc::ViECaptureImpl::NumberOfCapabilities 	media/webrtc/trunk/webrtc/video_engine/vie_capture_impl.cc:442
4 	xul.dll 	mozilla::MediaEngineWebRTCVideoSource::ChooseCapability 	content/media/webrtc/MediaEngineWebRTCVideo.cpp:157
5 	xul.dll 	mozilla::MediaEngineWebRTCVideoSource::Allocate 	content/media/webrtc/MediaEngineWebRTCVideo.cpp:224
6 	xul.dll 	mozilla::GetUserMediaRunnable::ProcessGetUserMedia 	dom/media/MediaManager.cpp:739
7 	xul.dll 	mozilla::GetUserMediaRunnable::Run 	dom/media/MediaManager.cpp:600
8 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
9 	xul.dll 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:265
10 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:395
11 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:90
12 	msvcr100.dll 	_callthreadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:314
13 	msvcr100.dll 	_threadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:292
14 	kernel32.dll 	BaseThreadInitThunk 	
15 	ntdll.dll 	__RtlUserThreadStart 	
16 	ntdll.dll 	_RtlUserThreadStart

Comment 1

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

Looks like a bug in GetFrameRateList() in the directx/camera-driver....  it would help a lot to know what camera/driver the reports were for.

Bas, any ideas?

Comment 2

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

Attachment: null out pointer for GetFrameRateList

Comment 3

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

Comment on attachment 755485 [details] [diff] [review]
null out pointer for GetFrameRateList

Worth a try...

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 755485 [details] [diff] [review]
null out pointer for GetFrameRateList

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

This looks good for me but I'm afraid I'm not really qualified to review this code as I'm not a WebRTC peer.

Comment 5

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

Comment on attachment 755485 [details] [diff] [review]
null out pointer for GetFrameRateList

As module owner, I'll interpret that as r+ ;-)

Comment 6

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

https://hg.mozilla.org/projects/cypress/rev/758527363f29

We should watch crash stats once this gets to the field, and also investigate what broken cameras/drivers might be at fault.

Comment 7

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

Comment on attachment 755485 [details] [diff] [review]
null out pointer for GetFrameRateList

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Patch might not get a chance to fix/wallpaper over camera driver bug

Testing completed (on m-c, etc.): Still waiting for m-c merge - trivial fix


Risk to taking this patch (and alternatives if risky): No risk to the patch.  Just initializing a pointer to NULL in case the broken driver is leaving it unset.

String or IDL/UUID changes made by this patch: none

Comment 8

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/758527363f29

Comment 9

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

https://hg.mozilla.org/releases/mozilla-beta/rev/f7ddec3461a7
Aurora is waiting on closure

Comment 10

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/34fdc5b5710d

Comment 11

[Manuela Muntean [Away]]

Considering the time the patch was submitted, I'll verify the fix on Firefox 22 beta 5. (I'll check in Socorro if until then new crash reports will appear)

Comment 12

[Manuela Muntean [Away]]

While checking in Socorro for crash reports from last month, I found these 4 reports, which seem to be related to Firefox 22 beta 4 build (they have 20130605070403 as build ID). They only appear under the "Reports" tab in Socorro, and not also under the "Signature Summary" tab (Fx 22 beta 4 isn't mentioned).

https://crash-stats.mozilla.com/report/index/01ce5409-f812-4938-8d8a-1d09d2130613

https://crash-stats.mozilla.com/report/index/7426a742-4d0a-45b0-b176-5ea472130613

https://crash-stats.mozilla.com/report/index/62634ceb-b37c-4ec5-b48c-25e592130613


https://crash-stats.mozilla.com/report/index/5bb257a5-6f27-401a-abbf-5c57b2130613


Does anyone have any suggestions?

Comment 13

[Jason Smith [:jsmith]]

Suggestions specifically on what?

Comment 14

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

This would indicate that the problem was indeed caused by a bug in the driver.  All signs still point to a bug in the camera driver returning success without setting the pointer to a value.  This also proves that this was a potential security bug (though limited to machines with the bad camera driver - and webrtc isn't going to work on those cameras, so it may be hard to engineer users into enabling their cameras, since everytime they try for any site it will crash (in 22 beta-3 and before, and 23/24 builds up to when we landed the ptr-nulling patch).

The followon patch needed will be to check for a null array returned, and cleanly fail instead of null-derefing.

Needinfo to dveditz to decide if this makes sense to keep closed.

Comment 15

[Manuela Muntean [Away]]

(In reply to Jason Smith [:jsmith] from comment #13)
> Suggestions specifically on what?

I was wondering about the strange behavior that I experienced in comment 12. It seems weird to me. Is it expected?

Comment 16

[Manuela Muntean [Away]]

7 new crashes appeared with Firefox 22 beta 5: https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=webrtc%3A%3Avideocapturemodule%3A%3AGetMaxOfFrameArray%28__int64%2A%2C%20long%29&reason_type=contains&date=06%2F18%2F2013%2006%3A42%3A25&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=webrtc%3A%3Avideocapturemodule%3A%3AGetMaxOfFrameArray%28__int64%2A%2C%20long%29

Comment 17

[Jason Smith [:jsmith]]

By far the worst crash on the list for WebRTC-related crashes.

Comment 18

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

Note that all the crashes in B5 and later are null derefs (i.e. the safety patch works).

We need to land an "error and don't crash patch".  Also, we need to find out what camera and what driver is causing this failure.  This is a bug in some camera driver on windows.

Comment 19

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

Grr.  Thanks keyboard.  I was going to say, clearing needinfo to dveditz since we should keep this closed, as it applies to FF20 and FF21 (hard to test, but we see the crashes).

Comment 20

[Daniel Veditz [:dveditz]]

I'll call this sec-moderate rather than worse mostly because of the mitigations mentioned in comment 14

Comment 21

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

Attachment: Avoid null deref if camera doesn't update framelist ptr

Final 'don't crash with null deref if the driver does something stupid' patch

Comment 22

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

Comment on attachment 780534 [details] [diff] [review]
Avoid null deref if camera doesn't update framelist ptr

bas: you can review this one. :-) Just adds a null-check, and treats a broken driver as having failed the GetMaxOfFrameArray() call.  I'll propose this to the upstream codebase.

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/19d9842325b2

Will nominate for Aurora shortly.  Not sure we need to nominate it for beta at this late date.

Comment 24

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/19d9842325b2

Assuming this won't make it onto Fx23 since it's only sec-moderate.

Comment 25

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

Comment on attachment 780534 [details] [diff] [review]
Avoid null deref if camera doesn't update framelist ptr

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Crashes (null-deref) if you have a particular-but-unknown broken camera driver.

Testing completed (on m-c, etc.): on m-c.

Risk to taking this patch (and alternatives if risky): virtually none
 
String or IDL/UUID changes made by this patch: none

Comment 26

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c93b75b0efdb

Comment 27

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

getUserMedia wasn't enabled-by-default until FF 20