Closed Bug 994281 Opened 6 years ago Closed 6 years ago

Assertion failure: bufferByteLength - arrayByteOffset >= arrayByteLength, at vm/TypedArrayObject.cpp or Assertion failure: arrayByteOffset <= bufferByteLength, at vm/TypedArrayObject.cpp

Categories

(Core :: JavaScript Engine: JIT, defect, P1, critical)

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox28 --- unaffected
firefox29 --- unaffected
firefox30 --- unaffected
firefox31 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: gkw, Assigned: terrence)

References

(Blocks 1 open bug)

Details

(4 keywords)

Attachments

(4 files, 1 obsolete file)

Attached file stack
The upcoming testcase asserts js debug shell on m-c changeset 43cd629879c2 with --ion-parallel-compile=off --no-baseline --no-asmjs --ion-eager at Assertion failure: bufferByteLength - arrayByteOffset >= arrayByteLength, at vm/TypedArrayObject.cpp

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

Not bisecting because of the fragility of the testcase - I had to vary the value of gcslice to get this to reproduce on a local compiled shell vs one from tbpl, so if it doesn't reproduce, please try varying it first.

Setting needinfo? from Waldo, the patch in bug 991981 comment 2 does not seem to fix this. s-s because this is either a TypedArray or GC issue, or both.
Flags: needinfo?(jwalden+bmo)
Attached file reduced testcase
Attached file less reduced testcase
This less-reduced testcase asserts at:

Assertion failure: arrayByteOffset <= bufferByteLength, at vm/TypedArrayObject.cpp
Summary: Assertion failure: bufferByteLength - arrayByteOffset >= arrayByteLength, at vm/TypedArrayObject.cpp → Assertion failure: bufferByteLength - arrayByteOffset >= arrayByteLength, at vm/TypedArrayObject.cpp or Assertion failure: arrayByteOffset <= bufferByteLength, at vm/TypedArrayObject.cpp
Going to mark this sec-critical for now, typed array assertions are very scary (ref recent pwn2own exploits), please feel free to re-rate this after further analysis.
Group: core-security
Keywords: sec-critical
Waldo: can you take a look at this sec-crit assertion?
Priority: -- → P1
(I think Waldo mentioned somewhere that this is pending the more comprehensive fix for typed arrays post-pwn2own)
Looking at this in a debugger: the typed array has a length and a data pointer, but the typed array's buffer and its length/data pointer aren't consistent with that.  Something's whack.  It's not inconceivable this is caused by bug 993768, somehow, but if I apply that patch (and assuming I unbitrotted it correctly) it doesn't fix things.  Hmm.
Flags: needinfo?(jwalden+bmo)
This is some sort of GC and/or view list manipulation issue.

Here's the important parts of attachment 8404172 [details] (stupid bits replaced with placeholder comments):

  //DDBEGIN
  x = ArrayBuffer(64);
  gcslice(4972);
  y = Uint32Array(x);
  y.subarray(0);
  for (var i = 0; i < 99999; i++) {
    "(void 0)".charCodeAt(i);
  }
  neuter(x);
  y.subarray(0);
  //DDEND

Sometime during the mega-loop, a GC happens.  We trace both views.  Then ArrayBufferObject::sweep is called.  At the time of the GC the ArrayBuffer *should* have two views, one for |y|, one for the abandoned subarray.  It has this.  During sweeping we progress to this point:

        // Rebuild the list of views of the ArrayBufferObject, discarding dead
        // views.  If there is only one view, it will have already been marked.
        ArrayBufferViewObject *prevLiveView = nullptr;
        ArrayBufferViewObject *view = viewsHead;
        while (view) {
            JS_ASSERT(buffer->compartment() == view->compartment());
            ArrayBufferViewObject *nextView = view->nextView();
            if (!IsObjectAboutToBeFinalized(&view)) {
                view->setNextView(prevLiveView);
                prevLiveView = view;
            }
            view = UpdateObjectIfRelocated(rt, &nextView);
        }

        buffer->setViewList(prevLiveView);

It happens that IsObjectAboutToBeFinalized is true for *both* views, because |!view->isMarked()| for both of them.  (This is true even during the ArrayBufferObject::trace calls!  At least, if we're careful not to check at inopportune times that cause stuff to crash.)  This is clownshoes.  The result is the buffer's view list gets set to nullptr, the subsequent |neuter(x);| doesn't update state in the views, and we hit inconsistency assertions in the final subarray creation.

Terrence, billm claims this is a generational GC thingamabob best left to you to fix.  Go ahead, make my day.  ;-)
Flags: needinfo?(terrence)
Yes, it is. These particular bits are... eww. AFAICT, everything touching the weak view list is a pile of hacks. If mine happens to be on top right now, I'll take a look Monday.

I'm going to needinfo Steve too: his hacks are on the bottom of the pile, so maybe he'll have some idea at all of what we should do here.
Flags: needinfo?(terrence) → needinfo?(sphink)
Assignee: nobody → sphink
Yeah, this one was totally my fault. Thankfully only 31 is affected, so lets get this in asap.
Assignee: sphink → terrence
Status: NEW → ASSIGNED
Attachment #8409762 - Flags: review?(sphink)
Flags: needinfo?(sphink)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #5)
> (I think Waldo mentioned somewhere that this is pending the more
> comprehensive fix for typed arrays post-pwn2own)

(Oops, seems like I got this one mixed up and confused with another one) :-/
Yeah, this is 100% GGC.
Comment on attachment 8409762 [details] [diff] [review]
give_true_answers_in_IATBF_during_minorgc-v0.diff

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

It seems like we ought to have a (more robust) test for this. Is there a way to force something to be pretenured from JS, without forcing a collection? Then at least we could get the situation where some views are tenured and some aren't.

I suppose we could add something to the shell similar to OOMAfterAllocations. TenureNthObject?
Attachment #8409762 - Flags: review?(sphink) → review+
A checkin-ready copy.
Attachment #8409762 - Attachment is obsolete: true
Attachment #8409874 - Flags: review+
Keywords: checkin-needed
(In reply to Terrence Cole [:terrence] from comment #9)
> Yeah, this one was totally my fault. Thankfully only 31 is affected, so lets
> get this in asap.

Setting flags.
Nightly-only, so landing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/988725fe5c1c
Keywords: checkin-needed
Target Milestone: --- → mozilla31
landed on central - https://hg.mozilla.org/mozilla-central/rev/988725fe5c1c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Group: javascript-core-security
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.