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

VERIFIED FIXED in Firefox 51

Status

()

defect
--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: gkw, Assigned: jandem)

Tracking

(Blocks 2 bugs, 5 keywords)

Trunk
mozilla53
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 unaffected, firefox51+ verified, firefox52+ verified, firefox53 verified)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments)

Reporter

Description

3 years ago
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.
Reporter

Comment 2

3 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/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?
Blocks: 1292858
Flags: needinfo?(jdemooij)
Reporter

Comment 3

3 years ago
=== 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...
Blocks: 1279992
No longer blocks: 1292858
Reporter

Comment 4

3 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
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.
Assignee

Comment 6

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

Comment 7

3 years ago
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.
Reporter

Comment 8

3 years ago
Sander, what's next here?
Flags: needinfo?(sandervv)
Reporter

Comment 9

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

Comment 10

3 years ago
Posted patch PatchSplinter Review
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8809834 - Flags: review?(jwalden+bmo)
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.
Attachment #8809834 - Flags: review?(jwalden+bmo) → review+
Assignee

Comment 12

3 years ago
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.
Attachment #8809834 - Flags: sec-approval?
Assignee

Updated

3 years ago
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?
Attachment #8809834 - Flags: sec-approval? → sec-approval+
Assignee

Comment 15

3 years ago
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.
Attachment #8809834 - Flags: approval-mozilla-beta?
Attachment #8809834 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/6194aa03e1ad
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

3 years ago
Status: RESOLVED → VERIFIED

Comment 17

3 years ago
JSBugMon: This bug has been automatically verified fixed.
Track 51+/52+ as sec-high.
Comment on attachment 8809834 [details] [diff] [review]
Patch

Fix a sec-high. Beta51+ and Aurora52+. Should be in 51 beta 6.
Attachment #8809834 - Flags: approval-mozilla-beta?
Attachment #8809834 - Flags: approval-mozilla-beta+
Attachment #8809834 - Flags: approval-mozilla-aurora?
Attachment #8809834 - Flags: approval-mozilla-aurora+
Assignee

Updated

3 years ago
Duplicate of this bug: 1314175

Comment 23

3 years ago
JSBugMon: This bug has been automatically verified fixed on Fx51
JSBugMon: This bug has been automatically verified fixed on Fx52
Verified fixed, no longer tracking for 53.
Group: javascript-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.