Assertion failure: IsObjectValueInCompartment(value, compartment())

VERIFIED FIXED in Firefox 33

Status

()

Core
JavaScript Engine
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: Tomcat, Assigned: jimb)

Tracking

(Blocks: 1 bug, {assertion, sec-moderate})

unspecified
mozilla33
x86
All
assertion, sec-moderate
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox33 verified, firefox-esr24 wontfix, firefox-esr31 wontfix)

Details

(Whiteboard: [adv-main33+], URL)

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 8432435 [details]
bughunter stack

found via bughunter on http://www.sherwin-williams.com/store-locator/

loading this site / or reloading cause

Assertion failure: IsObjectValueInCompartment(value, compartment()), at c:\work\mozilla\builds\nightly\mozilla\js\src\vm/ObjectImpl.h:731
(gdb) frame 0
#0  js::ObjectImpl::setSlot (this=0x133a760c0, slot=1, value=@0x7fff5fbfbbd8) at ObjectImpl.h:731
731             MOZ_ASSERT(IsObjectValueInCompartment(value, compartment()));
(gdb) p ((nsGlobalWindow*)js::GetReservedSlot(this->compartment()->global_.value, 0).toPrivate())->mDoc.mRawPtr->mDocumentURI.mRawPtr->mSpec
$23 = {
  <nsACString_internal> = {
    mData = 0x124989b88 "http://www.sherwin-williams.com/store-locator/", 
    mLength = 46, 
    mFlags = 5
  }, <No data fields>}
(gdb) p ((nsGlobalWindow*)js::GetReservedSlot(value.toObjectOrNull()->compartment()->global_.value, 0).toPrivate())->mDoc.mRawPtr->mDocumentURI.mRawPtr->mSpec
$24 = {
  <nsACString_internal> = {
    mData = 0x124989b88 "http://www.sherwin-williams.com/store-locator/", 
    mLength = 46, 
    mFlags = 5
  }, <No data fields>}
(gdb) p this->compartment()
$25 = (class JSCompartment *) 0x12de7f000
(gdb) p value.toObjectOrNull()->compartment()
$26 = (class JSCompartment *) 0x11b614800

This is the source element stuff for scripts; not a JIT issue afaict.  Jim, why aren't we wrapping that into the right compartment?
Component: JavaScript Engine: JIT → JavaScript Engine
Flags: needinfo?(jimb)
Keywords: sec-high
(Assignee)

Comment 3

4 years ago
We're certainly supposed to be wrapping these things. Looking into it.
(Assignee)

Comment 4

4 years ago
I am not able to reproduce this crash. But as far as I can tell, the owning element is supposed to be wrapped by this code:

https://hg.mozilla.org/mozilla-central/file/c90b38c47a1d/content/base/src/nsScriptLoader.cpp#l1091

  // XXXbz this is super-fragile, because the code that _uses_ the
  // JS::CompileOptions is nowhere near us, but we have to coordinate
  // compartments with it... and in particular, it will compile in the
  // compartment of aScopeChain, so we want to wrap into that compartment as
  // well.
  JSAutoCompartment ac(cx, aScopeChain);
  if (NS_SUCCEEDED(nsContentUtils::WrapNative(cx, aRequest->mElement,
                                              &elementVal,
                                              /* aAllowWrapping = */ true))) {
    MOZ_ASSERT(elementVal.isObject());
    aOptions->setElement(&elementVal.toObject());
  }

The crash is in an off-main-thread compilation. Is it possible that the current compartment of the JSContext passed to JS::FinishOffThreadScript might be different from the compartment of the JSContext passed to JS::CompileOffThread?
Flags: needinfo?(jimb)
(Assignee)

Comment 5

4 years ago
It would be possible to change the rules for CompileOptions: it could be SpiderMonkey's responsibility to properly wrap the element. Then, the above "super-fragile" code could be deleted.
(Assignee)

Comment 6

4 years ago
Created attachment 8447508 [details] [diff] [review]
In off-thread compilation, rewrap the compilation options that might need it before saving them on the ScriptSourceObject.

I can't reproduce the crash myself, but this patch addresses my best guess as to the problem. If the try push looks clean, please see if the patch fixes the crash.

https://tbpl.mozilla.org/?tree=Try&rev=60588d4295ab

If it does fix the crash, then the patch should be expanded to cover on-thread compilation as well.
I can't reproduce the crash anymore either.  Either the site or something in our code changed...
(Assignee)

Comment 8

4 years ago
I don't think it's actually sec-critical. It's a compartment violate, but the only thing that ever uses the field that would have the bad cross-compartment reference is Debugger; if the user never opens the devtools, this bug can't affect them.

Even if they do, Debugger will immediately wrap the bad cross-compartment edge in a Debugger.Object, which should be pretty safe. The devtools will just see a surprising reference.
(Assignee)

Comment 9

4 years ago
I've filed bug 1031636 to simplify this code. Perhaps the nsScriptLoader.cpp snarl mentioned above can come out if it lands.
Keywords: sec-high → sec-moderate
(Assignee)

Comment 10

4 years ago
Now that I've landed bug 1031636, I would expect this not to occur any more. It's also no longer reproducible. Closing.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Resolution: FIXED → WORKSFORME
Bob Clary asked me to change this back to FIXED in email, so I will do that.  My interpretation of FIXED was that a fix was in the bug itself, but I guess that does not match the description.
Resolution: WORKSFORME → FIXED
Assignee: nobody → jimb
status-firefox33: --- → fixed
Target Milestone: --- → mozilla33
status-firefox-esr24: --- → wontfix
status-firefox-esr31: --- → wontfix
Whiteboard: [adv-main33+]
Using URL in bug description:
Confirmed assert on Fx32, 2014-06-05.
Verified fixed on Fx33, 2014-10-06.
Status: RESOLVED → VERIFIED
status-firefox33: fixed → verified
Flags: qe-verify+

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.