813245 - crash in nsPluginHost::FindPlugins

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Scoobidiver (away)]

It's #46 top browser crasher in 17.0b6.

Stack traces are various:
Frame 	Module 	Signature 	Source
0 	mozalloc.dll 	mozalloc_abort 	memory/mozalloc/mozalloc_abort.cpp:23
1 	mozalloc.dll 	mozalloc_handle_oom 	memory/mozalloc/mozalloc_oom.cpp:27
2 	mozalloc.dll 	moz_xmalloc 	memory/mozalloc/mozalloc.cpp:59
3 	xul.dll 	nsPluginHost::FindPlugins 	dom/plugins/base/nsPluginHost.cpp:2378
4 	xul.dll 	nsPluginHost::LoadPlugins 	dom/plugins/base/nsPluginHost.cpp:2344
5 	xul.dll 	nsPluginHost::GetPluginCount 	dom/plugins/base/nsPluginHost.cpp:1497
6 	xul.dll 	nsPluginArray::GetLength 	dom/base/nsPluginArray.cpp:61
7 	xul.dll 	nsPluginArray::GetPlugins 	dom/base/nsPluginArray.cpp:221
8 	xul.dll 	nsPluginArray::GetNamedItem 	dom/base/nsPluginArray.cpp:115
9 	xul.dll 	nsPluginArraySH::GetNamedItem 	dom/base/nsDOMClassInfo.cpp:10046
...

Frame 	Module 	Signature 	Source
0 	mozalloc.dll 	mozalloc_abort 	memory/mozalloc/mozalloc_abort.cpp:79
1 	mozalloc.dll 	mozalloc_handle_oom 	memory/mozalloc/mozalloc_oom.cpp:60
2 	mozalloc.dll 	moz_xmalloc 	
3 	xul.dll 	nsPluginHost::FindPlugins 	dom/plugins/base/nsPluginHost.cpp:2341
4 	xul.dll 	nsPluginHost::LoadPlugins 	dom/plugins/base/nsPluginHost.cpp:2307
5 	xul.dll 	nsPluginHost::GetPluginCount 	dom/plugins/base/nsPluginHost.cpp:1537
6 	xul.dll 	nsPluginArray::GetLength 	dom/base/nsPluginArray.cpp:93
7 	xul.dll 	nsMimeTypeArray::GetMimeTypes 	dom/base/nsMimeTypeArray.cpp:245
8 	xul.dll 	mozilla::dom::Navigator::RefreshMIMEArray 	dom/base/Navigator.cpp:560
9 	xul.dll 	mozilla::dom::Navigator::JavaEnabled 	dom/base/Navigator.cpp:523
10 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
...

Frame 	Module 	Signature 	Source
0 	mozalloc.dll 	mozalloc_abort 	memory/mozalloc/mozalloc_abort.cpp:23
1 	mozalloc.dll 	mozalloc_handle_oom 	memory/mozalloc/mozalloc_oom.cpp:27
2 	mozalloc.dll 	moz_xmalloc 	memory/mozalloc/mozalloc.cpp:59
3 	xul.dll 	nsPluginHost::FindPlugins 	dom/plugins/base/nsPluginHost.cpp:2378
4 	xul.dll 	nsPluginHost::LoadPlugins 	dom/plugins/base/nsPluginHost.cpp:2344
5 	xul.dll 	nsPluginHost::FindPluginEnabledForExtension 	dom/plugins/base/nsPluginHost.cpp:1621
6 	xul.dll 	nsPluginHost::IsPluginEnabledForExtension 	dom/plugins/base/nsPluginHost.cpp:1369
7 	xul.dll 	nsExternalHelperAppService::GetTypeFromExtension 	uriloader/exthandler/nsExternalHelperAppService.cpp:2594
8 	xul.dll 	nsExternalHelperAppService::GetTypeFromFile 	uriloader/exthandler/nsExternalHelperAppService.cpp:2718
9 	xul.dll 	nsFileChannel::MakeFileInputStream 	netwerk/protocol/file/nsFileChannel.cpp:298
10 	xul.dll 	nsFileChannel::OpenContentStream 	netwerk/protocol/file/nsFileChannel.cpp:367
...

