Closed Bug 1263803 Opened 6 years ago Closed 6 years ago

Assertion failure: cloneLength % BYTES_PER_ELEMENT == 0, at js/src/vm/TypedArrayObject.cpp:850


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox47 --- unaffected
firefox48 --- verified


(Reporter: gkw, Assigned: arai)



(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update])


(2 files)

The following testcase crashes on mozilla-central revision 29d5a4175c8b (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

x = new ArrayBuffer(38);
y = new Float64Array(x, 24, x);
new Float64Array(y);


0   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001008f8761 (anonymous namespace)::TypedArrayObjectTemplate<double>::fromTypedArray(JSContext*, JS::Handle<JSObject*>, bool, JS::Handle<JSObject*>) + 2337 (TypedArrayObject.cpp:850)
1   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001008df472 (anonymous namespace)::TypedArrayObjectTemplate<double>::class_constructor(JSContext*, unsigned int, JS::Value*) + 690 (TypedArrayObject.cpp:482)
2   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007d617e js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 222 (jscntxtinlines.h:236)
3   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007e03c6 js::CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 118 (jscntxtinlines.h:268)
4   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007c74d7 InternalConstruct(JSContext*, js::AnyConstructArgs const&) + 487 (Interpreter.cpp:553)
5   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007c70f7 js::ConstructFromStack(JSContext*, JS::CallArgs const&) + 87 (Interpreter.cpp:592)
6   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007bbdc4 Interpret(JSContext*, js::RunState&) + 48212 (Interpreter.cpp:2799)
7   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007b00a7 js::RunScript(JSContext*, js::RunState&) + 519 (Interpreter.cpp:426)
8   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007c8044 js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) + 1124 (Interpreter.cpp:682)
9   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007c83c5 js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) + 469 (RootingAPI.h:667)
10  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x0000000100595241 ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) + 417 (jsapi.cpp:4372)
11  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001005954b2 JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) + 82 (RootingAPI.h:667)
12  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x0000000100020aa9 Process(JSContext*, char const*, bool, FileKind) + 3609 (js.cpp:530)
13  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x000000010000610b main + 11739 (js.cpp:6732)
14  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x0000000100001374 start + 52

Setting s-s because this involves TypedArrays.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
user:        Tooru Fujisawa
date:        Sat Mar 05 18:57:51 2016 +0900
summary:     Bug 1165053 - Part 7: Call SpeciesConstructor in TypedArray ctors. r=lth

arai-san, is bug 1165053 a likely regressor? (This landed on April 7, 2016 fwiw)
Blocks: 1165053
Flags: needinfo?(arai.unmht)
Looks like, the issue is that we always allocate ArrayBuffer with |BYTES_PER_ELEMENT * typedArray.length|, sometimes lazily,
but in CloneArrayBuffer, it should allocate ArrayBuffer that doesn't match to |BYTES_PER_ELEMENT * n| length (in the example's case, it should allocate 14 bytes buffer)

Also, we cannot allocate ArrayBuffer lazily when the length of the ArrayBuffer is not |BYTES_PER_ELEMENT * typedArray.length|.

In the example's case, typedArray.length is 0, but the ArrayBuffer's byteLength should be 14.
Flags: needinfo?(arai.unmht)
Sorry, I always forgot to say, yes, it's a regression from bug 1165053.

Then, the issue related to the assertion could be fixed by passing byteLength instead of elementLength to AllocateArrayBuffer,
but it won't fix the issue related to lazily allocated ArrayBuffer.  maybe we should fix it in separated bug tho.
Changing AllocateArrayBuffer and maybeCreateArrayBuffer to receive byteLength fixes the issue related to byte length that is not |BYTES_PER_ELEMENT*n|.

Then in the testcase |a2.buffer.byteLength| becomes 0 because the ArrayBuffer gets created lazily with |a2.byteLength| that is 0.
The issue doesn't happen when I use ArrayBuffer subclass, so I added subclassed case as a testcase for this bug.

Is there any bug filed for lazily allocated ArrayBuffer's length?
Assignee: nobody → arai.unmht
Attachment #8740260 - Flags: review?(lhansen)
Triage is looking at this from a security point of view. What are the implications of this? A 14 byte write?

We're trying to come up with a security rating for this issue.
Flags: needinfo?(arai.unmht)
With this bug, ArrayBuffer object can be created with less capacity than requested length, if the passed buffer is a subclass of ArrayBuffer.  This has no effect when the ArrayBuffer is not subclass, as the wrong value calculated there is discarded

