Closed Bug 1199578 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
firefox40 --- disabled
firefox41 --- disabled
firefox42 --- disabled
firefox43 --- verified
firefox45 --- verified
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: gkw, Assigned: lth)

References

Details

(5 keywords, Whiteboard: [jsbugmon:update][b2g-adv-main2.5-][adv-main45-])

Attachments

(3 files)

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.
Flags: needinfo?(lhansen)
Attached file 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)
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.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Flags: needinfo?(lhansen)
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.
That code has been unchanged for quite a while:

changeset:   206755:bcf65e0b6da6
user:        Jeff Walden <jwalden@mit.edu>
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.
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.
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.
Attachment #8654061 - Flags: review?(jwalden+bmo)
Keywords: sec-high
From comment 6, it sounds like a possible buffer overflow, so I'm going to set it to high.
Keywords: csectype-bounds
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.
Attachment #8654061 - Flags: review?(jwalden+bmo) → review+
Group: core-security → javascript-core-security
I'll prep a test case too.
Keywords: leave-open
Attached patch Test caseSplinter Review
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).
Attachment #8655856 - Flags: review?(jwalden+bmo)
https://hg.mozilla.org/mozilla-central/rev/62efa6d695d0

We don't typically leave bugs open for tests, fwiw.
Flags: in-testsuite?
Keywords: leave-open
Target Milestone: --- → mozilla43
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.
Attachment #8655856 - Flags: review?(jwalden+bmo) → review+
Test case landed on m-i yesterday: changeset: 261162:0244b29cc5f3
https://hg.mozilla.org/mozilla-central/rev/0244b29cc5f3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Group: javascript-core-security → core-security-release
Group: core-security-release
Whiteboard: [jsbugmon:update] → [jsbugmon:update][b2g-adv-main2.5-]
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx43
Whiteboard: [jsbugmon:update][b2g-adv-main2.5-] → [jsbugmon:update][b2g-adv-main2.5-][adv-main45-]
You need to log in before you can comment on or make changes to this bug.