Closed Bug 746376 Opened 12 years ago Closed 12 years ago

IonMonkey: Assertion failure: !templateObject->hasDynamicSlots(), at ion/IonMacroAssembler.cpp:381 or Crash [@ js::HeapPtr::operator]

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Linux
defect
Not set
major

Tracking

()

VERIFIED FIXED
Tracking Status
firefox13 - ---
firefox14 - ---
firefox15 - ---
firefox-esr10 --- unaffected

People

(Reporter: decoder, Assigned: sstangl)

References

Details

(Keywords: assertion, sec-critical, testcase, Whiteboard: [sg:critical] [jsbugmon:update,reconfirm,ignore])

Attachments

(2 files, 3 obsolete files)

Attached file Testcase for shell
The attached testcase asserts on ionmonkey revision 67bf9a4a1f77 (run with --ion -n -m --ion-eager).
Stepping through the assertion crashes with evidence of use-after-free:

Program received signal SIGSEGV, Segmentation fault.
0x00000000004137c4 in js::HeapPtr<js::BaseShape, unsigned long>::operator js::BaseShape* (this=0xdadadadadadadada) at ../../gc/Barrier.h:214
214         operator T*() const { return value; }
(gdb) bt 8
#0  0x00000000004137c4 in js::HeapPtr<js::BaseShape, unsigned long>::operator js::BaseShape* (this=0xdadadadadadadada) at ../../gc/Barrier.h:214
#1  0x0000000000405b9c in js::Shape::base (this=0xdadadadadadadada) at ../../jsscope.h:704
#2  0x0000000000405aa6 in js::Shape::getObjectClass (this=0xdadadadadadadada) at ../../jsscope.h:603
#3  0x000000000040708c in js::ObjectImpl::getClass (this=0x7ffff0914a00) at ../../vm/ObjectImpl-inl.h:245
#4  0x00000000004070aa in js::ObjectImpl::hasClass (this=0x7ffff0914a00, c=0xcc14c0) at ../../vm/ObjectImpl-inl.h:257
#5  0x0000000000425503 in js::ObjectImpl::isDenseArray (this=0x7ffff0914a00) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/vm/ObjectImpl-inl.h:52
#6  0x000000000088981b in js::ion::MacroAssembler::getNewObject (this=0x7fffffff9e70, cx=0xd14d40, result=..., templateObject=0x7ffff0914a00, fail=0xd442c0)
    at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/IonMacroAssembler.cpp:416
#7  0x000000000086db46 in js::ion::CodeGenerator::visitCreateThis (this=0x7fffffff9e30, lir=0xd36078) at /home/ownhero/homes/mozilla/repos/ionmonkey/js/src/ion/CodeGenerator.cpp:1072
(More stack frames follow...)
(gdb) x /i $pc
=> 0x4137c4 <js::HeapPtr<js::BaseShape, unsigned long>::operator js::BaseShape*() const+12>:    mov    (%rax),%rax
(gdb) info reg rax
rax            0xdadadadadadadada       -2676586395008836902
Whiteboard: [jsbugmon:update] → [sg:critical][jsbugmon:update]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision de015aff650d).
Whiteboard: [sg:critical][jsbugmon:update] → [sg:critical] [jsbugmon:update,ignore]
Whiteboard: [sg:critical] [jsbugmon:update,ignore] → [sg:critical] [jsbugmon:update,reconfirm]
JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision bc1833f2111e).
Whiteboard: [sg:critical] [jsbugmon:update,reconfirm] → [sg:critical] [jsbugmon:update,reconfirm,ignore]
The issue in the testcase is that MCall remembers the target JSObject * returned by TI, but by the time the JSObject is used in code generation, the Object has been GC'd. A number of other GCThings are also stored in places in the compiler -- JSFunctions, for example -- without explicit rooting.

To solve this, we'll maintain a global linked list of HeapPtrs to GCThings returned from TI during the current compilation, traced at the start of GC. The linked list itself will be managed using IonMonkey's TempAlloc. Upon return from compilation, the global reference to the linked list will be removed. Apparently HeapPtrs can't get extra dependencies added during IGC, so this should be safe.

Actually using one of these values requires a RootedVar on the stack, in case of moving GC.
Blocks: 505308
Summary: IonMonkey: Assertion failure: !templateObject->hasDynamicSlots(), at ion/IonMacroAssembler.cpp:381 or Crash [@ js::HeapPtr::operator] → IonMonkey: GCThings stored during compilation must be rooted.
Assignee: general → sstangl
It would be very helpful to keep the original subject because it helps us to spot duplicates and also later will allow people to verify bugs more easily (not on IonMonkey but on central later).
Summary: IonMonkey: GCThings stored during compilation must be rooted. → IonMonkey: Assertion failure: !templateObject->hasDynamicSlots(), at ion/IonMacroAssembler.cpp:381 or Crash [@ js::HeapPtr::operator]
Can't seem to reproduce this bug anymore as of this afternoon, even with changesets that previously failed.
Attached patch Fix. (obsolete) — Splinter Review
This should fix the issue by explicitly rooting data remembered during compilation. Leaving without review until the bug manifests again so it can actually be tested.
I'll see if JSBugmon can still repro this.
Whiteboard: [sg:critical] [jsbugmon:update,reconfirm,ignore] → [sg:critical] [jsbugmon:update,reconfirm]
JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision 6e58d603a684).
Whiteboard: [sg:critical] [jsbugmon:update,reconfirm] → [sg:critical] [jsbugmon:update,reconfirm,ignore]
Attached patch Define MarkGCThing. (obsolete) — Splinter Review
MarkGCThingRoot() can't be called after the root marking phase is over. This adds a helper to let CompilerRootNode manually mark roots between IGC slices. To land against m-i.
Attachment #622944 - Flags: review?(wmccloskey)
Attached patch Store and trace compiler roots. (obsolete) — Splinter Review
Attachment #622508 - Attachment is obsolete: true
Attachment #622945 - Flags: review?(wmccloskey)
Attachment #622945 - Flags: review?(dvander)
Comment on attachment 622944 [details] [diff] [review]
Define MarkGCThing.

