1302682 - Crash [@ __memcpy_sse2_unaligned] with TypedArray

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

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.

Comment 2

[Andrew McCreight [:mccr8]]

Could you look at this, Jon? Thanks.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: 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?

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8794291 [details] [diff] [review]
bug1302682-eval-new-target

Ah, wrong bug.

Comment 5

[Jon Coppeard (:jonco)]

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.

Comment 6

[Jon Coppeard (:jonco)]

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

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1302682-assertion

Not a fix, but a patch to add an assertion to help catch issues like this.  (Th testcase above requires this patch).

Comment 8

[Jon Coppeard (:jonco)]

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?

Comment 9

[Jan de Mooij [:jandem]]

smvv, any thoughts on this? If not I can take a look.

Comment 10

[Sander Mathijs van Veen]

Attachment: 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

Comment 11

[Sander Mathijs van Veen]

Attachment: 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

Comment 12

[Jon Coppeard (:jonco)]

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.

Comment 13

[Sander Mathijs van Veen]

Attachment: 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

Comment 14

[Sander Mathijs van Veen]

Attachment: 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.

Comment 15

[Marcia Knous [:marcia]]

Tracking 52+ for this sec high crash.

Comment 16

[Jon Coppeard (:jonco)]

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.

Comment 17

[Emanuel Hoogeveen [:ehoogeveen]]

This patch also seems to fix the crashes I'm seeing running Octane locally on Windows \o/

Comment 18

[Steve Fink [:sfink] [:s:]]

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.

Comment 19

[Sander Mathijs van Veen]

(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.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7bb6f6235cd

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Emanuel Hoogeveen [:ehoogeveen]]

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.

Comment 23

[Sander Mathijs van Veen]

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.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We should get an aurora nominated patch too so we can avoid shipping the original bug.

Comment 26

[Jan de Mooij [:jandem]]

Can we get this landed?

Comment 27

[Sander Mathijs van Veen]

Attachment: 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?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This got backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/94812daa91cfbfe35c2f077157a39fcac6a98bbb for failing it's own test.
https://treeherder.mozilla.org/logviewer.html#?job_id=37397871&repo=mozilla-inbound

Comment 30

[Sander Mathijs van Veen]

Attachment: 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

Comment 31

[Jon Coppeard (:jonco)]

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.

Comment 32

[Sander Mathijs van Veen]

Test results are good. Two unrelated timeouts, and two issues that were gone when the job was retriggered.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e813ac799ffea42936de59caf1991eb99d514990

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e813ac799ffe

Comment 35

[Gerry Chang [:gchang]]

Hi Sander,
Since this bug is a regression and also affects 51, is this worth uplifting to 51?

Comment 36

[Sander Mathijs van Veen]

Based on comment 23 and comment 24, I think it is worth to uplift it.

Comment 37

[Gerry Chang [:gchang]]

Hi Sander,
Can you help create an uplift request for 51?

Comment 38

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 39

[Sander Mathijs van Veen]

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.

Comment 40

[Gerry Chang [:gchang]]

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

Fix a sec-high. Take it in 51 aurora.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/74ddf7de7dc9

Comment 42

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx51