1199578 - Assertion failure: AnyTypedArrayLength(source) <= target->length() - offset, at vm/TypedArrayCommon.h

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

x = new SharedInt32Array(4);
x.__proto__ = (function(){});
Uint8ClampedArray(x);

asserts js debug shell on m-c changeset 87e23922be37 with --fuzzing-safe --no-threads --no-ion --no-baseline at Assertion failure: AnyTypedArrayLength(source) <= target->length() - offset, at vm/TypedArrayCommon.h

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 87e23922be37

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/4b1566e6f3d0
user:        Lars T Hansen
date:        Tue Dec 16 04:43:26 2014 +0100
summary:     Bug 1107365 - Make TypedArray's set() method able to handle shared and nonshared.  r=waldo

Lars, is bug 1107365 a likely regressor?

Setting s-s because this involves TypedArrays.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x213e4f, 0x00000001003f4d6b js-dbg-64-dm-nsprBuild-darwin-87e23922be37`js::TypedArrayMethods<js::TypedArrayObject>::setFromAnyTypedArray(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, unsigned int) + 120 at TypedArrayCommon.h:159, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001003f4d6b js-dbg-64-dm-nsprBuild-darwin-87e23922be37`js::TypedArrayMethods<js::TypedArrayObject>::setFromAnyTypedArray(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, unsigned int) + 120 at TypedArrayCommon.h:159
    frame #1: 0x00000001003f4cf3 js-dbg-64-dm-nsprBuild-darwin-87e23922be37`js::TypedArrayMethods<js::TypedArrayObject>::setFromAnyTypedArray(cx=<unavailable>, target=<unavailable>, source=<unavailable>, offset=<unavailable>) + 65763 at TypedArrayCommon.h:757
    frame #2: 0x00000001003e4b5c js-dbg-64-dm-nsprBuild-darwin-87e23922be37`js::TypedArrayMethods<js::TypedArrayObject>::setFromArrayLike(cx=0x000000010284c400, target=<unavailable>, source=<unavailable>, len=<unavailable>, offset=0) + 140 at TypedArrayCommon.h:729
    frame #3: 0x0000000100391bcc js-dbg-64-dm-nsprBuild-darwin-87e23922be37`(anonymous namespace)::TypedArrayObjectTemplate<js::uint8_clamped>::fromArray(cx=0x000000010284c400, other=<unavailable>) + 364 at TypedArrayObject.cpp:677
    frame #4: 0x00000001003971fc js-dbg-64-dm-nsprBuild-darwin-87e23922be37`(anonymous namespace)::TypedArrayObjectTemplate<js::uint8_clamped>::class_constructor(JSContext*, unsigned int, JS::Value*) + 237 at TypedArrayObject.cpp:452
(lldb)

Comment 2

[Lars T Hansen [:lth]]

The surface problem is that setFromArrayLike() does not pass the 'len' argument on to ElementSpecific::setFromAnyTypedArray(), so that ends up using the length of the source, but that's wrong: the length of the source should not be used, and that's the reason why a length argument was being passed in the first place.

At first blush it does not look like bug 1107365 introduced this problem per se, but it did make it possible for SharedTypedArray objects to take this path to the code.  It's probable that there's something deeper in SharedTypedArrays (witness the necessary prototype hack in the TC) that is wrong.

Comment 3

[Lars T Hansen [:lth]]

Arguably this is the immediate problem:

