795165 - Assertion failure: (ptrBits & 0x7) == 0, at ../../jsval.h:708 or Crash [@ js::ParallelArrayObject::toStringBufferImpl]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, critical)

Description

[Christian Holler (:decoder)]

The following test asserts/crashes on mozilla-central revision df69d95f636c (no options required):


try {
function k(f_arg) {}
function testScatter() {
  var shape = [5];
  for (var i = 0; i < 7; i++) {
    shape.push(i+(65536));
    var p = new ParallelArray(shape, function() { return k; });
    var r = p.scatter([0,1,0,3,4], 9, function (a,b) { return new (function k() {}) +b; }, 10);
  }
}
testScatter();
} catch(exc1) {}


Found this on the ASan testing server, opt build causes an ASan error here:

==17126== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7fc440efe090 at pc 0x99f4d0 bp 0x7fff6a02bc10 sp 0x7fff6a02bc08
READ of size 8 at 0x7fc440efe090 thread T0
    #0 0x99f4cf in js::ParallelArrayObject::toStringBufferImpl(JSContext*, js::ParallelArrayObject::IndexInfo&, bool, JS::Handle<JSObject*>, js::StringBuffer&) ../jscntxt.h:1892
    #1 0x99e6c9 in js::ParallelArrayObject::toStringBufferImpl(JSContext*, js::ParallelArrayObject::IndexInfo&, bool, JS::Handle<JSObject*>, js::StringBuffer&) js/src/builtin/ParallelArray.cpp:1483
    #2 0x99f946 in js::ParallelArrayObject::toStringBuffer(JSContext*, bool, js::StringBuffer&) js/src/builtin/ParallelArray.cpp:1565
    #3 0x99fbe7 in js::ParallelArrayObject::toString(JSContext*, JS::CallArgs) js/src/builtin/ParallelArray.cpp:1572
    #4 0x990554 in JS::CallNonGenericMethod(JSContext*, bool (*)(JS::Value const&), bool (*)(JSContext*, JS::CallArgs), JS::CallArgs) ../jsapi.h:1572
    #5 0x5f5403 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) ../jscntxtinlines.h:372
    #6 0x5f62b7 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) ../jsinterp.h:119
    #7 0x66e2b3 in js::MaybeCallMethod(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, JS::MutableHandle<JS::Value>) js/src/jsobj.cpp:5095
    #8 0x66dbac in js::DefaultValue(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) js/src/jsobj.cpp:5157
    #9 0x5e2b87 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) ../jsobjinlines.h:69
    #10 0x5c9be6 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) js/src/jsinterp.cpp:324
    #11 0x5f52f4 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:378
    #12 0x98ae8e in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) ../jsinterp.h:119
    #13 0x99b7ed in js::ParallelArrayObject::FallbackMode::scatter(JSContext*, JS::Handle<js::ParallelArrayObject*>, JS::Handle<JSObject*>, JS::Value const&, JS::Handle<JSObject*>, JS::Handle<JSObject*>) js/src/builtin/ParallelArray.cpp:639
[...]


S-s due to assertion known to be dangerous and crashing in opt-builds.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: backport fix from iontrail

The toString implementation was changed a while back in the development branch of RiverTrail to be iterative instead of recursive, much like the build operation.

I can't reproduce this bug on the iterative version, so I'm just going to backport the iterative version.

Comment 2

[Shu-yu Guo [:shu]]

Bug 795227 has a more systematic fix.

While we manually check when getting particular elements that the indices are in bounds, the toString loop does not go through the same code to get an element. So what happens in toString when the total length of the array overflows is, well, as you can see, really bad.

The iterative version avoids this by never iterating outside of the actual allocated bounds, even if we allocated the wrong amount. The real fix, though, is to check for the range like in Array.

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 665749 [details] [diff] [review]
backport fix from iontrail

Flipping r? over to dvander

Comment 4

[Shu-yu Guo [:shu]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f1869aa32cba

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 7

[Robert Kaiser]

This is marked sec-critical and tracking-firefox17+, are we planning to land this on beta?

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 665749 [details] [diff] [review]
backport fix from iontrail


[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Bug #778559
User impact if declined:
  Possible buffer overflow on allocating multidimensional arrays whose scalar length overflows uint32
Testing completed (on m-c, etc.): 
  on m-c.
Risk to taking this patch (and alternatives if risky): 
  None.
String or UUID changes made by this patch: 
  None.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Please also flag the patch sec-approval?  so we can ensure proper communication around landing timing for this issue since it's sec-critical and looking for uplift.

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 665749 [details] [diff] [review]
backport fix from iontrail

[Security approval request comment]
How easily can the security issue be deduced from the patch?
  Not entirely obvious, as it changes implementation styles from recursive to iterative. Such a change (indeed, this change too) is often motivated by performance.
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?
  Beta
If not all supported branches, which bug introduced the flaw?
  Same as above
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
  The existing patch should apply to Beta
How likely is this patch to cause regressions; how much testing does it need?
  Highly unlikely.

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to Lukas Blakk [:lsblakk] from comment #9)
> Please also flag the patch sec-approval?  so we can ensure proper
> communication around landing timing for this issue since it's sec-critical
> and looking for uplift.

We'd actually like sec-approval before landing on mozilla-central in the future -- that's where the risk of disclosure is greatest if we're not prepared to uplift revealing patches.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 665749 [details] [diff] [review]
backport fix from iontrail

sec-approval+

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Daniel Veditz [:dveditz] from comment #11)
> 
> We'd actually like sec-approval before landing on mozilla-central in the
> future -- that's where the risk of disclosure is greatest if we're not
> prepared to uplift revealing patches.

My bad, I mis-read the process last week.

Please go ahead with landing to beta branch ideally before EOD PT tomorrow (Monday) so that this is in out 17.0 beta 3 build.

Comment 14

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/637ef92521d6

Comment 15

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

Marking this as 16 unaffected based on regressing bug. Someone please tell me if this is incorrect.

Comment 16

[Anthony Hughes (:ashughes) [QA]]

Can this bug be marked verified 17, 18, 19 based on in-testsuite+?

Comment 17

[Gary Kwong [:gkw] [:nth10sd]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #16)
> Can this bug be marked verified 17, 18, 19 based on in-testsuite+?

Generally, in-testsuite+ means the test has at least landed on mozilla-central. It does not necessarily mean that the test also landed on other affected branches.

For this case, inspecting:

https://hg.mozilla.org/mozilla-central/rev/f1869aa32cba
https://hg.mozilla.org/releases/mozilla-aurora/rev/f1869aa32cba
https://hg.mozilla.org/releases/mozilla-beta/rev/637ef92521d6

shows that the test did indeed land on other branches, so yes, it can be marked verified 17, 18, 19.