Frame 	Module 	Signature 	Source
0 	mozalloc.dll 	mozalloc_abort 	memory/mozalloc/mozalloc_abort.cpp:23
1 	mozalloc.dll 	mozalloc_handle_oom 	memory/mozalloc/mozalloc_oom.cpp:27
2 	mozalloc.dll 	moz_xmalloc 	memory/mozalloc/mozalloc.cpp:59
3 	xul.dll 	nsPluginHost::FindPlugins 	dom/plugins/base/nsPluginHost.cpp:2378
4 	xul.dll 	nsPluginHost::LoadPlugins 	dom/plugins/base/nsPluginHost.cpp:2344
5 	xul.dll 	nsPluginHost::GetPluginTags 	dom/plugins/base/nsPluginHost.cpp:1534
6 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
7 	xul.dll 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2406
8 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1478
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozalloc_abort%28char+const*+const%29+|+mozalloc_handle_oom%28unsigned+int%29+|+moz_xmalloc+|+nsPluginHost%3A%3AFindPlugins%28bool%2C+bool*%29

Comment 1

[Benjamin Smedberg]

https://crash-stats.mozilla.com/report/index/a69c84d6-f115-42e6-9d68-7e4612121119
OOMAllocationSize	4294967295

== 0xFFFFFFFF underflow.

This is similar or identical to bug 786233.

Comment 2

[cristian.cornea]

Workaround: 

Delete/rename the file %APPDATA%\Mozilla\Firefox\Profiles\%PID%.default\pluginreg.dat

Comment 3

[cristian.cornea]

Attachment: The plugin registry file that crashes firefox at startup

Comment 4

[Georg Fritzsche [:gfritzsche]]

Thanks Cristian, i can reproduce the crash with that. 

We try allocate an array of size |0x7fffffff * 3| here:
http://hg.mozilla.org/mozilla-central/annotate/dd277d439d31/dom/plugins/base/nsPluginHost.cpp#l2808
... from |atoi("1347734823984|0|5")| here:
http://hg.mozilla.org/mozilla-central/annotate/dd277d439d31/dom/plugins/base/nsPluginHost.cpp#l2800

We could do a sanity bounding here for safety, but the interesting question is how those corrupt values for the mimetype count got in there.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Reading through |nsPluginHost::WritePluginInfo()|, this looks definitely broken. For the npgtpo3dautoplugin the whole block with description, name and mimetype-count is missing that should have been written here:
http://hg.mozilla.org/mozilla-central/annotate/dd277d439d31/dom/plugins/base/nsPluginHost.cpp#l2538

Comment 6

[Benjamin Smedberg]

Maybe there was a crash. Maybe the computer shut off. We should definitely be sanity-checking the numbers on input regardless of whether there's a bug we can fix at output time. It's a cache, so we should be safe to just throw the whole thing away if we detect anything awry.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Sanity check for parsed mimetype count

Straight-forward sanity check on the parsed mimetype count.

Do we have any test-environment that could cover this?

Comment 8

[Benjamin Smedberg]

Good question. You might be able to do this in an xpcshell test by writing the "bad" pluginreg.dat before you "start" a profile.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 693899 [details] [diff] [review]
Sanity check for parsed mimetype count

Ah, thanks - turns out we already do write some plugin registries in dom/plugins/test/unit.

Holding off the review request until i added the test.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Sanity check for parsed mimetype count

Added test.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=d0c94879d370

control try run to see that this triggers properly (only the xpcshell test):
https://tbpl.mozilla.org/?tree=Try&rev=decae962dd56

Comment 11

[Georg Fritzsche [:gfritzsche]]

Attachment: Sanity check for parsed mimetype count, v4

Fix Linux build bustage. Also updated to tip and killed a redundant JS function.
https://tbpl.mozilla.org/?tree=Try&rev=763f9efc76cd

Comment 12

[Georg Fritzsche [:gfritzsche]]

The Windows debug builds for the control try run are timing out. 
Looks like this is the runtime throwing up an error message box (for the invalid allocation), which is pretty unhelpful in automated runs.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Windows debug builds hang with the call stack below for the unit test, never even showing a message box. This is apparently specific to xpcshell-tests, as the MsgBox shows up for the test-registry with Fx. 
Benjamin or Jim, do you see some easy solution for this? 