diff --git a/js/src/vm/TypedArrayObject.cpp b/js/src/vm/TypedArrayObject.cpp
--- a/js/src/vm/TypedArrayObject.cpp
+++ b/js/src/vm/TypedArrayObject.cpp
@@ -658,18 +658,18 @@ struct TypedArrayObject::OfType
     typedef TypedArrayObjectTemplate<T> Type;
 };
 
 template<typename T>
 /* static */ JSObject*
 TypedArrayObjectTemplate<T>::fromArray(JSContext* cx, HandleObject other)
 {
     uint32_t len;
-    if (other->is<TypedArrayObject>()) {
-        len = other->as<TypedArrayObject>().length();
+    if (IsAnyTypedArray(other)) {
+        len = AnyTypedArrayLength(other);
     } else if (!GetLengthProperty(cx, other, &len)) {
         return nullptr;
     }
 
     Rooted<ArrayBufferObject*> buffer(cx);
     if (!maybeCreateArrayBuffer(cx, len, &buffer))
         return nullptr;
 
What happens in the existing code is that we fall through to GetLengthProperty(), which, because of the hack with __proto__, returns 0.  Later we use a different path to grab the intrinisic length property, which is 4, and that causes the assertion to fail.

Comment 4

[Lars T Hansen [:lth]]

That code has been unchanged for quite a while:

changeset:   206755:bcf65e0b6da6
user:        Jeff Walden <[e-mail]>
date:        Thu Aug 21 20:39:30 2014 -0700
summary:     Bug 896116 - Implement %TypedArray% and %TypedArray%.prototype.  r=till, r=bholley

So the error is that the code has not been updated (whether here, or deeper) to fit a new reality.

Comment 5

[Lars T Hansen [:lth]]

Finally, as the initial bisection shows, the error appears to have been introduced in bug 1107365 with these two lines (second patch on that bug):

-        MOZ_ASSERT(source->length() <= target->length() - offset);
+        MOZ_ASSERT(AnyTypedArrayLength(source) <= target->length() - offset);

but the actual bug is that the lines quoted above were not changed in that patch.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1199578-typetest.patch

Probably is mildly s-s actually since not enough memory would be allocated for the target buffer, on the other hand, SharedTypedArray is Nightly-only so no special action needed.

Comment 7

[Andrew McCreight [:mccr8]]

From comment 6, it sounds like a possible buffer overflow, so I'm going to set it to high.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8654061 [details] [diff] [review]
bug1199578-typetest.patch

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

This should have a testcase ready to land, after a delay for nightly users to get a fixed build.

Comment 9

[Lars T Hansen [:lth]]

I'll prep a test case too.

Comment 10

[Lars T Hansen [:lth]]

Attachment: Test case

Waldo, want to run your eyes over this?  This is the original 3-line TC (with a guard and a tweak to avoid Uint8ClampedArray, which is an outlier we should avoid when we can).

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62efa6d695d0

We don't typically leave bugs open for tests, fwiw.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8655856 [details] [diff] [review]
Test case

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

::: js/src/jit-test/tests/sharedbuf/ta-set-from-sta-1199578.js
@@ +1,1 @@
> +if (!this.SharedArrayBuffer)

Name the test file typedarray-from-sharedtypedarray-with-overridden-length.js -- more clear than this name.  I don't think the bug number is useful, and I'd remove it.

@@ +3,5 @@
> +
> +// This would hit an assertion in debug builds due to an incorrect
> +// type guard in the code that copies data from STA to TA.
> +
> +x = new SharedInt32Array(4);

Make this var x.

@@ +4,5 @@
> +// This would hit an assertion in debug builds due to an incorrect
> +// type guard in the code that copies data from STA to TA.
> +
> +x = new SharedInt32Array(4);
> +x.__proto__ = (function(){});

Does the test also fail if you instead of this line did

  Object.defineProperty(x, "length", { value: 0 });

?  I think it should, and this makes clearer that it's the unusual-valued length property that's at issue here.

Might also be worth duplicating the effective test here, with a length that's way-large, too:

  var y = new SharedInt32Array(4);
  Object.defineProperty(y, "length", { value: Math.pow(2, 20) });
  new Uint8Array(y);

@@ +5,5 @@
> +// type guard in the code that copies data from STA to TA.
> +
> +x = new SharedInt32Array(4);
> +x.__proto__ = (function(){});
> +Uint8Array(x);

new Uint8Array, please -- we're somewhat-trying to require typed arrays only be constructed, no good adding more bad instances of calling to the tree.

Comment 13

[Lars T Hansen [:lth]]

Test case landed on m-i yesterday: changeset: 261162:0244b29cc5f3

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0244b29cc5f3

Comment 15

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx43