941766 - Fix an exact rooting hazard in NPAPI

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Terrence Cole [:terrence]]

Attachment: suppress_createobject-v0.diff

The allocation happens through a field call that the analysis cannot see though. Asserting that no gc can happen will let the analysis know it is safe.

Comment 1

[John Schoenick [:johns]]

Comment on attachment 8336211 [details] [diff] [review]
suppress_createobject-v0.diff

Review of attachment 8336211 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +1371,5 @@
>  
>    NPObject *npobj;
>  
>    if (aClass->allocate) {
> +    JS::AutoAssertNoGC nogc; // The JS GC hazard analysis cannot see through this field call.

Does this inhibit GC or merely assert that we cannot GC here? As far as I'm aware, there's nothing stopping a particularly insane plugin from calling arbitrary JS inside a custom allocator :(

Comment 2

[Benjamin Smedberg]

yeah, we shouldn't assume that ->allocate is safe, especially for Java. Once Java is OOPP then we probably can assume that all plugins are OOPP and this would become safe.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8336211 [details] [diff] [review]
suppress_createobject-v0.diff

You are correct: if that can run arbitrary code then we'll need to exactly root here. Eww.

I'll draft a patch to do that instead.

Comment 4

[Terrence Cole [:terrence]]

Attachment: hazard_createobject-v1.diff

This turned out to be pretty simple. Since |key| is just a trivial typey wrapper, we can just re-generate it from the rooted object after the potential GC.

Comment 5

[Terrence Cole [:terrence]]

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

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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