Closed Bug 638324 Opened 9 years ago Closed 5 years ago

JS_Enumerate doesn't root the JSIdArray it returns

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED WORKSFORME
Tracking Status
blocking2.0 --- .x+
status1.9.2 --- wanted
status1.9.1 --- wanted

People

(Reporter: gal, Assigned: gal)

References

Details

(Keywords: sec-moderate, Whiteboard: [sg:moderate])

Attachments

(1 file)

This is a great foot gun since we don't offer any JS API-way of rooting JSIdArrays, people tend to write code that ends up not rooting the array:

http://mxr.mozilla.org/mozilla-central/source/modules/plugin/base/src/nsJSNPRuntime.cpp#964

Here we get an id array and then call into JS, which might GC.

http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcwrappedjsclass.cpp#434

Dito here.

http://mxr.mozilla.org/mozilla-central/source/xpinstall/src/nsJSInstallTriggerGlobal.cpp#291

Same here.

Basically every use of JS_Enumerate outside the engine is unsafe.

There is a couple ways we can fix this, including handing back an object (thats what JS_NewPropertyEnumerator tries to do, but that API is really ugly too).

I think a nice API is for JS_Enumerate to automatically root the IdArray in the context and JS_DestroyIdArray will unroot it.
Also, I have a hunch that this might be responsible for our #10 top crasher, bug 615098, which is all about MarkId failing. These id arrays contain ... well ids. I don't want to link the dependency in the other bug since this is sg:crit.
Whiteboard: [sg:critical]
I conferred with mrbkap. We think that this is not critical. ids are strings, minus the * object, so its not possible to do any code injection with this. But I still think this might be the top crasher. Asking for 2.0.x and 1.9.1 1.9.2 blocking.
Whiteboard: [sg:critical] → [sg:moderate]
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
blocking2.0: ? → .x+
Attached patch patchSplinter Review
This patch rips out an API that is unused and that is totally broken and really difficult to reign in since it has a very fuzzy ownership model for ida. I think its extremely unlikely that anyone depends on it. If someone really insists on keeping JS_EnumerateResolvedStandardClasses, I am happy to take a patch. Again, in our codebase its unused.

As for JS_Enumerate, this patch tracks the lifetime between JS_Enumerate and JS_DestroyIdArray, which should remove the GC hazard.
Assignee: general → gal
Comment on attachment 516487 [details] [diff] [review]
patch

API removal, better ask Brendan for his ok.
Attachment #516487 - Flags: review?(brendan)
Note: for FF4 and branches it is also an option to just keep the old JS_EnumerateResolvedStandardClasses API and not fix it. We are not using it. Nobody else is. And that one extension using it (seriously unlikely) ... is just out of luck.
Comment on attachment 516487 [details] [diff] [review]
patch

Want more noise when the idArraysHandedOutViaAPI underflows or has a stuck number of elements across inactive cx GCs. r=me with that.

/be
Attachment #516487 - Flags: review?(brendan) → review+
(In reply to comment #6)
> Note: for FF4 and branches it is also an option to just keep the old
> JS_EnumerateResolvedStandardClasses API and not fix it. We are not using it.
> Nobody else is. And that one extension using it (seriously unlikely) ... is
> just out of luck.

It is a useless API. Let's back off and nuke the site from orbit... it's the only way to be sure.

/be
I was thinking the exact same thing when writing the patch. The vector is stored in the thread, so I can't check during GC because threads without a pending request do not participate in the GC protocol. And as for underflow, I can't easily check for that either because JS_DestroyIdArray is called if RegisterIdArray fails, in which case ... well the ida isn't registered. I could split out the guts of DestroyIdArray and stick the check in JS_DestroyIdArray and call DestroyIdArray internally I guess. Will do that.
Blocks: 638347
Do what you can to add checks, otherwise we'll have less pleasant diagnostic fun.

It's possible (IIRC) to enumerate threads in a runtime during a full GC.

/be
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---
http://hg.mozilla.org/tracemonkey/rev/bd821ea0ad41
Whiteboard: [sg:moderate] → [sg:moderate], fixed-in-tracemonkey
Sheila, I think this patch will fix a bunch of crashes with MarkId in the signature.
http://hg.mozilla.org/mozilla-central/rev/bd821ea0ad41
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This got backed-out from TM.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #14)
> This got backed-out from TM.

Please remove FITM from the whiteboard on backouts.
Whiteboard: [sg:moderate], fixed-in-tracemonkey → [sg:moderate]
yeah, I forgot to do that, obviously
Group: javascript-core-security
Group: javascript-core-security
JS_Enumerate still doesn't root the JSIDArray it returns, but every user of it in the tree immediately roots the result using AutoIDArray.  Presumably the exact rooting static analysis will detect if somebody forgets to root things, so I think this is okay to close.
Group: core-security
Status: REOPENED → RESOLVED
Closed: 9 years ago5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.