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
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 on attachment 755485 [details] [diff] [review] null out pointer for GetFrameRateList Worth a try...
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 on attachment 755485 [details] [diff] [review] null out pointer for GetFrameRateList As module owner, I'll interpret that as r+ ;-)
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 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
https://hg.mozilla.org/releases/mozilla-beta/rev/f7ddec3461a7 Aurora is waiting on closure
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)
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?
Suggestions specifically on what?
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.
(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?
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
By far the worst crash on the list for WebRTC-related crashes.
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.
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).
I'll call this sec-moderate rather than worse mostly because of the mitigations mentioned in comment 14
Created attachment 780534 [details] [diff] [review] 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 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.
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.
https://hg.mozilla.org/mozilla-central/rev/19d9842325b2 Assuming this won't make it onto Fx23 since it's only sec-moderate.
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
getUserMedia wasn't enabled-by-default until FF 20