Crash [@ __memcpy_sse2_unaligned] with TypedArray

VERIFIED FIXED in Firefox 51

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: decoder, Assigned: Sander Mathijs van Veen)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla52
x86_64
Linux
crash, jsbugmon, regression, sec-high, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 unaffected, firefox-esr45 unaffected, firefox50 unaffected, firefox51+ verified, firefox52+ verified)

Details

(Whiteboard: [jsbugmon:ignore], crash signature)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

a year ago
The following testcase crashes on mozilla-central revision cfdb7af3af2e (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --ion-eager):

for (var i = 0; i < 30000; i++) {
    var a = inIon() ? 7 : 300;
    var buf = new Uint8ClampedArray(a);
    (function() {}) * this;
    try {} catch (e) {}
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
__memcpy_sse2_unaligned () at ../sysdeps/x86_64/multiarch/memcpy-sse2-unaligned.S:38
#0  __memcpy_sse2_unaligned () at ../sysdeps/x86_64/multiarch/memcpy-sse2-unaligned.S:38
#1  0x0000000000d17ec5 in js::TenuringTracer::moveObjectToTenured (this=this@entry=0x7fffffffc5e0, dst=dst@entry=0x7ffff36851c0, src=src@entry=0x7ffff37fffa8, dstKind=dstKind@entry=js::gc::AllocKind::OBJECT8_BACKGROUND) at js/src/gc/Marking.cpp:2504
#2  0x0000000000d1880f in js::TenuringTracer::moveToTenured (this=this@entry=0x7fffffffc5e0, src=0x7ffff37fffa8) at js/src/gc/Marking.cpp:2406
#3  0x0000000000d18abc in js::TenuringTracer::traverse<JSObject> (this=0x7fffffffc5e0, objp=0x7fffffffc370) at js/src/gc/Marking.cpp:2226
#4  0x0000000000d32be8 in js::TenuringTraversalFunctor<JS::Value>::operator()<JSObject> (this=<synthetic pointer>, trc=0x7fffffffc5e0, t=0x7ffff37fffa8) at js/src/gc/Marking.cpp:2232
#5  js::DispatchTyped<js::TenuringTraversalFunctor<JS::Value>, js::TenuringTracer*>(js::TenuringTraversalFunctor<JS::Value>, JS::Value const&, js::TenuringTracer*&&) (f=..., val=...) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:1913
#6  0x0000000000d1a551 in js::TenuringTracer::traverse<JS::Value> (thingp=0x7ffff332ba18, this=0x7fffffffc5e0) at js/src/gc/Marking.cpp:2241
#7  js::TenuringTracer::traceSlots (end=<optimized out>, vp=0x7ffff332ba18, this=0x7fffffffc5e0) at js/src/gc/Marking.cpp:2482
#8  js::TenuringTracer::traceObjectSlots (this=this@entry=0x7fffffffc5e0, nobj=nobj@entry=0x7ffff3679060, start=start@entry=339, length=<optimized out>) at js/src/gc/Marking.cpp:2475
#9  0x0000000000d1a73a in js::gc::StoreBuffer::SlotsEdge::trace (this=this@entry=0x7ffff69e75b0, mover=...) at js/src/gc/Marking.cpp:2290
#10 0x0000000000d32d82 in js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::SlotsEdge>::trace (this=this@entry=0x7ffff695fdf8, owner=owner@entry=0x7ffff695fd48, mover=...) at js/src/gc/Marking.cpp:2254
#11 0x0000000000d1c67a in js::gc::StoreBuffer::traceSlots (mover=..., this=0x7ffff695fd48) at js/src/gc/StoreBuffer.h:423
#12 js::Nursery::doCollection (this=this@entry=0x7ffff695f9b0, rt=rt@entry=0x7ffff695f208, reason=reason@entry=JS::gcreason::OUT_OF_NURSERY, tenureCounts=...) at js/src/gc/Nursery.cpp:666
#13 0x0000000000d1d09f in js::Nursery::collect (this=this@entry=0x7ffff695f9b0, rt=0x7ffff695f208, reason=reason@entry=JS::gcreason::OUT_OF_NURSERY) at js/src/gc/Nursery.cpp:574
#14 0x0000000000903ddf in js::gc::GCRuntime::minorGC (this=this@entry=0x7ffff695f958, reason=reason@entry=JS::gcreason::OUT_OF_NURSERY, phase=phase@entry=js::gcstats::PHASE_MINOR_GC) at js/src/jsgc.cpp:6502
#15 0x0000000000d076e1 in js::gc::GCRuntime::tryNewNurseryObject<(js::AllowGC)1> (this=this@entry=0x7ffff695f958, cx=cx@entry=0x7ffff695f000, thingSize=thingSize@entry=64, nDynamicSlots=nDynamicSlots@entry=0, clasp=clasp@entry=0x1d7cb60 <JSFunction::class_>) at js/src/gc/Allocator.cpp:87
#16 0x0000000000d08c48 in js::Allocate<JSObject, (js::AllowGC)1> (cx=cx@entry=0x7ffff695f000, kind=kind@entry=js::gc::AllocKind::FIRST, nDynamicSlots=0, heap=heap@entry=js::gc::DefaultHeap, clasp=clasp@entry=0x1d7cb60 <JSFunction::class_>) at js/src/gc/Allocator.cpp:51
#17 0x000000000093be6e in JSObject::create (cx=0x7ffff695f000, kind=js::gc::AllocKind::FIRST, heap=js::gc::DefaultHeap, shape=..., group=...) at js/src/jsobjinlines.h:377
#18 0x0000000000956a48 in NewObject (cx=0x7ffff695f000, group=..., kind=js::gc::AllocKind::FIRST, newKind=js::GenericObject, initialShapeFlags=<optimized out>) at js/src/jsobj.cpp:667
#19 0x0000000000957466 in js::NewObjectWithClassProtoCommon (cx=cx@entry=0x7ffff695f000, clasp=clasp@entry=0x1d7cb60 <JSFunction::class_>, protoArg=..., protoArg@entry=..., allocKind=allocKind@entry=js::gc::AllocKind::FIRST, newKind=newKind@entry=js::GenericObject) at js/src/jsobj.cpp:787
#20 0x00000000009276a6 in js::NewObjectWithClassProto (newKind=js::GenericObject, allocKind=js::gc::AllocKind::FIRST, proto=..., clasp=0x1d7cb60 <JSFunction::class_>, cx=0x7ffff695f000) at js/src/jsobjinlines.h:726
#21 NewFunctionClone (cx=cx@entry=0x7ffff695f000, fun=..., fun@entry=..., newKind=newKind@entry=js::GenericObject, allocKind=allocKind@entry=js::gc::AllocKind::FIRST, proto=..., proto@entry=...) at js/src/jsfun.cpp:2062
#22 0x0000000000931ade in js::CloneFunctionReuseScript (cx=cx@entry=0x7ffff695f000, fun=fun@entry=..., enclosingEnv=enclosingEnv@entry=..., allocKind=allocKind@entry=js::gc::AllocKind::FIRST, newKind=newKind@entry=js::GenericObject, proto=proto@entry=...) at js/src/jsfun.cpp:2097
#23 0x0000000000b02cd3 in js::CloneFunctionObjectIfNotSingleton (cx=cx@entry=0x7ffff695f000, fun=..., fun@entry=..., parent=..., proto=..., proto@entry=..., newKind=newKind@entry=js::GenericObject) at js/src/jsfuninlines.h:89
#24 0x0000000000ac62c6 in js::Lambda (cx=0x7ffff695f000, fun=..., parent=...) at js/src/vm/Interpreter.cpp:4301
#25 0x00007ffff7e45d50 in ?? ()
#26 0x0000000000000000 in ?? ()
rax	0x17ad88	1551752
rbx	0x7ffff37fffa8	140737278640040
rcx	0xc0	192
rdx	0x60	96
rsi	0x7ffff37fffa8	140737278640040
rdi	0x7ffff36851c0	140737277088192
rbp	0x7fffffffc2b0	140737488339632
rsp	0x7fffffffc1e8	140737488339432
r8	0x7ffff3685220	140737277088288
r9	0x6	6
r10	0x0	0
r11	0x7ffff3685220	140737277088288
r12	0x7ffff36851c0	140737277088192
r13	0x60	96
r14	0x7ffff36851c0	140737277088192
r15	0x9	9
rip	0x7ffff6bd0e1e <__memcpy_sse2_unaligned+46>
=> 0x7ffff6bd0e1e <__memcpy_sse2_unaligned+46>:	movdqu -0x10(%rsi,%rdx,1),%xmm8
   0x7ffff6bd0e25 <__memcpy_sse2_unaligned+53>:	movdqu %xmm8,-0x10(%rdi,%rdx,1)


This crash is intermittent and only reproduces about 10% of the time. Marking s-s due to crash address and GC involved.
(Reporter)

Comment 1

a year ago
Jan, can you try to find someone to work on this? I would have needinfo'ed Jon but he's on PTO. We won't get a bisect for this.
Keywords: sec-high
Could you look at this, Jon? Thanks.
Flags: needinfo?(jcoppeard)
Created attachment 8794291 [details] [diff] [review]
bug1302682-eval-new-target

The problem seems to be that the baseline frame can be traced by a GC before the eval new target value is pushed.

This patch fixes the test failure (which only reproduces at certain revisions presumably because it depends on the stack contents).

However the code for this has changed quite a lot since.  Does this sound like it's something that's still an issue?
Flags: needinfo?(jcoppeard)
Attachment #8794291 - Flags: feedback?(jdemooij)
Comment on attachment 8794291 [details] [diff] [review]
bug1302682-eval-new-target

Ah, wrong bug.
Attachment #8794291 - Attachment is obsolete: true
Attachment #8794291 - Flags: feedback?(jdemooij)
This one is because we're copying past the end of the nursery when tenuring a Uint8ClampedArray.  The size calculations seem to have gone awry.
Here's a better testcase that reproduces reliably on tip:

gc();
var x = [];
for (var i = 0; i < 20000; i++) {
    var a = inIon() ? 7 : 300;
    var buf = new Uint8ClampedArray(a);
    x.push(buf);
}

Run with: --ion-eager --ion-offthread-compile=off --nursery-size=1
Created attachment 8794818 [details] [diff] [review]
bug1302682-assertion

Not a fix, but a patch to add an assertion to help catch issues like this.  (Th testcase above requires this patch).
Attachment #8794818 - Flags: review?(sphink)
Keywords: leave-open
The problem seems to be that CodeGenerator::visitNewTypedArrayDynamicLength uses the alloc kind of the template object regardless of the length requested, which could be wrong.  TypedArrayObjectTemplate::makeTypedArrayWithTemplate calculates the alloc kind every time.

I'm not sure of the best approach here.  Do we need to calculate the alloc kind in the JIT code too?
Blocks: 1279992
Flags: needinfo?(jdemooij)
smvv, any thoughts on this? If not I can take a look.
Flags: needinfo?(jdemooij) → needinfo?(sandervv)
(Assignee)

Comment 10

a year ago
Created attachment 8794891 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

The new alloc kind did not match the alloc kind of the old object. That caused problems when memcpy()ing the old JSObject data to the new JSObject. This patch calculates the correct alloc kind based on whether the old object has inline data and given that it has, it will use the byteLength to determine the right alloc kind. If there is no inline data, use the standard alloc kind for heap allocated typed arrays.

I've added two assertions to make sure that typed arrays with inline data remain typed arrays with inline data, or when heap allocated they remain heap allocated. Note that it is possible to allocate a buffer directly under the JSObject for the typed array elements. Then, hasInlineElements will return true, which is correct and wrong. It will not cause any problems because allocKindForTenure will also check if the old object's byteLength fits in the byteLength.

Try job results are pending: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4338ee611505
Assignee: nobody → sandervv
Flags: needinfo?(sandervv)
Attachment #8794891 - Flags: review?(jcoppeard)
(Assignee)

Comment 11

a year ago
Created attachment 8795181 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

Removed the second assertion because that one is always true.

New try build results are pending: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f74a1e0905ab
Attachment #8794891 - Attachment is obsolete: true
Attachment #8794891 - Flags: review?(jcoppeard)
Attachment #8795181 - Flags: review?(jcoppeard)
Comment on attachment 8795181 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

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

Thanks for fixing this, and for the helpful discussion on IRC yesterday.

The changes in JSObject::allocKindForTenure look good to me, but the assertion in TypedArrayObject::objectMovedDuringMinorGC is failing on try.

Cancelling review for now.
Attachment #8795181 - Flags: review?(jcoppeard) → feedback+
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected
status-firefox52: --- → affected
status-firefox-esr45: --- → unaffected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
(Assignee)

Comment 13

a year ago
Created attachment 8796139 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

Removed the failing assertion. Added the test case listed in this bug report.

Try job results are pending: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9070193987e2
Attachment #8795181 - Attachment is obsolete: true
Attachment #8796139 - Flags: review?(jcoppeard)
(Assignee)

Comment 14

a year ago
Created attachment 8796968 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

This patch succeeds on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2724b30a49cb
There are some test failures, but I did not see any related to the patch.

The added assertions are now checking that typed arrays with inline data remain inlined, while typed arrays with heap allocated buffers remain heap allocated, when typed arrays are moved to the tenured heap.
Attachment #8796139 - Attachment is obsolete: true
Attachment #8796139 - Flags: review?(jcoppeard)
Attachment #8796968 - Flags: review?(jcoppeard)
Tracking 52+ for this sec high crash.
tracking-firefox52: ? → +
Comment on attachment 8796968 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

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

Great.  Thank you for adding the extra assertions.

::: js/src/vm/TypedArrayObject.cpp
@@ +238,5 @@
>  bool
>  TypedArrayObject::hasInlineElements() const
>  {
> +    return elements() == this->fixedData(TypedArrayObject::FIXED_DATA_START) &&
> +        byteLength() <= TypedArrayObject::INLINE_BUFFER_LIMIT;

Is the second comparison ever expected to be false if the first is true?  If not we should make it an assertion.

@@ +592,4 @@
>          // Template objects do not need memory for its elements, since there
> +        // won't be any elements to store. Therefore, we set the pointer to
> +        // nullptr and avoid allocating memory that will never be used.
> +        tarray->initPrivate(nullptr);

I kind of feel like we should set this to point to the inline elements if appropriate because we check this fact elsewhere.  But this is probably OK.  And better than setting it to always point inline like it does now.
Attachment #8796968 - Flags: review?(jcoppeard) → review+
This patch also seems to fix the crashes I'm seeing running Octane locally on Windows \o/
Comment on attachment 8794818 [details] [diff] [review]
bug1302682-assertion

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

Somehow missed this review request. r=me if it isn't already done in the other patch.
Attachment #8794818 - Flags: review?(sphink) → review+
(Assignee)

Comment 19

a year ago
(In reply to Jon Coppeard (:jonco) from comment #16)
> ::: js/src/vm/TypedArrayObject.cpp
> @@ +238,5 @@
> >  bool
> >  TypedArrayObject::hasInlineElements() const
> >  {
> > +    return elements() == this->fixedData(TypedArrayObject::FIXED_DATA_START) &&
> > +        byteLength() <= TypedArrayObject::INLINE_BUFFER_LIMIT;
> 
> Is the second comparison ever expected to be false if the first is true?  If
> not we should make it an assertion.

The problem occurs when allocating in the nursery: if the data buffer is allocated on the nursery, it will be right after the JSObject. Without checking if it fits in the inline buffer limit, it is not possible to differentiate between the two cases (inline data or allocated buffer).

> @@ +592,4 @@
> >          // Template objects do not need memory for its elements, since there
> > +        // won't be any elements to store. Therefore, we set the pointer to
> > +        // nullptr and avoid allocating memory that will never be used.
> > +        tarray->initPrivate(nullptr);
> 
> I kind of feel like we should set this to point to the inline elements if
> appropriate because we check this fact elsewhere.  But this is probably OK. 
> And better than setting it to always point inline like it does now.

Setting it to inline data gave problems with the added assertions. For template object with a large size, the byteLength of the data is larger than the inline buffer limit, while the data pointer says that it has inline data. But there is no data at all, and therefore I set it to nullptr.
Should this bug have been closed or is more work still intended to land? Looks like it also needs an Aurora uplift request and should have had sec-approval before landing too :\
https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(sandervv)
I think only the patch from comment #7 (which adds an assertion) has landed. The patch from comment #14 has r+, but has yet to land.
(Assignee)

Comment 23

a year ago
Comment on attachment 8796968 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It leaks memory from the nursery since it copies more data to the tenured typed array object than the typed array object had in the nursery.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I think that the test case gives a clear idea where the problem is.

Which older supported branches are affected by this flaw?
Version 51

If not all supported branches, which bug introduced the flaw?
Version 51

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
They should not be risky and not hard to create. I think the patch can land as-is on those branches.

How likely is this patch to cause regressions; how much testing does it need?
I do not think that this will cause a regression. When the two added assertions fail, there is a clear sign that there is a regression. The added assertion verify that a typed array with inline data remains a typed array with inline data and a typed array with heap allocated memory remains heap allocated. When this check fails, there is a regression.
Flags: needinfo?(sandervv)
Attachment #8796968 - Flags: sec-approval?
sec-approval+ for trunk. We should get an aurora nominated patch too so we can avoid shipping the original bug.
tracking-firefox51: ? → +
Attachment #8796968 - Flags: sec-approval? → sec-approval+
Duplicate of this bug: 1308563
Can we get this landed?
Flags: needinfo?(sandervv)
(Assignee)

Comment 27

a year ago
Created attachment 8799771 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

Rebased the patch on top of aurora.

$ hg update -r FIREFOX_AURORA_51_BASE
$ hg qpush --move bug1302682-crash-typed-array-alloc-kind.patch
$ hg qref

Is this enough or are more steps needed?
Flags: needinfo?(sandervv) → needinfo?(jdemooij)
(Assignee)

Updated

a year ago
Keywords: checkin-needed
Attachment #8796968 - Attachment is obsolete: true
Based on comment 27, it sounds like the patch applied cleanly to Aurora, so we should be able to just graft what landed on m-c.

https://hg.mozilla.org/integration/mozilla-inbound/rev/65b43a0e62b8ab0f6652fd2f8d3486422eb789e3
Flags: needinfo?(jdemooij) → in-testsuite+
Keywords: checkin-needed, leave-open
(Assignee)

Comment 30

a year ago
Created attachment 8800596 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

I somehow missed that the patch with the GC assertion had already landed. This patch fixes the issue. There was a mismatch between the size of the nursery allocated typed array with inline data and the size of a typed array with inline data allocated in the slow path. The one allocated in the nursery is not aligned to the size of the destination AllocKind, but only aligned to sizeof(HeapSlot).

Try job results are pending: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0258956ca1e1b4f9c1a5e96f60ff0af75c90483c
Attachment #8799771 - Attachment is obsolete: true
Flags: needinfo?(sandervv)
Attachment #8800596 - Flags: review?(jcoppeard)
Comment on attachment 8800596 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

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

r=me pending a clean try run.
Attachment #8800596 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 32

a year ago
Test results are good. Two unrelated timeouts, and two issues that were gone when the job was retriggered.
(Assignee)

Updated

a year ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e813ac799ffe
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi Sander,
Since this bug is a regression and also affects 51, is this worth uplifting to 51?
Flags: needinfo?(sandervv)
(Assignee)

Comment 36

a year ago
Based on comment 23 and comment 24, I think it is worth to uplift it.
Flags: needinfo?(sandervv)
Hi Sander,
Can you help create an uplift request for 51?
Flags: needinfo?(sandervv)

Updated

a year ago
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified

Comment 38

a year ago
JSBugMon: This bug has been automatically verified fixed.
(Assignee)

Comment 39

a year ago
Comment on attachment 8800596 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

Approval Request Comment
[Feature/regressing bug #]: 1279992
[User impact if declined]:

Typed arrays with inline data do not necessarily have the same AllocKind between src and dst. The nursery does not allocate an inline data buffer that has the same size as the slow path will do. In the slow path, the Typed Array Object stores the inline data in the allocated space that fits the AllocKind. In the fast path, the nursery will allocate another buffer that is directly behind the minimal JSObject. That buffer size plus the JSObject size is not necessarily as large as the slow path's AllocKind size.

Without this patch, extra GC nursery data is copied.

[Describe test coverage new/current, TreeHerder]:

The added assertions guarantee that the same JSObject is allocated in the fast and slow path. With this patch applied, it is not possible to copy memory that should not have been copied.

Try results of this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0258956ca1e1b4f9c1a5e96f60ff0af75c90483c&group_state=expanded

[Risks and why]: 

I'm not aware of any risks.

[String/UUID change made/needed]: No change needed.
Flags: needinfo?(sandervv)
Attachment #8800596 - Flags: approval-mozilla-aurora?
Comment on attachment 8800596 [details] [diff] [review]
bug1302682-crash-typed-array-alloc-kind.patch

Fix a sec-high. Take it in 51 aurora.
Attachment #8800596 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

a year ago
status-firefox51: fixed → verified

Comment 42

a year ago
JSBugMon: This bug has been automatically verified fixed on Fx51
Group: javascript-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.