Bugzilla@Mozilla – Bug 445229
NPAPI/NPruntime possible crash when returning a new NPObject as the result of an Invoke/InvokeDefault/GetProperty.
Last modified: 2008-09-23 21:28:14 PDT
Summon comment box
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15) Gecko/2008062306 Firefox/2.0.0.15 From modules/plugin/base/src/nsJSNPRuntime.cpp:1297, in the nsNPObjWrapper::GetNewOrUsed function: --------------------------------------------------------------------- NPObjWrapperHashEntry *entry = NS_STATIC_CAST(NPObjWrapperHashEntry *, PL_DHashTableOperate(&sNPObjWrappers, npobj, PL_DHASH_ADD)); if (!entry) { // Out of memory JS_ReportOutOfMemory(cx); return nsnull; } if (PL_DHASH_ENTRY_IS_BUSY(entry) && entry->mJSObj) { // Found a live NPObject wrapper, return it. return entry->mJSObj; } entry->mNPObj = npobj; entry->mNpp = npp; // No existing JSObject, create one. JSObject *obj = ::JS_NewObject(cx, &sNPObjectJSWrapperClass, nsnull, nsnull); if (!obj) { // OOM? Remove the stale entry from the hash. PL_DHashTableRawRemove(&sNPObjWrappers, entry); return nsnull; } OnWrapperCreated(); entry->mJSObj = obj; --------------------------------------------------------------------- The 'entry' variable is a pointer to an entry inside the data storage of the global 'sNPObjWrappers' hash table. The problem is that the ::JS_NewObject (line 1318) may trigger a JavaScript garbage collection, that can in turn finalize some NPObject, removing them from the 'sNPObjWrappers' hash table. That process can re-allocate the storage of the hash table, making the 'entry' point to released memory, crashing the process when accessed in the line 1330 (or worse, pointing to re-allocated memory, hence corrupting data). This bug was reproduced fairly reliably on Firefox 2.0.0.15. That code snippet looks similar in Firefox 3, suggesting the bug is probably still present (unless ::JS_NewObject can't trigger the JavaScript garbage collector). I don't have a repro test case sample plug-in available currently, but I can probably write one if necessary. It is not clear if specific allocation/garbage collection patterns make this bug easier to reproduce. Reproducible: Sometimes Steps to Reproduce: 1. Write a NPAPI plug-in that has a function that creates and returns a new NPObject every time it is called. 2. Call that function many times from JavaScript. Actual Results: Eventually Firefox will crash. Expected Results: No crash
Good catch! This seems like it's pretty easy to patch.
Created attachment 329679 [details] [review] Easiest fix We could avoid the duplicate lookup by trying to keep track of whether the JS_NewObject call caused any wrappers to get collected, but I don't think it's worth it.
Comment on attachment 329679 [details] [review] Easiest fix It'd be trivial to sample sNPObjWrappers.generation before and after the JS_NewObject() and only do the re-lookup if the table changed. r+sr=jst either way.
Created attachment 329803 [details] [review] With jst's suggestion
I tried this latest patch with the Firefox 2.0.0.15 codebase, and it appears to resolve the problem, no more crash. I put a breakpoint in that entry reload code, and it hit a few times, at roughly the same frequency Firefox would crash before, so I'm confident this is indeed fixing it.
This is a trivial and safe obvious crash fix that we should include in dot release etc.
Flagging as a potential security problem to make it more likely downstreams don't pass this one up.
Anyway to write an automated test for this one?
Unfortunately not at this point.
Pushed as http://hg.mozilla.org/index.cgi/mozilla-central/rev/20bff6157770 Thanks for the great analysis, Antoine!
Comment on attachment 329803 [details] [review] With jst's suggestion Blake, does this patch apply to both 1.8 and 1.9?
Comment on attachment 329803 [details] [review] With jst's suggestion This applies directly to the 1.9 branch.
1.9 drivers: It isn't possible to write an automated test for this bug because it depends heavily on GC timing (and we don't have a testsuite for plugin bugs yet).
Comment on attachment 329803 [details] [review] With jst's suggestion Approved for 1.9.0.2. Please land in CVS. a=ss (And can we get a 1.8 branch patch for this?)
Fix checked into the 1.9 branch.
Comment on attachment 329803 [details] [review] With jst's suggestion This applies as-is to the 1.8 branch.
Comment on attachment 329803 [details] [review] With jst's suggestion Approved for 1.8.1.17, a=dveditz for release-drivers.
Fix checked into the 1.8 branch.
Comment on attachment 329803 [details] [review] With jst's suggestion a=asac for 1.8.0.15
This is difficult to verify for 1.8.1.17. Any suggestions?
(In reply to comment #20) > This is difficult to verify for 1.8.1.17. Any suggestions? This is difficult to verify anywhere. Unless someone wants to do comment 5 style verification, I don't think it's worth it.