Closed Bug 1298773 Opened 8 years ago Closed 8 years ago

Intermittent dom/devicestorage/test/test_app_permissions.html | application crashed [@ js::CompartmentChecker::check(JSObject *)] after Assertion failure: IsInsideNursery(obj) || !obj->asTenured().isMarked(gc::GRAY)

Categories

(Core :: JavaScript: GC, defect, P3)

Product:
Core
Component:
JavaScript: GC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox-esr45 50+ fixed
firefox50 + fixed
firefox51 + fixed
firefox52 + fixed
firefox53 + fixed

People

(Reporter: intermittent-bug-filer, Assigned: jonco)

References

Details

(5 keywords, Whiteboard: [adv-main50.1+][adv-esr45.6+])

Attachments

(5 files)

Log which argument to CallJSNative is bad.
1.52 KB, patch
Details | Diff | Splinter Review
expose_in_getwrapper-v1.diff
1.11 KB, patch
jonco
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
ritu
: approval-mozilla-release+
abillings
: sec-approval+
Details | Diff | Splinter Review
Make ArrayBufferInputStream copy its input buffer
7.26 KB, patch
jonco
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
ritu
: approval-mozilla-release+
abillings
: sec-approval+
Details | Diff | Splinter Review
bug1298773-ArrayBufferInputStream-esr45
6.87 KB, patch
ritu
: approval-mozilla-esr45+
Details | Diff | Splinter Review
bug1298773-expose-wrapper-target-esr45
1.13 KB, patch
ritu
: approval-mozilla-esr45+
Details | Diff | Splinter Review
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Component: DOM → JavaScript: GC
Keywords: assertion, crash
The crashes here look like calls on CCWs.
Right. We had a similar problem that we fixed in bug 1289428. I wish we had some idea of which thing in the CallArgs is gray: an argument, or thisv, or callee or something else... :(
I hacked up a patch that should log which element of the array is bad. I'll try to gin up some failures. The failures in this bug are almost all Win8 Moth. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9bdb5572f3b
I got around to doing some retriggers today, and I managed to get a reproduction. My patch iterates over args, from base() to end(), and prints out the position in that iteration of any gray object it finds. In the case I hit, it printed out 1, so the second element in the iteration. It looks like base() is |argv_ - 2|, so I suppose the second element we iterate over is |argv_ - 1|, which looks like it is thisv, judging by setThis(). Is that useful, Boris? Should I see if it is consistently the same index (by doing more retriggers)?
Flags: needinfo?(bzbarsky)
That's useful, yes! Gray thisv is also what we had in bug 1289428... And this situation is _kinda_ similar in that we're going through a get on a cross-compartment wrapper. OK. So where is thisv coming from in this stack? The most proximate place is InternalCall (which is actually missing in the stacks, afaict because it got inlined), where thisv() is either preserved as-is or replaced by GetThisValue(thisv). GetThisValue will do things like ToWindowProxyIfWindow and some other things like that. May be worth double-checking whether any of those can cross a black-to-gray edge somehow, but seems unlikely. Before that, the thisv is passed around as the thisv or receiver argument, and is just passed through as-is until we get to CrossCompartmentWrapper::get (going backwards in time here, in case that wasn't clear). Here we do a WrapReceiver call to produce the thing we will pass through. Oh, so here I bet is where the problem is. WrapReceiver tries to be all smart and fast and bypass the slow wrap() call, so it does: if (ObjectValue(*wrapper) == receiver) { JSObject* wrapped = Wrapper::wrappedObject(wrapper); if (!IsWrapper(wrapped)) { MOZ_ASSERT(wrapped->compartment() == cx->compartment()); MOZ_ASSERT(!IsWindow(wrapped)); receiver.setObject(*wrapped); return true; } } and here we have a problem just like the one we had in bug 1289428. I expect it's the same setup: wrapper and its target (what will become the receiver) both start out gray, we start a compartmental GC in the compartment of "wrapper" and mark it white, WrapReceiver pulls out the gray thing and returns it in the "receiver" outparam. So WrapReceiver should do the relevant expose call... I guess we could have found this through very careful code inspection, but knowing we wanted specifically the thisv sure made it simpler. But wait, it gets worse. I looked at some of the other uses of wrappedObject in CrossCompartmentWrapper. Let's consider something like CrossCompartmentWrapper::enumerate. It does the following: 1) Enters the compartment of wrappedObject(). 2) Calls Wrapper::enumerate with the wrapper as arg. Wrapper::enumerate then grabs the proxy target (aka the wrapper target) and calls GetIterator. So we could fail the same exact way there: GetIterator would get a gray second arg. Or am I missing something? If I'm not missing something, we could try to handle this bit in Wrapper code: all the places where it works with the proxy target it could unmark gray. Or we could handle this in CrossCompartmentWrapper (which is the only one this should be an issue for?): all the places where it enters the wrappedObject(wrapper) compartment it could unmark gray.... Note that the latter relies on there not being gc before the next time Wrapper pokes at the proxy target, because it's not like CrossCompartmentWrapper is _rooting_ the thing whose compartment it enters. So maybe we could in fact do the unmark gray as needed in CrossCompartmentWrapper, make sure to root the wrappedObject for the duration of us being in the compartment, and document the heck out of it or something... Note that CrossCompartmentWrapepr::call _does_ root wrappedObject(wrapper), in exactly this way. Anyway, we definitely need to audit all the wrappedObject() callsites. For example, I think CrossCompartmentWrapper::nativeCall has shenanigans too: it roots the wrappedObject(), but never unmarks gray on it, and then pokes it into the CallArgs afaict. Seems to me like we should make wrappedObject unmark gray and add a "PreserveColor" variant for the few places that shouldn't, or something.... We may want to spin some of this off into a separate bug and just fix the WrapReceiver bit here, though. Terrence, any suggestions on what the right approach here is, or whose problem this should be? ;)
Flags: needinfo?(bzbarsky) → needinfo?(terrence)
Our only other option is [1]. But that's just trading one escape analysis for another, so I think our best bet is to just do the simplest thing: e.g. what you proposed, ExposeToActiveJS in wrapperObject. The static rooting analysis should already be catching places where we don't Root, so I think that's a bit of a red-herring (I hope!). Or hurm, maybe we do need [1] after all. If the wrappee escapes into script (e.g. as thisv), then we need it to be black regardless, but if it doesn't escape then we want it to not be exposed *unless* it participates in GC by being on the stack. E.g. We'd need to store it in a WrapperRooted. Of course this last is already true (as there are no WrapperRooters in this code), so I think there must not be places where we both don't want to expose and may GC other than Nuke. This is too complicated. Let's just Expose in wrapperObject and see what breaks. 1- http://searchfox.org/mozilla-central/source/js/src/jscompartment.h#982-994
Flags: needinfo?(terrence)
> The static rooting analysis should already be catching places where we don't Root, > so I think that's a bit of a red-herring The concern are cases which look like this: AutoCompartment call(cx, wrappedObject(wrapper)); // do something that can GC (and hence mark things gray again). // Call some Wrapper::* hook that will look at the wrapper target _without_ going through wrappedObject. This is not a hypothetical situation. We get exactly this in the current CrossCompartmentWrapper::get code, which looks like this: AutoCompartment call(cx, wrappedObject(wrapper)); if (!WrapReceiver(cx, wrapper, &receiverCopy)) return false; if (!Wrapper::get(cx, wrapper, receiverCopy, id, vp)) return false; Here Wrapper::get ends up using the wrapper target explicitly like so: RootedObject target(cx, proxy->as<ProxyObject>().target()); which is not going through wrappedObject(). At the same time, WrapReceiver can gc, on the codepath that calls cx->wrap(). Maybe this is all a non-issue because we know wrapper itself is rooted so any GC under WrapReceiver is guaranteed to mark it black and hence mark its target black? The other worry I have is things that call Wrapper::* things that use ProxyObject::target without going through CrossCompartmentWrapper::*. I don't know whether this is a thing. Someone needs to audit all uses of ProxyObject::target and ways they can be reached and whether any of those cases involve the proxy being a CrossCompartmentWrapper instance (i.e. CrossCompartmentWrapper, SecurityWrapper<CrossCompartmentWrapper>, or one of the XrayWrapper classes; I _hope_ that's everything that subclasses CrossCompartmentWrapper). Oh, and I hope CrossCompartmentWrapper subclasses are the only case in which we have a cross-compartment proxy target...
To be clear, ProxyObject::target is only a problem if it can be reached for a cross-compartment case _without_ having done a wrappedObject() call first.
Jon please take a look.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
See Also: → 1292852
Terrence, does bug 1227750 fix this or is there more work to be done here? It seems to me that that will mark the wrapped object black in the situation where we have a zone GC that collects the wrapper zone but not the wrapped zone.
Flags: needinfo?(jcoppeard) → needinfo?(terrence.d.cole)
Oh, I suppose that's possible! The intended fix is the one proposed by Boris and implemented in comment 19. That angered JetPack though and I was not able to get it to reproduce locally in the couple hours I tried last week. I'll at least upload the patch.
Flags: needinfo?(terrence.d.cole)
Looking at the stack traces again and I'm not sure why I was so confused before. The crash is happening because the wrapper target is nullptr. Not all wrappers have targets though, so I think this is actually fine. I'm pretty sure this case is what we do when someone steals the array buffer contents so that future access will not be able to re-steal it: e.g. the target is intentionally nullptr. In any case we need to nullcheck here before Exposing, regardless.
And here is the patch with null checking. Try run is up at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af0f428548e9
Assignee: jcoppeard → terrence.d.cole
Status: NEW → ASSIGNED
Attachment #8794279 - Flags: review?(jcoppeard)
Comment on attachment 8794279 [details] [diff] [review] expose_in_getwrapper-v1.diff Review of attachment 8794279 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8794279 - Flags: review?(jcoppeard) → review+
Pushed by tcole@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ac1ae4224375 Expose wrappees that may be used through a wrapper; r=jonco
Prior try run was green except for JP, so I figure I might as well just push this now. Try run cancelled.
Welp, that just made it madder! What in the world is ArrayBuffer storing here that a null check didn't work?
Steve, I don't have time to figure out what's going on with |target| here, could you take a look when you get back?
Flags: needinfo?(sphink)
Assignee: terrence.d.cole → jcoppeard
Flags: needinfo?(sphink)
This is crashing because JS_GetArrayBufferByteLength is being called from off the main thread (and not even on a JS helper thread). 0 xul.dll!js::CurrentThreadIsIonCompiling() [Barrier.cpp:4e86cc2b7a2b : 69 + 0xa] 1 xul.dll!js::gc::TenuredCell::writeBarrierPre(js::gc::TenuredCell *) [Heap.h:4e86cc2b7a2b : 1316 + 0x5] 2 xul.dll!JS::DispatchTyped<IncrementalReferenceBarrierFunctor>(IncrementalReferenceBarrierFunctor,JS::GCCellPtr) [HeapAPI.h:4e86cc2b7a2b : 267 + 0xe] 3 xul.dll!JS::IncrementalReferenceBarrier(JS::GCCellPtr) [jsgc.cpp:4e86cc2b7a2b : 7330 + 0xb] 4 xul.dll!js::gc::ExposeGCThingToActiveJS [GCAPI.h:4e86cc2b7a2b : 613 + 0xc] 5 xul.dll!JS::ExposeObjectToActiveJS [GCAPI.h:4e86cc2b7a2b : 646 + 0x10] 6 xul.dll!js::Wrapper::wrappedObject(JSObject *) [Wrapper.cpp:4e86cc2b7a2b : 334 + 0x6] 7 xul.dll!js::CheckedUnwrap(JSObject *,bool) [Wrapper.cpp:4e86cc2b7a2b : 366 + 0x3e] 8 xul.dll!JS_GetArrayBufferByteLength(JSObject *) [ArrayBufferObject.cpp:4e86cc2b7a2b : 1570 + 0xa] 9 xul.dll!ArrayBufferInputStream::ReadSegments(nsresult (*)(nsIInputStream *,void *,char const *,unsigned int,unsigned int,unsigned int *),void *,unsigned int,unsigned int *) [ArrayBufferInputStream.cpp:4e86cc2b7a2b : 89 + 0x13] 10 xul.dll!nsMultiplexInputStream::ReadSegments(nsresult (*)(nsIInputStream *,void *,char const *,unsigned int,unsigned int,unsigned int *),void *,unsigned int,unsigned int *) [nsMultiplexInputStream.cpp:4e86cc2b7a2b : 310 + 0x25] 11 xul.dll!nsStreamCopierIB::DoCopy(nsresult *,nsresult *) [nsStreamUtils.cpp:4e86cc2b7a2b : 559 + 0x21] 12 xul.dll!nsAStreamCopier::Process() [nsStreamUtils.cpp:4e86cc2b7a2b : 319 + 0xf] Steve, do you know if that's meant to be allowed? It's a friend API so I guess it might be an intended usage.
Flags: needinfo?(sphink)
Wait, what? How does ArrayBufferInputStream make any possible sense as written? ReadSegments can be called on arbitrary necko threads, but it's using JSObject pointers... which can change at any time due to GC running on the JSRuntime thread. Or put another way, this call: char* src = (char*) JS_GetArrayBufferData(mArrayBuffer->get(), &isShared, nogc) + mOffset + mPos; looks totally unsafe to me: the object can get moved between the mArrayBuffer->get() call returning and the JS_GetArrayBufferData call being entered.
Group: dom-core-security
Flags: needinfo?(josh)
That's not to mention the raciness if someone is modifying the arraybuffer while the stream is being read from.. Seems to me like ArrayBufferInputStream should either make an up-front copy of the data or detach the passed-in array buffer and steal the data from it. It's not clear to me which behavior would be better for the original TCPSocket use case, and there's no spec for it, right?
(In reply to Jon Coppeard (:jonco) from comment #33) > This is crashing because JS_GetArrayBufferByteLength is being called from > off the main thread (and not even on a JS helper thread). > > Steve, do you know if that's meant to be allowed? It's a friend API so I > guess it might be an intended usage. I would not expect that to be allowed. I think we should assert in JS_GetArrayBuffer* that you're calling from the main thread. Other things that access the contents take a cx, which makes it harder to mess up in this way. (The cx was removed from various functions in bug 800915.) I see that JS_GetArrayBufferData already requires a nogc token to be passed in. But the nogc token isn't necessarily the right thing -- we're offthread, so we definitely aren't going to GC, but that doesn't mean we can do cross-thread accesses. I haven't read the code, but I wonder if it needs to be reading into an ArrayBuffer at all these days. At one point, you couldn't easily adopt an external buffer, but now you can. Would that simplify things, in addition to fixing up the data race? Hmm... I see that it's passing the buffer into an nsWriteSegmentFun. That doesn't seem valid in any situation where you're not on the buffer's JSRuntime's main thread. Perhaps you could patch this together with creative use of JS_ExternalizeArrayBufferContents. The pointer returned from that should never be updated on a GC, and it'll stay live as long as you have your PersistentRooted on the buffer (but the PersistentRooted should be created on the main thread too.) You still can't access the ArrayBufferObject itself, but an externalized data pointer is conceivably safe to use on another thread if you can be sure that nothing else will modify the data it points to. (Or rather: an externalized data pointer is usable on other threads. The data it points to can still be raced on.)
Flags: needinfo?(sphink)
Yeah, this stuff is clearly wrong. There's no spec for how it should behave; I lean towards detaching the array buffer because this stuff is used for e10s WebRTC and that sounds better for controlling memory usage. Should I go and fix this, or is someone else looking at it?
Flags: needinfo?(josh)
Given that, I think we should be able to just JS_StealArrayBufferContents here when initially setting up the stream, cast the result to char* or whatever it is we want, and not hold on to the JS object at all. We could probably just make our thing a subclass of nsStringInputStream and use ShareData() to point it to the right place in our buffer. We can't use AdoptData because we might have a nonzero offset. However note that https://dxr.mozilla.org/addons/search?q=ArrayBufferInputStream shows a bunch of uses of ArrayBufferInputStream in addons, including in the addon sdk. That makes things a bit extra-fun. :(
Maybe we should leave the current API but have it make a copy of the data up front, while adding a new API that does the JS_StealArrayBufferContents() thing and using it in the places we know it's safe... Note that I am not planning to work on this for the moment.
(In reply to Josh Matthews [:jdm] from comment #37) Do you have time to work on this? AFAIK no one else is looking at it.
Flags: needinfo?(josh)
Yes.
Flags: needinfo?(josh)
This does the straightforward copy strategy.
Assignee: jcoppeard → josh
Attachment #8806137 - Flags: review?(jcoppeard)
Assignee: josh → jcoppeard
Comment on attachment 8806137 [details] [diff] [review] Make ArrayBufferInputStream copy its input buffer Review of attachment 8806137 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, thank you for fixing!
Attachment #8806137 - Flags: review?(jcoppeard) → review+
Comment on attachment 8794279 [details] [diff] [review] expose_in_getwrapper-v1.diff [Security approval request comment] How easily could an exploit be constructed based on the patch? Very hard. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? This has always been this way AFAIK, so all of them. If not all supported branches, which bug introduced the flaw? N/A. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be trivial. How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8794279 - Flags: sec-approval?
Comment on attachment 8806137 [details] [diff] [review] Make ArrayBufferInputStream copy its input buffer [Security approval request comment] How easily could an exploit be constructed based on the patch? I think that would be difficult. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? Everything back to FF23. If not all supported branches, which bug introduced the flaw? It looks like this has used an array buffer object since it was added in bug 831107. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be simple. How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8806137 - Flags: sec-approval?
Sec-approval+ for trunk checkin on 11/29 (two weeks into the new cycle) because we're about to make builds for shipping Firefox 50. At that time, we'll want Aurora, Beta, and ESR45 patches made and nominated as well.
status-firefox50: --- → wontfix
status-firefox51: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox51: --- → +
tracking-firefox-esr45: --- → 51+
Whiteboard: [checkin on 11/29]
Attachment #8794279 - Flags: sec-approval? → sec-approval+
Attachment #8806137 - Flags: sec-approval? → sec-approval+
Blocks: 1317672
https://hg.mozilla.org/mozilla-central/rev/11a5444e54a9 https://hg.mozilla.org/mozilla-central/rev/48fc404b0637
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please nominate this for Aurora/Beta/Release/ESR45 approval ASAP so we can hopefully get it uplifted in time for 51b6.
tracking-firefox53: --- → ?
Flags: needinfo?(jcoppeard)
Tracking 53+ for this sec high issue.
tracking-firefox53: ? → +
Hi Dan, this is a sec-high. Do we need this in 50.1.0?
Flags: needinfo?(dveditz)
Comment on attachment 8794279 [details] [diff] [review] expose_in_getwrapper-v1.diff Approval Request Comment [Feature/Bug causing the regression]: JS wrapper objects? It has been this way for a long time. [User impact if declined]: Potential crash / security vulnerability. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: The other patch in this bug. [Is the change risky?]: No. [Why is the change risky/not risky?]: This is a small change to ensure anything returned from the JS API is not marked gray by calling ExposeObjectToActiveJS on it. This is always a safe thing to do. [String changes made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8794279 - Flags: approval-mozilla-release?
Attachment #8794279 - Flags: approval-mozilla-beta?
Attachment #8794279 - Flags: approval-mozilla-aurora?
Comment on attachment 8806137 [details] [diff] [review] Make ArrayBufferInputStream copy its input buffer Approval Request Comment [Feature/Bug causing the regression]: Bug 831107. [User impact if declined]: Possible crash / security vulnerability. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: The other patch in this bug. [Is the change risky?]: No. [Why is the change risky/not risky?]: This is a simplification of the existing code and removes the possibility of racy access to a JS object. [String changes made/needed]: None.
Attachment #8806137 - Flags: approval-mozilla-release?
Attachment #8806137 - Flags: approval-mozilla-beta?
Attachment #8806137 - Flags: approval-mozilla-aurora?
Comment on attachment 8816429 [details] [diff] [review] bug1298773-ArrayBufferInputStream-esr45 [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug. User impact if declined: Possible crash / security vulnerability. Fix Landed on Version: FF 53. Risk to taking this patch (and alternatives if risky): Low. String or UUID changes made by this patch: None.
Attachment #8816429 - Flags: approval-mozilla-esr45?
Comment on attachment 8816430 [details] [diff] [review] bug1298773-expose-wrapper-target-esr45 [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug. User impact if declined: Possible crash / security vulnerability. Fix Landed on Version: FF 53. Risk to taking this patch (and alternatives if risky): Low. String or UUID changes made by this patch: None.
Attachment #8816430 - Flags: approval-mozilla-esr45?
Comment on attachment 8794279 [details] [diff] [review] expose_in_getwrapper-v1.diff Sec-high, approving for all branches.
Attachment #8794279 - Flags: approval-mozilla-release?
Attachment #8794279 - Flags: approval-mozilla-release+
Attachment #8794279 - Flags: approval-mozilla-beta?
Attachment #8794279 - Flags: approval-mozilla-beta+
Attachment #8794279 - Flags: approval-mozilla-aurora?
Attachment #8794279 - Flags: approval-mozilla-aurora+
Attachment #8806137 - Flags: approval-mozilla-release?
Attachment #8806137 - Flags: approval-mozilla-release+
Attachment #8806137 - Flags: approval-mozilla-beta?
Attachment #8806137 - Flags: approval-mozilla-beta+
Attachment #8806137 - Flags: approval-mozilla-aurora?
Attachment #8806137 - Flags: approval-mozilla-aurora+
Attachment #8816429 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Attachment #8816430 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
I really wish you folks hadn't just checked this into 50.1 without approval from me or Dan...
Flags: needinfo?(dveditz) → needinfo?(rkothari)
Whiteboard: [adv-main50.1+]
Whiteboard: [adv-main50.1+] → [adv-main50.1+][adv-esr45.6+]
Group: dom-core-security → core-security-release
Group: core-security-release
Depends on: 1349419
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: