Closed Bug 795165 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox16 --- unaffected
firefox17 + verified
firefox18 --- verified
firefox19 --- verified
firefox-esr10 --- unaffected

People

(Reporter: decoder, Assigned: shu)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [asan][jsbugmon:update,bisect][adv-track-main17-])

Crash Data

Attachments

(1 file)

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.
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.
Assignee: general → shu
Attachment #665749 - Flags: review?(dmandelin)
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.
See Also: → 795227
Comment on attachment 665749 [details] [diff] [review]
backport fix from iontrail

Flipping r? over to dvander
Attachment #665749 - Flags: review?(dmandelin) → review?(dvander)
Attachment #665749 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/f1869aa32cba
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
This is marked sec-critical and tracking-firefox17+, are we planning to land this on beta?
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.
Attachment #665749 - Flags: approval-mozilla-beta?
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 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.
Attachment #665749 - Flags: sec-approval?
(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 on attachment 665749 [details] [diff] [review]
backport fix from iontrail

sec-approval+
Attachment #665749 - Flags: sec-approval? → sec-approval+
Attachment #665749 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Marking this as 16 unaffected based on regressing bug. Someone please tell me if this is incorrect.
Whiteboard: [asan][jsbugmon:update,bisect] → [asan][jsbugmon:update,bisect][adv-track-main17-]
Can this bug be marked verified 17, 18, 19 based on in-testsuite+?
(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.
Blocks: 778559
Group: core-security
Keywords: regression
You need to log in before you can comment on or make changes to this bug.