Last Comment Bug 445229 - NPAPI/NPruntime possible crash when returning a new NPObject as the result of an Invoke/InvokeDefault/GetProperty.
: NPAPI/NPruntime possible crash when returning a new NPObject as the result of...
Status: RESOLVED FIXED
: [sg:high?]
: crash, fixed1.8.1.17, fixed1.9.0.2
Product: Core
Classification: Components
Component: Plug-ins
: unspecified
: All All
: -- critical with 1 vote (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: plugins
:
:
:
  Show dependency treegraph
 
Reported: 2008-07-14 15:01 PDT by Antoine Labour
Modified: 2008-09-23 21:28 PDT (History)
12 users (show)
samuel.sidler+old: blocking1.9.0.2+
jst: wanted1.9.0.x+
dveditz: blocking1.8.1.17+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
See Also:
Crash Signature:


Attachments
Easiest fix (905 bytes, patch)
2008-07-15 08:47 PDT, Blake Kaplan (:mrbkap)
jst: review+
jst: superreview+
Details | Diff | Splinter Review
With jst's suggestion (1.08 KB, patch)
2008-07-16 03:17 PDT, Blake Kaplan (:mrbkap)
mrbkap: review+
mrbkap: superreview+
dveditz: approval1.8.1.17+
samuel.sidler+old: approval1.9.0.2+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Summon comment box

Description Antoine Labour 2008-07-14 15:01:23 PDT
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
Comment 1 Blake Kaplan (:mrbkap) 2008-07-15 08:45:12 PDT
Good catch! This seems like it's pretty easy to patch.
Comment 2 Blake Kaplan (:mrbkap) 2008-07-15 08:47:56 PDT
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 3 Johnny Stenback (:jst, jst@mozilla.com) 2008-07-15 13:01:14 PDT
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.
Comment 4 Blake Kaplan (:mrbkap) 2008-07-16 03:17:17 PDT
Created attachment 329803 [details] [review]
With jst's suggestion
Comment 5 Antoine Labour 2008-07-16 15:52:42 PDT
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.
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2008-07-16 17:25:17 PDT
This is a trivial and safe obvious crash fix that we should include in dot release etc.
Comment 7 Daniel Veditz 2008-07-18 11:27:45 PDT
Flagging as a potential security problem to make it more likely downstreams don't pass this one up.
Comment 8 Samuel Sidler (old account; do not CC) 2008-07-21 17:46:23 PDT
Anyway to write an automated test for this one?
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2008-07-21 17:57:38 PDT
Unfortunately not at this point.
Comment 10 Blake Kaplan (:mrbkap) 2008-07-23 09:52:01 PDT
Pushed as http://hg.mozilla.org/index.cgi/mozilla-central/rev/20bff6157770

Thanks for the great analysis, Antoine!
Comment 11 Samuel Sidler (old account; do not CC) 2008-08-03 14:25:13 PDT
Comment on attachment 329803 [details] [review]
With jst's suggestion

Blake, does this patch apply to both 1.8 and 1.9?
Comment 12 Blake Kaplan (:mrbkap) 2008-08-04 18:14:03 PDT
Comment on attachment 329803 [details] [review]
With jst's suggestion

This applies directly to the 1.9 branch.
Comment 13 Blake Kaplan (:mrbkap) 2008-08-04 18:42:18 PDT
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 14 Samuel Sidler (old account; do not CC) 2008-08-04 18:44:27 PDT
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?)
Comment 15 Blake Kaplan (:mrbkap) 2008-08-11 11:10:28 PDT
Fix checked into the 1.9 branch.
Comment 16 Blake Kaplan (:mrbkap) 2008-08-11 11:32:29 PDT
Comment on attachment 329803 [details] [review]
With jst's suggestion

This applies as-is to the 1.8 branch.
Comment 17 Daniel Veditz 2008-08-11 11:46:33 PDT
Comment on attachment 329803 [details] [review]
With jst's suggestion

Approved for 1.8.1.17, a=dveditz for release-drivers.
Comment 18 Blake Kaplan (:mrbkap) 2008-08-11 14:36:16 PDT
Fix checked into the 1.8 branch.
Comment 19 Alexander Sack 2008-08-31 06:06:04 PDT
Comment on attachment 329803 [details] [review]
With jst's suggestion

a=asac for 1.8.0.15
Comment 20 Al Billings [:abillings] 2008-09-02 16:46:07 PDT
This is difficult to verify for 1.8.1.17. Any suggestions?
Comment 21 Blake Kaplan (:mrbkap) 2008-09-02 16:52:39 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.