If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

crash in webrtc::videocapturemodule::GetMaxOfFrameArray

RESOLVED FIXED in Firefox 24

Status

()

Core
WebRTC
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jsmith, Assigned: jesup)

Tracking

({crash, sec-moderate, sec-vector})

22 Branch
mozilla25
x86
Windows NT
crash, sec-moderate, sec-vector
Points:
---

Firefox Tracking Flags

(firefox21 wontfix, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, firefox25 fixed, firefox-esr17 unaffected, firefox-esr24 unaffected, b2g18 unaffected)

Details

(Whiteboard: [getUserMedia], [blocking-gum-][leave-open][webrtc-topcrash][adv-main24+], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
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?
(Assignee)

Comment 2

4 years ago
Created attachment 755485 [details] [diff] [review]
null out pointer for GetFrameRateList
(Assignee)

Comment 3

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

Worth a try...
Attachment #755485 - Flags: review?(bas)
(Assignee)

Updated

4 years ago
Whiteboard: [getUserMedia], [blocking-gum-][webrtc-uplift]
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.
Attachment #755485 - Flags: review?(bas)
(Assignee)

Comment 5

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

As module owner, I'll interpret that as r+ ;-)
Attachment #755485 - Flags: review+
(Assignee)

Comment 6

4 years ago
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.
Whiteboard: [getUserMedia], [blocking-gum-][webrtc-uplift] → [getUserMedia], [blocking-gum-][webrtc-uplift][leave-open]
(Assignee)

Comment 7

4 years ago
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
Attachment #755485 - Flags: approval-mozilla-beta?
Attachment #755485 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/758527363f29

Updated

4 years ago
Assignee: nobody → rjesup

Updated

4 years ago
Attachment #755485 - Flags: approval-mozilla-beta?
Attachment #755485 - Flags: approval-mozilla-beta+
Attachment #755485 - Flags: approval-mozilla-aurora?
Attachment #755485 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/f7ddec3461a7
Aurora is waiting on closure
Target Milestone: --- → mozilla24
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/34fdc5b5710d
Whiteboard: [getUserMedia], [blocking-gum-][webrtc-uplift][leave-open] → [getUserMedia], [blocking-gum-][leave-open]
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)

Updated

4 years ago
Flags: needinfo?

Updated

4 years ago
Flags: needinfo?
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?
Flags: needinfo?
(Reporter)

Comment 13

4 years ago
Suggestions specifically on what?
Flags: needinfo?
(Assignee)

Comment 14

4 years ago
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.
Group: core-security
Flags: needinfo?(dveditz)
(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

Updated

4 years ago
tracking-firefox22: --- → ?
(Reporter)

Comment 17

4 years ago
By far the worst crash on the list for WebRTC-related crashes.
Whiteboard: [getUserMedia], [blocking-gum-][leave-open] → [getUserMedia], [blocking-gum-][leave-open][webrtc-topcrash]
(Assignee)

Comment 18

4 years ago
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.
Flags: needinfo?(dveditz)
(Assignee)

Comment 19

4 years ago
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).
status-firefox21: --- → affected
I'll call this sec-moderate rather than worse mostly because of the mitigations mentioned in comment 14
Keywords: sec-moderate, sec-vector

Updated

4 years ago
tracking-firefox22: ? → ---
(Assignee)

Comment 21

4 years ago
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
(Assignee)

Updated

4 years ago
Attachment #755485 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
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.
Attachment #780534 - Flags: review?(bas)
Attachment #780534 - Flags: review?(bas) → review+
(Assignee)

Comment 23

4 years ago
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.
(Assignee)

Updated

4 years ago
Target Milestone: mozilla24 → mozilla25
https://hg.mozilla.org/mozilla-central/rev/19d9842325b2

Assuming this won't make it onto Fx23 since it's only sec-moderate.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox21: affected → wontfix
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → affected
status-firefox25: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 25

4 years ago
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
Attachment #780534 - Flags: approval-mozilla-aurora?

Updated

4 years ago
Attachment #780534 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/c93b75b0efdb
status-firefox24: affected → fixed
Whiteboard: [getUserMedia], [blocking-gum-][leave-open][webrtc-topcrash] → [getUserMedia], [blocking-gum-][leave-open][webrtc-topcrash][adv-main24+]
status-firefox-esr17: --- → wontfix
status-firefox-esr24: --- → unaffected
(Assignee)

Comment 27

4 years ago
getUserMedia wasn't enabled-by-default until FF 20
status-firefox-esr17: wontfix → unaffected
status-b2g18: --- → unaffected
Group: core-security
You need to log in before you can comment on or make changes to this bug.