>     MOZ_ASSERT(cloneLength % BYTES_PER_ELEMENT == 0);
>     if (!AllocateArrayBuffer(cx, cloneCtor, cloneLength / BYTES_PER_ELEMENT, buffer))
>         return false;

>         if (!nonDefaultProto && nelements <= INLINE_BUFFER_LIMIT / sizeof(NativeType)) {
>             // The array's data can be inline, and the buffer created lazily.
>             return true;
>         }
> ...
>         ArrayBufferObject* buf = ArrayBufferObject::create(cx, nelements * sizeof(NativeType),
>                                                            nonDefaultProto);
>         if (!buf)
>             return false;

That area cannot be accessed from the TypedArray instance, so I cannot think of any way to exploit this.

For example, in a similar code to the comment #0:

  x = new ArrayBuffer(38);
  y = new Float64Array(x, 24, 2);
  z = new Float64Array(y);

|y| is a 2 element array, and can see following range:

                       y.byteOffset     y.byteOffset+y.byteLength
                       |                |
                       v                v
  x: 01234567 89012345 67890123 45678901 234567

|z| is also a 2 element array, and can see following range:

            z.byteOffset     z.byteOffset+z.byteLength
            |                |
            v                v
  z.buffer: 67890123 45678901 234567

The trailing "234567" there is the area that is not created because of this bug.
The area cannot be accessed, as it needs to create |y| with 3 elements, but it's out of bound and throws TypeError.

>         if (arrayByteLength + byteOffset > buffer->byteLength()) {
>             JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_TYPED_ARRAY_BAD_ARGS);
>             return nullptr; // byteOffset + len is too big for the arraybuffer
>         }

So, I think this matches to sec-other (Bugs that may not be exploitable security issues).
Flags: needinfo?(arai.unmht)
Less mysterious test case that provokes the same crash:

x = new ArrayBuffer(38);
y = new Float64Array(x, 24, 0);
z = new Float64Array(y);
Comment on attachment 8740260 [details] [diff] [review]
Change AllocateArrayBuffer to receive byteLength instead of nelements.

Review of attachment 8740260 [details] [diff] [review]:

::: js/src/tests/ecma_6/ArrayBuffer/indivisible-byteLength.js
@@ +11,5 @@
> +assertEq(a1.byteLength, 0);
> +assertEq(a1.byteOffset, 24);
> +
> +var a2 = new Float64Array(a1);
> +// FIXME bug XXX: ArrayBuffer is created lazily with wrong byteLength.

Would be good to replace XXX by an actual bug number here.  Unless that bug is more exploitable than this one.
Attachment #8740260 - Flags: review?(lhansen) → review+
Thank you for reviewing :D

(In reply to Lars T Hansen [:lth] from comment #8)
> > +// FIXME bug XXX: ArrayBuffer is created lazily with wrong byteLength.
> Would be good to replace XXX by an actual bug number here.  Unless that bug
> is more exploitable than this one.

Sorry, I forgot to note about that.
Do you know any existing bug related to that behavior?
otherwise I'll file it.
Flags: needinfo?(lhansen)
Not aware of a bug regarding that.
Flags: needinfo?(lhansen)

Filed bug 1264941, and it turned out that current behavior is better.
So I'll change the comment in the testcase, and enable the assertion for the buffer.byteLength==0,
so that the behavior change in bug 1264941 will hit either assertion.
Sorry, it hits following error:
> 740 INFO TEST-UNEXPECTED-FAIL | dom/canvas/test/webgl-conformance/_wrappers/test_conformance__typedarrays__array-unit-tests.html | Construction of huge Int16Array should throw exception

it's because of uint32_t overflow.
I should check the range *before* multiply BYTES_PER_ELEMENT.

will fix the patch.
Moved the INT32_MAX check before `* BYTES_PER_ELEMENT`.
Attachment #8741810 - Flags: review?(lhansen)
Comment on attachment 8741810 [details] [diff] [review]
followup - Check byteLength before multiplication.

Review of attachment 8741810 [details] [diff] [review]:

OK, these are clearly correct in the sense that they check invariants that must hold.  I can't quite tell if they're redundant but I'll trust you that they are not, and in any case, it does not matter if they are.
Attachment #8741810 - Flags: review?(lhansen) → review+
Bug 1263803 - Change AllocateArrayBuffer to receive byteLength instead of nelements. r=lth
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
JSBugMon: This bug has been automatically verified fixed.
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.