Closed Bug 811050 Opened 8 years ago Closed 8 years ago

Assertion failure: thing->compartment()->rt == trc->runtime, at Marking.cpp:91

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 --- unaffected
firefox18 + fixed
firefox19 + fixed
firefox20 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: eeejay, Assigned: sfink)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main18-])

Attachments

(3 files)

I manage to bring up this failure with a tweaked mochitest.
Crash Signature: #0 0x00007ffff47e4b64 in js::gc::CheckMarkedThing<JSObject> (trc=0x7fffe61022a8 thing=0x7fffcee5fb20) at /home/eitan/Projects/mozilla/mozilla-central/js/src/gc/Marking.cpp:91 #1 0x00007ffff47e389a in js::gc::MarkInternal<JSObject> (trc=0x7fffe6102…
Please post stacks as an attachment, not as inline comments, and definitely not in the crash signature field.

Why do you think this is related to workers?  The crash is on the main thread.
Crash Signature: #0 0x00007ffff47e4b64 in js::gc::CheckMarkedThing<JSObject> (trc=0x7fffe61022a8 thing=0x7fffcee5fb20) at /home/eitan/Projects/mozilla/mozilla-central/js/src/gc/Marking.cpp:91 #1 0x00007ffff47e389a in js::gc::MarkInternal<JSObject> (trc=0x7fffe6102…
I suppose it does involve different runtimes.  That smells workerish.
Attached patch make test crashSplinter Review
I suspect GC needs to be forced in the worker, and I don't know how to do that. On my machine the assertion will come up after around 2000 test1 calls.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> I suppose it does involve different runtimes.  That smells workerish.

Yeah, has something to do with the ByteArray having a view and neutering.
Attached file Back trace
Steve, could you take a look at this? It seems like a worker object might be leaking into the main thread.
No longer blocks: transferables
Group: core-security
Eitan: thanks for the really nice test case!

The problem is that when transferring the contents of an ArrayBuffer to a new owner ArrayBuffer, it was inheriting the old buffer's list of views as well. I marked this s-s because it means you have an indirect way of accessing somebody else's memory (a view in the origin compartment until it gets GC'd, then some random memory after that.) It's probably pretty hard to do much with it, but it still smells a little dangerous.

The quick fix of NULLing out the view list when accepting a contents buffer seems to be running fine without crashing, but really that pointer should never be inside of a returned contents buffer in the first place.
Zero out the view list pointer before returning contents for JS_StealArrayBufferData
Attachment #681267 - Flags: review?(wmccloskey)
Assignee: nobody → sphink
Attachment #681267 - Flags: review?(wmccloskey) → review+
That sounds bad, so marking sec-high. Feel free to lower or raise as appropriate.
Keywords: sec-high
Steve, what branches are affected here and should we request tracking?
It goes back to beta, unfortunately.

It probably doesn't need to be sec-high, but I'm no expert. It only provides very indirect access to the data -- the only thing that value is used for is walking down the list of views to neuter them. So if the origin views are still alive, this will have no effect because the only thing you could do to them is neuter them, and they'll already be neutered. If they became garbage, then you could use this to write a couple of zeroes into the headers of new objects in the origin compartment. Bad, but I lack the imagination to see how that's exploitable.
I added additional rooting to workers in bug 814026 and had a bizarre round of additional crashes that caused it to be backed out... Is there some chance that this is the underlying cause?
Seems unlikely. The immediate memory corruption that this would cause would be in objects of the same alloc kind, and you're seeing crashes in JSStrings. (You *are* using workers, though, which is the only thing I know of that steals buffers right now. So it's not out of the question that this is affecting you.)
Attachment #681267 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/b3d02828b6f3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment on attachment 681267 [details] [diff] [review]
Initialize stolen ArrayBuffer data

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 720949
User impact if declined: crashes, security hole
Testing completed (on m-c, etc.): on m-c, test case passes
Risk to taking this patch (and alternatives if risky): low. This is NULLing out an invalid pointer, preventing traversal through it to dangerous lands.
String or UUID changes made by this patch: none
Attachment #681267 - Flags: approval-mozilla-beta?
Attachment #681267 - Flags: approval-mozilla-aurora?
Comment on attachment 681267 [details] [diff] [review]
Initialize stolen ArrayBuffer data

low risk - go ahead with landing today so it gets into beta 4 tomorrow.
Attachment #681267 - Flags: approval-mozilla-beta?
Attachment #681267 - Flags: approval-mozilla-beta+
Attachment #681267 - Flags: approval-mozilla-aurora?
Attachment #681267 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.