1292858 - Assertion failure: (detail::IsInBounds<From, To>(aFrom)), at dist/include/mozilla/Casting.h:237

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

z = Object.defineProperty(this, "y", {
    get: function() {
        return new Uint8ClampedArray(x >>= x);
    }
});
y[5];
z[--x];
try {
    y
} catch (e) {}
y.t;
var x = 0;


Backtrace:

0   js-dbg-64-dm-clang-darwin-6b65dd49d4f0	0x00000001072e2b6e (anonymous namespace)::TypedArrayObjectTemplate<js::uint8_clamped>::initTypedArraySlots(JSContext*, js::TypedArrayObject*, unsigned int, void*, js::gc::AllocKind) + 622 (Casting.h:237)
1   js-dbg-64-dm-clang-darwin-6b65dd49d4f0	0x00000001072bbafa js::TypedArrayCreateWithTemplate(JSContext*, JS::Handle<JSObject*>, int) + 4362 (TypedArrayObject.cpp:660)
2   ???                           	0x0000000108caa337 0 + 4442465079
3   js-dbg-64-dm-clang-darwin-6b65dd49d4f0	0x0000000106c42f67 js::jit::IonCannon(JSContext*, js::RunState&) + 871 (Ion.cpp:2822)
4   js-dbg-64-dm-clang-darwin-6b65dd49d4f0	0x000000010719d62a js::RunScript(JSContext*, js::RunState&) + 410 (Interpreter.cpp:379)
5   js-dbg-64-dm-clang-darwin-6b65dd49d4f0	0x00000001071af2fd js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 541 (Interpreter.cpp:471)
/snip

For detailed crash information, see attachment.

Setting s-s by default because this involves TypedArrays pending further investigation.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/cb1f1638d126
user:        Sander Mathijs van Veen
date:        Thu Aug 04 07:42:00 2016 +0200
summary:     Bug 1279992 - Inline constructor of typed arrays with non-compile-time known size r=jandem,Waldo

Jan/Waldo, is bug 1279992 a likely regressor?

Comment 3

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

This was originally: Assertion failure: nbytes > 0, at js/src/gc/Nursery.cpp:304 and probably morphed after reduction.

Stack:

#0  js::Nursery::allocateBuffer (this=0xf7134344, zone=0xf7154800, nbytes=0) at /home/ubuntu/trees/mozilla-central/js/src/gc/Nursery.cpp:304
#1  0x083a1a89 in AllocateObjectBufferWithInit (cx=0xf7134000, obj=0xf5f004b0, count=4294967295) at /home/ubuntu/trees/mozilla-central/js/src/jit/MacroAssembler.cpp:1066
#2  0xf7398806 in ?? ()
Backtrace stopped: Cannot access memory at address 0xffffff85

Comment 4

[Sander Mathijs van Veen]

Attachment: fix-typed-array-is-in-bounds-assertion-failure.patch

The problem is that there is no check for negative numbers when the typed array size is known at runtime. I thought that this was covert by the overflow check, but apparently it is not . This happens in the C++ slow path as well as in the jit/MacroAssembler.cpp's AllocateObjectBufferWithInit. See the following simplified test case for the bug, and the attached patch for a fix. I verified that the patch fixes the test case from the fuzzer as well.

smvv@multivac ~/work/leaningtech/gecko-hg/js/src $ cat test-is-in-bounds.js
var buf = null;
for (var i = 0; i < 10; i++) {
    var a = i == 7 ? -1 : 300;
    try {
        buf = new Uint8ClampedArray(a);
    } catch (e) { }
}
print(buf.length);
smvv@multivac ~/work/leaningtech/gecko-hg/js/src $ make -sj8 -C build_debug.obj/ && build_debug.obj/js/src/js --fuzzing-safe --no-threads --ion-eager test-is-in-bounds.js ; echo $?
Assertion failure: (detail::IsInBounds<From, To>(aFrom)), at /home/smvv/work/leaningtech/gecko-hg/js/src/build_debug.obj/dist/include/mozilla/Casting.h:237
Segmentation fault
139
smvv@multivac ~/work/leaningtech/gecko-hg/js/src $ hg qpush fix-typed-array-is-in-bounds-assertion-failure.patch
applying fix-typed-array-is-in-bounds-assertion-failure.patch
now at: fix-typed-array-is-in-bounds-assertion-failure.patch
smvv@multivac ~/work/leaningtech/gecko-hg/js/src $ make -sj8 -C build_debug.obj/ && build_debug.obj/js/src/js --fuzzing-safe --no-threads --ion-eager test-is-in-bounds.js ; echo $?
300
0

Comment 5

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07957e623587

Comment 6

[Sander Mathijs van Veen]

Try builds succeeded for the patch with "try -b do -p linux,linux64,linux64-asan,linux64-st-an,linux64-valgrind,linux64-shell-haz -u jsreftest,jittests,jittest-1,jittest-2,mochitests,mochitests-bc -t none:". See link above for the results.

Comment 7

[Sander Mathijs van Veen]

Attachment: fix-typed-array-is-in-bounds-assertion-failure.patch

Add a test case that triggers the failures, and throw a type error when negative length is used for typed arrays.

Comment 8

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=214c98914b61

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8778834 [details] [diff] [review]
fix-typed-array-is-in-bounds-assertion-failure.patch

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

::: js/src/vm/TypedArrayObject.cpp
@@ +633,3 @@
>          size_t nbytes;
>          if (!js::CalculateAllocSize<NativeType>(len, &nbytes))
>              return nullptr;

Shouldn't we throw an exception here as well?

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8778834 [details] [diff] [review]
fix-typed-array-is-in-bounds-assertion-failure.patch

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

::: js/src/jit-test/tests/basic/bug1292858.js
@@ +1,5 @@
> +var caughtInvalidArguments = false;
> +var a = -1
> +try {
> +    var buf = new Uint8ClampedArray(a);
> +    assertEq(buf.len, 300);

I tend to prefer

  throw new Error("didn't throw");

and

  assertEq(e instanceof TypeError, true,
           "expected TypeError, instead threw: " + e);

myself.  Especially to a length-assert to a value that's wrong, but it's not obvious looking at that narrow line alone that that particular value has no meaning.

@@ +11,5 @@
> +assertEq(caughtInvalidArguments, true);
> +
> +var caughtInvalidArguments = false;
> +for (var i = 0; i < 2000; i++) {
> +    var a = i == 1900 ? -1 : 300;

If the goal is to wait until you're in Ion code, I would prefer

  var i = 0;
  while (true) {
    i = (i + 1) | 0;

    var a = inIon() ? -1 : 300;
    ...
  }

so that you're looping as long as necessary and no longer, without any magic numbers like 1900 and 2000 in the code, that are more heuristic than anything.

@@ +25,5 @@
> +assertEq(caughtInvalidArguments, true);
> +
> +var caughtInvalidArguments = false;
> +for (var i = 0; i < 2000; i++) {
> +    var a = i == 1900 ? -1 : 0;

Same sort of treatments as mentioned above, here.

Comment 11

[Sander Mathijs van Veen]

Attachment: fix-typed-array-is-in-bounds-assertion-failure.patch

Changed patch according to Jandem's and Waldo's suggestions. From now on, I'll use inIon() instead of the heuristics. I was not aware of that function and it makes much more sense.

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

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

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

Comment 14

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.