The default bug view has changed. See this FAQ.

Assertion failure: length < (uint32(1) << 23), at jshashtable.h:368

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: decoder, Assigned: luke)

Tracking

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

Trunk
mozilla9
x86_64
Linux
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js-triage-done][inbound])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
The following code asserts on mozilla-central (revision 4b4b359e77e4, no options required):

var replacer = [0, 1, 2, 3];
Object.defineProperty(replacer, 3.e7, {});
JSON.stringify({ 0: 0, 1: 1, 2: 2, 3: 3 }, replacer)


Using 3.e7 this passes after the assert, using 3.e8 fails with InternalError due to allocation failure. I haven't seen any crash or other suspicious behavior but according to Luke it's directly not clear if this is a security problem or not. Remove S-s when this is confirmed to be non-harmful.
(Reporter)

Comment 1

6 years ago
Sorry, the revision was for mozilla-inbound, mozilla-central is also affected though.
(Assignee)

Comment 2

6 years ago
Created attachment 552561 [details] [diff] [review]
fix

So, this (1<<23) bound is real since it guards the subsequent multiply.  On overflow, this will allocate less capacity then requested.  That would be a security problem if json code expected that, if init() succeeded, that it could insert infallibly, but json doesn't.

With a bit of rejiggering, init() can still do only a single test that covers all overflow.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #552561 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 years ago
Group: core-security
Whiteboard: js-triage-needed → js-triage-done
(Assignee)

Comment 3

6 years ago
Created attachment 552565 [details] [diff] [review]
fix v.2

This patch adds static asserts to explain the relaxing of >= to >.
Attachment #552561 - Attachment is obsolete: true
Attachment #552561 - Flags: review?(jwalden+bmo)
Attachment #552565 - Flags: review?(jwalden+bmo)
Comment on attachment 552565 [details] [diff] [review]
fix v.2

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

::: js/src/tests/ecma_5/JSON/stringify-replacer.js
@@ +161,5 @@
> +}
> +catch (e)
> +{
> +  assertEq(""+e, "InternalError: allocation size overflow");
> +}

1. New tests should go in new files, for ease of debugging when we break them during development (or after, if it happens that way).

2. InternalError grunge is totally not standard at all, so these two tests should be in an extensions/ directory.
Attachment #552565 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 5

6 years ago
Done and done.
http://hg.mozilla.org/integration/mozilla-inbound/rev/796b8466d549
Whiteboard: js-triage-done → [js-triage-done][inbound]
http://hg.mozilla.org/mozilla-central/rev/796b8466d549
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Depends on: 816033
(Reporter)

Comment 7

4 years ago
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.