user32.dll!_NtUserWaitMessage@0()  + 0x15 bytes	
user32.dll!_NtUserWaitMessage@0()  + 0x15 bytes	
xul.dll!nsAppShell::ProcessNextNativeEvent(bool mayWait)  Line 335	C++
xul.dll!nsBaseAppShell::DoProcessNextNativeEvent(bool mayWait, unsigned int recursionDepth)  Line 139 + 0x12 bytes	C++
xul.dll!nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal * thr, bool mayWait, unsigned int recursionDepth)  Line 298 + 0x14 bytes	C++
xul.dll!nsThread::ProcessNextEvent(bool mayWait, bool * result)  Line 603	C++
xul.dll!NS_ProcessNextEvent_P(nsIThread * thread, bool mayWait)  Line 237 + 0x17 bytes	C++
xul.dll!nsThread::Shutdown()  Line 474 + 0xb bytes	C++
xul.dll!mozilla::crashreporter::LSPAnnotationGatherer::Annotate()  Line 47	C++
xul.dll!nsRunnableMethodImpl<void (__thiscall mozilla::crashreporter::LSPAnnotationGatherer::*)(void),1>::Run()  Line 368	C++
xul.dll!nsThread::ProcessNextEvent(bool mayWait, bool * result)  Line 627 + 0x19 bytes	C++
xul.dll!NS_ProcessPendingEvents_P(nsIThread * thread, unsigned int timeout)  Line 187 + 0x14 bytes	C++
xul.dll!nsBaseAppShell::NativeEventCallback()  Line 98	C++
xul.dll!nsAppShell::EventWindowProc(HWND__ * hwnd, unsigned int uMsg, unsigned int wParam, long lParam)  Line 94	C++
user32.dll!_InternalCallWinProc@20()  + 0x23 bytes	
user32.dll!_UserCallWinProcCheckWow@32()  + 0xb7 bytes	
user32.dll!_DispatchMessageWorker@8()  + 0xed bytes	
user32.dll!_DispatchMessageW@4()  + 0xf bytes	
user32.dll!_DialogBox2@16()  + 0x1c0 bytes	
user32.dll!_InternalDialogBox@24()  + 0xc9 bytes	
user32.dll!_SoftModalMessageBox@4()  + 0x757 bytes	
user32.dll!_MessageBoxWorker@4()  + 0x269 bytes	
user32.dll!_MessageBoxTimeoutW@24()  + 0x52 bytes	
user32.dll!_MessageBoxTimeoutA@24()  + 0x76 bytes	
user32.dll!_MessageBoxExA@20()  + 0x1b bytes	
user32.dll!_MessageBoxA@16()  + 0x18 bytes	
msvcr100d.dll!___crtMessageBoxA()  + 0x20c bytes	
msvcr100d.dll!___crtMessageWindowA()  + 0x3b7 bytes	
msvcr100d.dll!__VCrtDbgReportA()  + 0x7d8 bytes	
msvcr100d.dll!__CrtDbgReportV()  + 0x22 bytes	
msvcr100d.dll!__CrtDbgReport()  + 0x2b bytes	
msvcr100d.dll!__nh_malloc_dbg()  + 0x265 bytes	
msvcr100d.dll!__nh_malloc_dbg()  + 0x7f bytes	
msvcr100d.dll!__nh_malloc_dbg()  + 0x2c bytes	
msvcr100d.dll!_malloc()  + 0x1b bytes	
mozalloc.dll!moz_xmalloc(unsigned int size)  Line 54 + 0xa bytes	C++
xul.dll!operator new[](unsigned int size)  Line 212 + 0xa bytes	C++
xul.dll!nsPluginHost::ReadPluginInfo()  Line 2821 + 0x1f bytes	C++
[...]

Comment 14

[(not currently active) Ted Mielczarek]

FYI bug 587982 got rid of these dialogs in the browser. We probably just need to use that same code in xpcshell. (The hang in the nested event loop is just a horrible side effect of that.)

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> FYI bug 587982 got rid of these dialogs in the browser. We probably just
> need to use that same code in xpcshell. (The hang in the nested event loop
> is just a horrible side effect of that.)

Ah, thanks Ted, will check that out.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Doing a straight-forward adoption i get a hang which is apparently waiting on the CrashReporter thread, which hangs in allocation related code (hanging on heap unlock in the CRT), stack below.

We receive allocation failure crash reports for Fx, so this must be another setup issue? Ted, any ideas?

