1313807 - Assertion failure: nbytes > 0, at js/src/gc/Nursery.cpp:365

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

function f(x) {
    new Uint16Array(x ^ -1);
}
f(4294967295);
f(2147483648);


Backtrace:

0   js-dbg-32-dm-clang-darwin-1561c917ee27	0x00afa218 js::Nursery::allocateBuffer(JSObject*, unsigned long) + 200 (Nursery.cpp:365)
1   js-dbg-32-dm-clang-darwin-1561c917ee27	0x00439ef8 AllocateObjectBufferWithInit(JSContext*, js::TypedArrayObject*, int) + 312 (MacroAssembler.cpp:1066)
2   ???                           	0x020ca5b3 0 + 34383283
3   js-dbg-32-dm-clang-darwin-1561c917ee27	0x002f23a3 js::jit::IonCannon(JSContext*, js::RunState&) + 819 (Ion.cpp:2846)
4   js-dbg-32-dm-clang-darwin-1561c917ee27	0x0082377d js::RunScript(JSContext*, js::RunState&) + 333 (Interpreter.cpp:384)
/snip

For detailed crash information, see attachment.

Setting s-s because of Uint16Array, as a start.

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/cbdee926460d
user:        Sander Mathijs van Veen
date:        Wed Aug 10 16:58:31 2016 +0200
summary:     Bug 1292858 - Assertion failure: (detail::IsInBounds<From, To>(aFrom)), at dist/include/mozilla/Casting.h:237 .r=jandem

Jan, is bug 1292858 a likely regressor?

Comment 3

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

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160805000432" and the hash "505e6acd9c291504700a57ddf7e88f704f65da46".
The "bad" changeset has the timestamp "20160805000922" and the hash "52a0d2d7639717858ce6868c19a37b95e7039736".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=505e6acd9c291504700a57ddf7e88f704f65da46&tochange=52a0d2d7639717858ce6868c19a37b95e7039736

Actually bug 1279992 might be a more likely regressor...

Comment 4

[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

Comment 5

[Sander Mathijs van Veen]

Code that causes the assertion failure: https://dxr.mozilla.org/mozilla-central/source/js/src/jit/MacroAssembler.cpp#1049-1065

The problem is that the JS_ROUNDUP after CalculateAllocSize overflows on 32-bit. The method AllocateObjectBufferWithInit is called on linux 32 bit with the value count=2147483647.

js::CalculateAllocSize<T>(count, &nbytes) returns: numElems * 2 = 4294967294.
The overflow happens in: nbytes = JS_ROUNDUP(nbytes, sizeof(Value));
>>> p nbytes
$8 = 4294967294
>>> p (nbytes + 4 - 1)
$9 = 1
>>> p (((nbytes + 4 - 1) / 4) * 4)
$10 = 0

Jan, should we add an if-statement that checks if (nbytes + sizeof(Value) == 0) and return when true? That will bailout to the slow path.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Sander Mathijs van Veen from comment #5)
> Jan, should we add an if-statement that checks if (nbytes + sizeof(Value) ==
> 0) and return when true? That will bailout to the slow path.

We could do something like:

  if (!(CheckedInt<size_t>(nbytes) + sizeof(Value)).isValid())
      return ...;

We should also fix JS_ROUNDUP to be saner, with proper checks/asserts (maybe even MOZ_RELEASE_ASSERT as these bugs are really bad). That should be done as a follow-up though.

Comment 7

[Jan de Mooij [:jandem]]

Actually let's use CheckedUint32(nbytes), that way we behave the same on x64 (when nbytes doesn't fit in an uint32_t). The fast path shouldn't handle these cases.

Comment 8

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

Sander, what's next here?

Comment 9

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

Jan, do you think you can take this on? Bug 1314175 is a similar bug and that crashes at a weird memory address with not much on the stack, making it difficult to ignore without ignoring other bugs accidentally.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8809834 [details] [diff] [review]
Patch

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

::: js/src/jit/MacroAssembler.cpp
@@ +1064,5 @@
>          MOZ_CRASH("Unsupported TypedArray type");
>      }
>  
> +    if (!(CheckedUint32(nbytes) + sizeof(Value)).isValid())
> +        return;

Guh.  Fugly to have a standalone check of this sort and then discard the Checked<> immediately without even using the valid value in it.

@@ +1070,1 @@
>      nbytes = JS_ROUNDUP(nbytes, sizeof(Value));

Meta-commentary, but I'd much rather we removed JS_ROUNDUP in favor of an actual C++ function, and with a much better name, than "fix" it.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8809834 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It's possible, it's a pretty basic integer overflow.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
51+.

> If not all supported branches, which bug introduced the flaw?
Bug 1279992.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be trivial to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8809834 [details] [diff] [review]
Patch

Sec-approval+ for trunk. Can you nominate patches for Aurora and Beta as well so we can avoid shipping this flaw publicly?

Comment 14

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6194aa03e1ad1b79c19e0ef146317731bb7b74e2

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8809834 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1279992.
[User impact if declined]: Security issues, crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Just landed.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It just adds a check for an edge case that's unlikely to show up on real websites.
[String changes made/needed]: None.

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6194aa03e1ad

Comment 17

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 18

[Gerry Chang [:gchang]]

Track 51+/52+ as sec-high.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8809834 [details] [diff] [review]
Patch

Fix a sec-high. Beta51+ and Aurora52+. Should be in 51 beta 6.

Comment 20

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e50d7247a130

Comment 21

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a1dbca5c927b

Comment 23

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx51
JSBugMon: This bug has been automatically verified fixed on Fx52

Comment 24

[Marcia Knous [:marcia]]

Verified fixed, no longer tracking for 53.