Closed
Bug 795165
Opened 9 years ago
Closed 9 years ago
Assertion failure: (ptrBits & 0x7) == 0, at ../../jsval.h:708 or Crash [@ js::ParallelArrayObject::toStringBufferImpl]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla18
Tracking | Status | |
---|---|---|
firefox16 | --- | unaffected |
firefox17 | + | verified |
firefox18 | --- | verified |
firefox19 | --- | verified |
firefox-esr10 | --- | unaffected |
People
(Reporter: decoder, Assigned: shu)
References
Details
(5 keywords, Whiteboard: [asan][jsbugmon:update,bisect][adv-track-main17-])
Crash Data
Attachments
(1 file)
10.22 KB,
patch
|
dvander
:
review+
lsblakk
:
approval-mozilla-beta+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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
Assignee | ||
Comment 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/f1869aa32cba
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f1869aa32cba
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox18:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Reporter | ||
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 6•9 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•9 years ago
|
status-firefox-esr10:
--- → unaffected
status-firefox17:
--- → affected
status-firefox19:
--- → fixed
tracking-firefox17:
--- → +
Keywords: sec-critical
![]() |
||
Comment 7•9 years ago
|
||
This is marked sec-critical and tracking-firefox17+, are we planning to land this on beta?
Assignee | ||
Comment 8•9 years ago
|
||
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?
Comment 9•9 years ago
|
||
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.
Assignee | ||
Comment 10•9 years ago
|
||
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?
Comment 11•9 years ago
|
||
(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•9 years ago
|
||
Comment on attachment 665749 [details] [diff] [review] backport fix from iontrail sec-approval+
Attachment #665749 -
Flags: sec-approval? → sec-approval+
Updated•9 years ago
|
Attachment #665749 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 13•9 years ago
|
||
(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.
Assignee | ||
Comment 14•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/637ef92521d6
Updated•9 years ago
|
Comment 15•9 years ago
|
||
Marking this as 16 unaffected based on regressing bug. Someone please tell me if this is incorrect.
status-firefox16:
--- → unaffected
Whiteboard: [asan][jsbugmon:update,bisect] → [asan][jsbugmon:update,bisect][adv-track-main17-]
Comment 16•8 years ago
|
||
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.
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•