Closed Bug 1313807 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 + verified
firefox52 + verified
firefox53 --- verified

People

(Reporter: gkw, Assigned: jandem)

References

Details

(5 keywords, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

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.
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)
=== 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
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.
(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)
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.
Sander, what's next here?
Flags: needinfo?(sandervv)
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)
Attached 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+
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?
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+
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Status: RESOLVED → VERIFIED
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+
Duplicate of this bug: 1314175
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.