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

VERIFIED FIXED in Firefox 51

Status

()

--
critical
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: gkw, Assigned: sandervv)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla51
x86_64
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 unaffected, firefox51 verified)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8778534 [details]
Detailed Crash Information
(Reporter)

Comment 2

2 years ago
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?
Blocks: 1279992
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jdemooij)
(Reporter)

Updated

2 years ago
Summary: Assertion failure: (detail::IsInBounds<From, To>(aFrom)), at /Users/skywalker/shell-cache/js-dbg-64-dm-clang-darwin-6b65dd49d4f0/objdir-js/dist/include/mozilla/Casting.h:237 → Assertion failure: (detail::IsInBounds<From, To>(aFrom)), at dist/include/mozilla/Casting.h:237
(Reporter)

Comment 3

2 years ago
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
(Assignee)

Comment 4

2 years ago
Created attachment 8778537 [details] [diff] [review]
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
Assignee: nobody → sandervv
Attachment #8778537 - Flags: review?(jdemooij)
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Comment 7

2 years ago
Created attachment 8778834 [details] [diff] [review]
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.
Attachment #8778537 - Attachment is obsolete: true
Attachment #8778537 - Flags: review?(jdemooij)
Attachment #8778834 - Flags: review?(jdemooij)
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?
Attachment #8778834 - Flags: review?(jdemooij) → review+

Updated

2 years ago
Flags: needinfo?(jdemooij)
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.

Updated

2 years ago
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 11

2 years ago
Created attachment 8779238 [details] [diff] [review]
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.
Attachment #8778834 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cbdee926460d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified

Comment 14

2 years ago
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
(Reporter)

Updated

2 years ago
Depends on: 1313807
(Reporter)

Updated

2 years ago
No longer depends on: 1313807
status-firefox50: --- → unaffected
status-firefox-esr45: --- → unaffected
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.