Closed
Bug 994281
Opened 10 years ago
Closed 10 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)
Tracking
()
RESOLVED
FIXED
mozilla31
Tracking | Status | |
---|---|---|
firefox28 | --- | unaffected |
firefox29 | --- | unaffected |
firefox30 | --- | unaffected |
firefox31 | --- | fixed |
firefox-esr24 | --- | unaffected |
People
(Reporter: gkw, Assigned: terrence)
Details
(4 keywords)
Attachments
(4 files, 1 obsolete file)
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)
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
This less-reduced testcase asserts at: Assertion failure: arrayByteOffset <= bufferByteLength, at vm/TypedArrayObject.cpp
Reporter | ||
Updated•10 years ago
|
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
Reporter | ||
Updated•10 years ago
|
status-firefox31:
--- → affected
Reporter | ||
Comment 3•10 years ago
|
||
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
Reporter | ||
Comment 5•10 years ago
|
||
(I think Waldo mentioned somewhere that this is pending the more comprehensive fix for typed arrays post-pwn2own)
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Updated•10 years ago
|
Assignee: nobody → sphink
Assignee | ||
Comment 9•10 years ago
|
||
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)
Reporter | ||
Comment 10•10 years ago
|
||
(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) :-/
Assignee | ||
Comment 11•10 years ago
|
||
Yeah, this is 100% GGC.
Comment 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
A checkin-ready copy.
Attachment #8409762 -
Attachment is obsolete: true
Attachment #8409874 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 14•10 years ago
|
||
(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.
status-firefox28:
--- → unaffected
status-firefox29:
--- → unaffected
status-firefox30:
--- → unaffected
status-firefox-esr24:
--- → unaffected
Reporter | ||
Comment 15•10 years ago
|
||
Nightly-only, so landing: https://hg.mozilla.org/integration/mozilla-inbound/rev/988725fe5c1c
Keywords: checkin-needed
Target Milestone: --- → mozilla31
Comment 16•10 years ago
|
||
landed on central - https://hg.mozilla.org/mozilla-central/rev/988725fe5c1c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Updated•10 years ago
|
Group: javascript-core-security
Updated•9 years ago
|
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•