445229 - NPAPI/NPruntime possible crash when returning a new NPObject as the result of an Invoke/InvokeDefault/GetProperty.

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Antoine Labour]

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) (inactive)]

Good catch! This seems like it's pretty easy to patch.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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)]

Comment on attachment 329679 [details] [diff] [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) (inactive)]

Attachment: With jst's suggestion

Comment 5

[Antoine Labour]

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)]

This is a trivial and safe obvious crash fix that we should include in dot release etc.

Comment 7

[Daniel Veditz [:dveditz]]

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)]

Anyway to write an automated test for this one?

Comment 9

[Johnny Stenback (:jst)]

Unfortunately not at this point.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

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)]

Comment on attachment 329803 [details] [diff] [review]
With jst's suggestion

Blake, does this patch apply to both 1.8 and 1.9?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 329803 [details] [diff] [review]
With jst's suggestion

This applies directly to the 1.9 branch.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

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)]

Comment on attachment 329803 [details] [diff] [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) (inactive)]

Fix checked into the 1.9 branch.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 329803 [details] [diff] [review]
With jst's suggestion

This applies as-is to the 1.8 branch.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 329803 [details] [diff] [review]
With jst's suggestion

Approved for 1.8.1.17, a=dveditz for release-drivers.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branch.

Comment 19

[Alexander Sack]

Comment on attachment 329803 [details] [diff] [review]
With jst's suggestion

a=asac for 1.8.0.15

Comment 20

[Al Billings [:abillings - ex-MoCo]]

This is difficult to verify for 1.8.1.17. Any suggestions?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

(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.