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

RESOLVED FIXED

Status

()

Core
Plug-ins
--
critical
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: Antoine Labour, Assigned: mrbkap)

Tracking

({crash, fixed1.8.1.17, fixed1.9.0.2})

unspecified
crash, fixed1.8.1.17, fixed1.9.0.2
Points:
---
Bug Flags:
blocking1.9.0.2 +
wanted1.9.0.x +
blocking1.8.1.17 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high?])

Attachments

(1 attachment, 1 obsolete attachment)

1.08 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Samuel Sidler (old account; do not CC)
: approval1.9.0.2+
Alexander Sack
: approval1.8.0.next+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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
Component: General → Plug-ins
Product: Firefox → Core
QA Contact: general → plugins
Version: unspecified → 1.9.0 Branch

Updated

10 years ago
Keywords: crash
(Assignee)

Comment 1

10 years ago
Good catch! This seems like it's pretty easy to patch.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

10 years ago
Created attachment 329679 [details] [diff] [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.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #329679 - Flags: superreview?(jst)
Attachment #329679 - Flags: review?(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.
Attachment #329679 - Flags: superreview?(jst)
Attachment #329679 - Flags: superreview+
Attachment #329679 - Flags: review?(jst)
Attachment #329679 - Flags: review+
(Assignee)

Comment 4

10 years ago
Created attachment 329803 [details] [diff] [review]
With jst's suggestion
Attachment #329679 - Attachment is obsolete: true
Attachment #329803 - Flags: superreview+
Attachment #329803 - Flags: review+
(Reporter)

Comment 5

10 years ago
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.
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.2?
Flags: blocking1.8.1.17?
Flagging as a potential security problem to make it more likely downstreams don't pass this one up.
Group: security
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.17?
Flags: blocking1.8.1.17+
Whiteboard: [sg:high?]
Version: 1.9.0 Branch → unspecified
Anyway to write an automated test for this one?
Flags: blocking1.9.0.2? → blocking1.9.0.2+
Unfortunately not at this point.
(Assignee)

Comment 10

10 years ago
Pushed as http://hg.mozilla.org/index.cgi/mozilla-central/rev/20bff6157770

Thanks for the great analysis, Antoine!
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Comment on attachment 329803 [details] [diff] [review]
With jst's suggestion

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

Comment 12

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

This applies directly to the 1.9 branch.
Attachment #329803 - Flags: approval1.9.0.2?
(Assignee)

Comment 13

10 years ago
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] [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?)
Attachment #329803 - Flags: approval1.9.0.2? → approval1.9.0.2+
(Assignee)

Comment 15

10 years ago
Fix checked into the 1.9 branch.
Keywords: fixed1.9.0.2
(Assignee)

Comment 16

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

This applies as-is to the 1.8 branch.
Attachment #329803 - Flags: approval1.8.1.17?
Comment on attachment 329803 [details] [diff] [review]
With jst's suggestion

Approved for 1.8.1.17, a=dveditz for release-drivers.
Attachment #329803 - Flags: approval1.8.1.17? → approval1.8.1.17+
(Assignee)

Comment 18

10 years ago
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1.17

Updated

9 years ago
Flags: blocking1.8.0.15+

Comment 19

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

a=asac for 1.8.0.15
Attachment #329803 - Flags: approval1.8.0.15+
This is difficult to verify for 1.8.1.17. Any suggestions?
(Assignee)

Comment 21

9 years ago
(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.
Group: core-security
See Also: → bug 1246054
You need to log in before you can comment on or make changes to this bug.