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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

function f(x) {
    new Int32Array(x);
}
f(0);
f(2147483647);


Backtrace:

0   js-dbg-64-dm-clang-darwin-97a52326b06a	0x0000000110610220 js::Nursery::allocateBuffer(JSObject*, unsigned int) + 256 (Nursery.cpp:357)
1   js-dbg-64-dm-clang-darwin-97a52326b06a	0x000000010fef08f7 AllocateObjectBufferWithInit(JSContext*, js::TypedArrayObject*, int) + 359 (MacroAssembler.cpp:1063)
2   ???                           	0x0000000112163ebb 0 + 4598415035
3   js-dbg-64-dm-clang-darwin-97a52326b06a	0x000000010fda76ab js::jit::IonCannon(JSContext*, js::RunState&) + 715 (Ion.cpp:2822)
4   js-dbg-64-dm-clang-darwin-97a52326b06a	0x000000011030fecf js::RunScript(JSContext*, js::RunState&) + 351 (Interpreter.cpp:379)
5   js-dbg-64-dm-clang-darwin-97a52326b06a	0x0000000110321a3e js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 750 (Interpreter.cpp:471)
6   js-dbg-64-dm-clang-darwin-97a52326b06a	0x000000010fcd0cde js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 1918 (BaselineIC.cpp:5989)
/snip

For detailed crash information, see attachment.

Setting s-s because gc stuff is on the stack, and this seems to involve TypedArrays.

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/810a43be36de
user:        Sander Mathijs van Veen
date:        Fri Aug 12 21:38:45 2016 -0400
summary:     Bug 1293258 - AddressSanitizer: attempting double-free on 0x614000342640 [@ __interceptor_free] or Crash [@ jemalloc_crash] with TypedArray. r=jandem

Sander, is bug 1293258 a likely regressor?

Comment 3

[Sander Mathijs van Veen]

Attachment: bug1296249-nbytes-overflow.patch

Prevent an overflow caused by the JS_ROUNDUP since |allocateBuffer| converts |nbytes| of type size_t to uint32_t. The value for |nbytes| will truncate to zero when |new Int32Array(2147483647)| is used.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8783469 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

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

::: js/src/jit/MacroAssembler.cpp
@@ +1059,5 @@
>  
> +    // Prevent an overflow caused by the JS_ROUNDUP since |allocateBuffer|
> +    // converts |nbytes| of type size_t to uint32_t. The value for |nbytes| will
> +    // truncate to zero when |new Int32Array(2147483647)| is used.
> +    if (nbytes > TypedArrayObject::SINGLETON_BYTE_LENGTH)

Can you make this >= to be consistent with other places where we check this?

Comment 5

[Sander Mathijs van Veen]

Attachment: bug1296249-nbytes-overflow.patch

Used ">=" for consistency.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/afd6ad990dd4 for the test failing like https://treeherder.mozilla.org/logviewer.html#?job_id=35787106&repo=mozilla-inbound in, so far, the ARM shell build we run on Linux32, the ARM shell build we run really weirdly, displaying on the Linux64 row but being the SM(arm) right next to the SM(arm64) so I bet it's actually 32-bit, and the SM(p) on the WinXP row (which is a lie, it's Server 2008, but probably means 32-bit as well).

Comment 8

[Phil Ringnalda (:philor)]

And 64-bit didn't save you from everything, in the browser jittests you got OOMs like https://treeherder.mozilla.org/logviewer.html#?job_id=35790646&repo=mozilla-inbound

Comment 9

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

Sander, your patch got backed out, any chance of working it through? (Else we'll just pass the ni? along)

Comment 10

[Sander Mathijs van Veen]

Attachment: bug1296249-nbytes-overflow.patch

I've verified locally that the OOM exception doesn't fail the test case when running the test in a shell with |ulimit -m 4096000; ulimit -v 4096000| set. The try job results are pending (https://treeherder.mozilla.org/#/jobs?repo=try&revision=683968dd3ed3).

I'm using |./mach 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| for the try build. Is this sufficient?

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8794776 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

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

I see you still got some jit-test failures on Try, let me know if you need help with that.

::: js/src/jit/MacroAssembler.cpp
@@ +1063,5 @@
>      }
>  
> +    // Prevent an overflow caused by the JS_ROUNDUP since |allocateBuffer|
> +    // converts |nbytes| of type size_t to uint32_t. The value for |nbytes| will
> +    // truncate to zero when |new Int32Array(2147483647)| is used.

Please change allocateBuffer to just take size_t instead of uint32_t, to fix this footgun. If we do that, can we remove this change?

Comment 12

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]: Sec bug.

Comment 13

[Sander Mathijs van Veen]

Attachment: bug1296249-nbytes-overflow.patch

I fixed the comments in the updated patch.

Steps to reproduce the test failure for arm-sim running on x86-64:

$ cd js/src/;
$ ./devtools/automation/autospider.py --build-only --dep arm-sim;
$ python jit-test/jit_test.py --jitflags=all -j8 ../../obj-spider/js/src/js basic/bug1296249.js
Assertion failure: cx->isExceptionPending() (Thunk execution failed but no exception was raised - missing call to js::ReportOutOfMemory()?), at /home/smvv/work/mozilla-central/js/src/builtin/TestingFunctions.cpp:1373
Exit code: -11
FAIL - basic/bug1296249.js

I'm starting a new try job to make sure that the uint32_t -> size_t change doesn't break any builds. Results will be listed here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=efbca55c4921

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8794828 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

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

Thanks. Don't forget to request sec-approval before landing, as this affects Aurora and is probably exploitable.

Comment 15

[Sander Mathijs van Veen]

The assertion fails on arm 32-bit (using the arm-sim running on a host x86-64 bit).

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Sander Mathijs van Veen from comment #15)
> The assertion fails on arm 32-bit (using the arm-sim running on a host
> x86-64 bit).

I filed bug 1305948 with a patch for this OOM bug.

Comment 17

[Sander Mathijs van Veen]

Comment on attachment 8794828 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I think it would be easy since the test case that illustrates the assertion failure is easy to construct.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The test gives a clear hint about the security problem.

Which older supported branches are affected by this flaw?
The patch landed in Firefox 51.

If not all supported branches, which bug introduced the flaw?
https://bugzilla.mozilla.org/show_bug.cgi?id=1279992

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I do not have backports, but the patch should be easy to backport to the affected branches. They are not risky and not hard to create.

How likely is this patch to cause regressions; how much testing does it need?
It is not likely to cause regressions.

Comment 18

[Marcia Knous [:marcia]]

Tracking 52+ for this assertion failure.

Comment 19

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

sec-approval+ for trunk.
We'll want an aurora patch made and nominated for this as well so we don't ship the bug.

Comment 20

[Sander Mathijs van Veen]

Comment on attachment 8794828 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

Approval Request Comment
[Feature/regressing bug #]: 1296249
[User impact if declined]: it could allocate zero bytes in the nursery for the typed array elements buffer. Writing to that typed array's buffer is dangerous.
[Describe test coverage new/current, TreeHerder]: The added test verifies that an OOM error is thrown instead of an assertion failure.
[Risks and why]: Not sure what risks this could have. The size parameter is changed from uint32_t to size_t so it would only affect platforms where size_t > uint32_t.
[String/UUID change made/needed]:

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Gerry Chang [:gchang]]

Comment on attachment 8794828 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

Sec-high. Take it in 51 aurora.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3fc50786827c

Comment 25

[Fuzzing Team]

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