Last Comment Bug 678362 - Assertion failure: length < (uint32(1) << 23), at jshashtable.h:368
: Assertion failure: length < (uint32(1) << 23), at jshashtable.h:368
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
-- critical (vote)
: mozilla9
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
Depends on: 816033
Blocks: langfuzz
  Show dependency treegraph
Reported: 2011-08-11 14:50 PDT by Christian Holler (:decoder)
Modified: 2013-01-19 14:04 PST (History)
5 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix (4.07 KB, patch)
2011-08-11 17:58 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
fix v.2 (4.52 KB, patch)
2011-08-11 18:10 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2011-08-11 14:50:15 PDT
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.
Comment 1 User image Christian Holler (:decoder) 2011-08-11 14:51:01 PDT
Sorry, the revision was for mozilla-inbound, mozilla-central is also affected though.
Comment 2 User image Luke Wagner [:luke] 2011-08-11 17:58:58 PDT
Created attachment 552561 [details] [diff] [review]

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.
Comment 3 User image Luke Wagner [:luke] 2011-08-11 18:10:58 PDT
Created attachment 552565 [details] [diff] [review]
fix v.2

This patch adds static asserts to explain the relaxing of >= to >.
Comment 4 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-15 09:22:33 PDT
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.
Comment 5 User image Luke Wagner [:luke] 2011-08-18 16:58:22 PDT
Done and done.
Comment 6 User image Marco Bonardo [::mak] 2011-08-19 03:21:11 PDT
Comment 7 User image Christian Holler (:decoder) 2013-01-19 14:04:28 PST
Automatically extracted testcase for this bug was committed:

Note You need to log in before you can comment on or make changes to this bug.