635137 - failure to root when using JSString::getChars(Z) with newly allocated strings

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jeff Walden [:Waldo]]

I was looking at some JSON code and saw what looked like another failure-to-anchor bug.  I poked Luke about it, and we determined nothing would go wrong so long as JSContext::malloc can't GC.  So we looked at that to see whether it could, and it comes down to whether the registered error reporter can GC.  (JSContext::mallocNoReport would be the alternative.)  So I looked at error reporters.  While NS_ScriptErrorReporter looks careful to not do anything, a number of others seem capable of invoking script again:

 * DOMWorkerErrorReporter synchronously calls an onerror handler
 * XBL_ProtoErrorReporter logs a message to the console which sync-notifies
   observers synchronously
 * mozJSLoaderErrorReporter also logs a console message

So it looks like most to all cx->mallocs can GC right now, and as I understand it we (JS engine) had thought of that as an invariant we could (and do) rely upon.

CCed a semi-random bunch of people associated with these reporters or with the JS engine, feel free to add other people if it makes sense.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ideally, we'd make error reporting async on the JS side, but JS itself has no concept of an event loop and whatnot....

Failing that, we should fix all those error reporters to post events, imo.

Comment 2

[Johnny Stenback (:jst)]

Boris, can you look into this for 2.0 final? If not, let me know...

Comment 3

[Andreas Gal :gal]

waldo, can you please point out the code where this is causing a rooting hazard?

Comment 4

[Andreas Gal :gal]

Stealing from bz.

Comment 5

[Andreas Gal :gal]

cx->malloc is a valid GC point IMO. Call sites that are not aware of it are wrong and needs to be fixed. We don't have to touch the error reporters.

I talked to Waldo. The case Luke and him found is in json. We grab the chars part of a string and the string might become unrooted under us. This is a bug that we ran into before and we have anchors for it. Waldo is right that its a pain to audit the code for this but thats the right thing to do here.

I will audit the code and add anchors. That patch should be very low risk and done in a few.

Comment 6

[Andreas Gal :gal]

Attachment: patch

Comment 7

[Andreas Gal :gal]

We only read from these strings (they are flat by definition, so no sharing of buffers via dependent strings). I don't think this is exploitable.

Comment 8

[Andreas Gal :gal]

This patch adds anchors in a few places that follow the same pattern that Waldo found. This is not a good complete fix. I am pretty sure I missed more places like this. We should model this with a nice type system. AutoAnchoredStringPtr and then allow getChars(Z) only on that. We should also get rid of the JS API calls that allow extracting string buffers. But thats all post-4 fodder. I will file a follow-up bug after this bug has landed.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 514969 [details] [diff] [review]
patch

omg the extra store caused by the Anchor is going to totall kill our e4x perf

Comment 10

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I'd be happy to do a belt-and-suspenders fix to the error reporters too.  It's really not that hard, and I'm sort of free at the moment.  Let me know.

Comment 11

[Andreas Gal :gal]

It can't hurt, but I don't think it helps much either. We do all sorts of stuff while holding getChars() and not rooting the string, the least of which is OOM reporting. The 2 other places I found would not be fixed by cleaning up OOM handling.

Comment 12

[Andreas Gal :gal]

We should check 1.9.2 and 1.9.1.

Comment 13

[Andreas Gal :gal]

Bleh, I messed up and this got split into two almost identical changesets, both with the same title. Go me.

http://hg.mozilla.org/tracemonkey/rev/95d1efa60347
http://hg.mozilla.org/tracemonkey/rev/06e88afd8bbf

Comment 14

[Johnny Stenback (:jst)]

Landed in mozilla-central (as one combined patch):

http://hg.mozilla.org/mozilla-central/rev/a5eccf9bebda

Marking fixed.

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/95d1efa60347

Comment 16

[Daniel Veditz [:dveditz]]

branches don't have Anchor<> so we'd need a completely different branch patch.