ntdll.dll!_NtWaitForSingleObject@12()  + 0x15 bytes	
ntdll.dll!_NtWaitForSingleObject@12()  + 0x15 bytes	
browsercomps.dll!_heap_alloc_dbg_impl(unsigned int nSize, int nBlockUse, const char * szFileName, int nLine, int * errno_tmp)  Line 507 + 0x7 bytes	C++
ntdll.dll!_RtlInitUnicodeStringEx@8() 	
msvcr100d.dll!__lock()  + 0x3d bytes	
msvcr100d.dll!__free_dbg()  + 0x39 bytes	
msvcr100d.dll!_free()  + 0x10 bytes	
mozalloc.dll!moz_free(void * ptr)  Line 48 + 0xa bytes	C++
xul.dll!operator delete[](void * ptr)  Line 236 + 0xa bytes	C++
xul.dll!nsAutoArrayPtr<wchar_t>::~nsAutoArrayPtr<wchar_t>()  Line 475 + 0x11 bytes	C++
xul.dll!`anonymous namespace'::patched_LdrLoadDll(wchar_t * filePath, unsigned long * flags, _UNICODE_STRING * moduleFileName, void * * handle)  Line 453 + 0x27 bytes	C++
KernelBase.dll!74d92c95() 	
[Frames below may be incorrect and/or missing, no symbols loaded for KernelBase.dll]	
Nvd3d9wrap.dll!74642654() 	
dbghelp.dll!Win32LiveSystemProvider::Initialize()  + 0x10 bytes	
dbghelp.dll!NtWin32LiveSystemProvider::Initialize()  + 0xa bytes	
dbghelp.dll!_MiniDumpCreateLiveSystemProvider@8()  + 0xb3 bytes	
dbghelp.dll!_MiniDumpWriteDump@28()  + 0x38 bytes	
xul.dll!google_breakpad::ExceptionHandler::WriteMinidumpWithExceptionForProcess(unsigned long requesting_thread_id, _EXCEPTION_POINTERS * exinfo, MDRawAssertionInfo * assertion, void * process, bool write_requester_stream)  Line 990 + 0x43 bytes	C++
xul.dll!google_breakpad::ExceptionHandler::WriteMinidumpWithException(unsigned long requesting_thread_id, _EXCEPTION_POINTERS * exinfo, MDRawAssertionInfo * assertion)  Line 819 + 0x1d bytes	C++
xul.dll!google_breakpad::ExceptionHandler::ExceptionHandlerThreadMain(void * lpParameter)  Line 374 + 0x26 bytes	C++

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 694879 [details] [diff] [review]
Sanity check for parsed mimetype count, v4

It looks like adressing the timeouts might take a little bit longer, but given that it's not an issue with this patch applied i'd like to get this landed.
I'd address the xpcshell timeouts in a separate bug as they are independent of the issue here anyway.

Comment 18

[Benjamin Smedberg]

Comment on attachment 694879 [details] [diff] [review]
Sanity check for parsed mimetype count, v4

I don't think 'return rv' is correct here, because rv will probably be a success code. You really want to return a failure code here, I think, so think should be return NS_ERROR_FAILURE;

r=me with that change

Comment 19

[Georg Fritzsche [:gfritzsche]]

Attachment: Sanity check for parsed mimetype count, v5

Fixed the return statement, thanks for catching that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef505122f02a

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/ef505122f02a

Comment 21

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 703983 [details] [diff] [review]
Sanity check for parsed mimetype count, v5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Start-up crashes with certain broken plugin registries.
Testing completed (on m-c, etc.): Fine on try, landed on m-c.
Risk to taking this patch (and alternatives if risky): Low-risk, it just guards against over-allocating.
String or UUID changes made by this patch: None.

Comment 22

[Alex Keybl [:akeybl]]

Comment on attachment 703983 [details] [diff] [review]
Sanity check for parsed mimetype count, v5

Not a tracked top crasher, so let's take this plugin code change on Aurora 20 and let it ride the trains.

Comment 23

[Georg Fritzsche [:gfritzsche]]

I stumbled over a slightly different crash signature:
mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | nsPluginHost::ReadPluginInfo()
https://crash-stats.mozilla.com/report/list?signature=mozalloc_abort%28char+const*+const%29+|+mozalloc_handle_oom%28unsigned+int%29+|+moz_xmalloc+|+nsPluginHost%3A%3AReadPluginInfo%28%29

This bug doesn't show up there - would i have to edit the crash signature field on this bug to get it to?

Comment 24

[Scoobidiver (away)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #23) 
> would i have to edit the crash signature field on this bug to get it to?
Done.

Comment 25

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4c3bea6e07ab

Comment 26

[Mihai Morar, (:MihaiMorar)]

There are no crashes in crash-stats for FF 20.b1, Aurora 21 or Nightly 22 for the signatures mentioned in this Bug, so I guess Georg fix is verified. Scoobi can you please confirm this?

Comment 27

[Scoobidiver (away)]

There are a few crashes in 19.0 and below but not crashes in 20.0 and above.

Comment 28

[Mihai Morar, (:MihaiMorar)]

(In reply to Scoobidiver from comment #27)
> There are a few crashes in 19.0 and below but not crashes in 20.0 and above.

Thanks for confirm!