This seems fine. I don't think you need it for the patch below (see comments), but feel free to check it in if you need it for something else.
Attachment #622944 - Flags: review?(wmccloskey) → review+
Comment on attachment 622945 [details] [diff] [review]
Store and trace compiler roots.

Review of attachment 622945 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Root.h
@@ +145,5 @@
>      template <typename S> inline Handle(const RootedVar<S> &root);
> +#ifdef JS_ION
> +    // Defined in ion/IonGC.h.
> +    template <typename S> inline Handle(const js::ion::CompilerRoot<S> &root);
> +#endif

As discussed, please take this out.

::: js/src/ion/InlineList.h
@@ +171,5 @@
> +    void clear() {
> +        this->next = NULL;
> +        tail_ = this;
> +#ifdef DEBUG
> +        modifyCount_ = 0;

Can you make this field DebugOnly<T> and take out the #ifdef?

::: js/src/ion/Ion.cpp
@@ +76,2 @@
>  IonOptions ion::js_IonOptions;
> +InlineForwardList<CompilerRootNode> ion::compilerRootList;

Because of web workers, we can't really use globals for stuff like this. You might want to store the list on the runtime. I'm not sure what makes js_IonOptions okay either. Is it immutable?

@@ +777,5 @@
>      IonBuilder builder(cx, fp->scopeChain(), temp, graph, &oracle, *info);
> +    bool ok = TestCompiler(builder, graph);
> +
> +    // Destroy list of GCThings to trace: they are either unused or rooted elsewhere.
> +    compilerRootList.clear();

It would be nice to use RAII here. It looks like IonCompile has a number of convenient objects that you might be able to piggyback on. Maybe the IonContext or the IonBuilder?

::: js/src/ion/IonGC.h
@@ +83,5 @@
> +    { }
> +
> +  public:
> +    static CompilerRoot<T> *New(JSContext *cx, T ptr) {
> +        CompilerRoot<T> *root = new CompilerRoot<T>(ptr);

As far as I know, we can't use new in SpiderMonkey. Is there any kind of compiler temp pool that you could allocate out of? Also, you probably need to handle the possibility that this allocation will fail.

@@ +90,5 @@
> +        // Explicitly mark this root if this executes between incremental GC slices:
> +        // the list may have already been traced, and this root would be otherwise missed.
> +        JSCompartment *comp = cx->compartment;
> +        if (comp->needsBarrier())
> +            gc::MarkGCThing(comp->barrierTracer(), root->address(), "ion-compiler-root");

You don't need this code. If the object was allocated during an incremental GC slice, it is automatically marked when the GC returns it. If it was allocated before the incremental GC started, then we must have obtained it somehow, and whoever was holding it when the GC started is responsible for marking it.

Also, this function doesn't need to take a |cx|. So you don't need to pass it down through the MIR. However, you will need to get to the runtime to find the compilerRootList. You can get that via ptr->compartment()->rt.

::: js/src/ion/MIR.h
@@ +1172,4 @@
>      JSFunction *getSingleTarget() const {
> +        if (!target_)
> +            return NULL;
> +        return target_->raw();

Do you need ->raw() (or below)? It seems like the coercion operator should take care of this.
Attachment #622945 - Flags: review?(wmccloskey)
Thank you for the quick review!

(In reply to Bill McCloskey (:billm) from comment #14)
> Because of web workers, we can't really use globals for stuff like this. You
> might want to store the list on the runtime. I'm not sure what makes
> js_IonOptions okay either. Is it immutable?

It's immutable after startup. I'd expect us to keep it immutable, so it should be safe.
 
> As far as I know, we can't use new in SpiderMonkey. Is there any kind of
> compiler temp pool that you could allocate out of? Also, you probably need
> to handle the possibility that this allocation will fail.

IonMonkey overloads new to use the LIFO bump allocator with (asked dvander) a ballast to ensure that a non-negligible amount of space is available; small allocations are therefore "infallible". The CompilerRoot inherits from this allocator, so this new is the overridden one, with the same assumption as the rest of IonMonkey.
Attachment #622944 - Attachment is obsolete: true
Attachment #622945 - Attachment is obsolete: true
Attachment #622945 - Flags: review?(dvander)
Attachment #623334 - Flags: review?(wmccloskey)
Attachment #623334 - Flags: review?(dvander)
Comment on attachment 623334 [details] [diff] [review]
Store and trace compiler roots. [v2]

Review of attachment 623334 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine but the two new files probably need better names - if it's okay to put them all in one file I'd just call it "CompilerRooting.h"
Attachment #623334 - Flags: review?(dvander) → review+
https://hg.mozilla.org/projects/ionmonkey/rev/269dba32ca50
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #623334 - Flags: review?(wmccloskey)
JSBugMon: This bug has been automatically verified fixed.
Status: RESOLVED → VERIFIED
Group: core-security
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.