Closed
Bug 886266
Opened 9 years ago
Closed 9 years ago
Stack out-of-bounds read [@ js::ion::IonFrameIterator::prevType]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla25
Tracking | Status | |
---|---|---|
firefox23 | --- | unaffected |
firefox24 | + | fixed |
firefox25 | + | fixed |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: decoder, Assigned: h4writer)
References
Details
(4 keywords, Whiteboard: [asan])
Attachments
(2 files, 1 obsolete file)
1.20 KB,
text/plain
|
Details | |
1.91 KB,
patch
|
jandem
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The following testcase shows Stack out-of-bounds read on mozilla-central revision 76820c6dff7b (run with --ion-eager): function coerceForeign(stdlib, foreign) { "use asm"; var g = foreign.g; var h = foreign.h; function f() { +g(0); +g(1); +g(2); +h(2); } return f; } var t = coerceForeign(undefined, { g: function(a) { if (a == (/\u00ea/ )) var blaat = new blaat(); }, h: function(b) function i() { } }) t();
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Comment 2•9 years ago
|
||
Here is the full trace from ASan: ==31307== ERROR: AddressSanitizer: stack-buffer-overflow on address 0xff816a08 at pc 0x8aefd23 bp 0xff814938 sp 0xff814930 READ of size 4 at 0xff816a08 thread T0 #0 0x8aefd22 in js::ion::IonFrameIterator::prevType() const js/src/ion/shared/IonFrames-x86-shared.h:31 #1 0x85b0a7d in JSContext::currentScript(unsigned char**, JSContext::MaybeAllowCrossCompartment) const js/src/jscntxtinlines.h:579 #2 0x85d4d89 in CallResolveOp(JSContext*, JS::Handle<JSObject*>, JS::Handle<int>, unsigned int, JS::MutableHandle<JSObject*>, JS::MutableHandle<js::Shape*>, bool*) js/src/jsobj.cpp:3538 #3 0x85e7cfd in JSObject::getGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<int>, JS::MutableHandle<JS::Value>) js/src/jsobjinlines.h:134 #4 0x85e6ee2 in js::DefaultValue(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) js/src/jsobj.cpp:4673 #5 0x858f3dd in JSObject::defaultValue(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) js/src/jsobjinlines.h:37 #6 0x880772a in JS::ToNumber(JSContext*, JS::Value const&, double*) js/src/jsapi.h:1515 #7 0xf727522d in Address 0xff816a08 is located at offset 328 in frame <InvokeFromAsmJS_ToNumber(JSContext*, js::AsmJSModule::ExitDatum*, int, JS::Value*)> of T0's stack: This frame has 6 object(s): [32, 40) 'agg.tmp.i21' [96, 104) 'agg.tmp.i' [160, 168) 'fval' [224, 232) 'rval' [288, 296) 'ref.tmp' [352, 360) 'dbl' HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext (longjmp and C++ exceptions *are* supported) Shadow bytes around the buggy address: 0x3ff02cf0: f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 0x3ff02d00: f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00 0x3ff02d10: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 0x3ff02d20: f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 0x3ff02d30: f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 =>0x3ff02d40: f2[f2]f2 f2 00 f4 f4 f4 f3 f3 f3 f3 00 00 00 00 0x3ff02d50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x3ff02d60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x3ff02d70: f1 f1 f1 f1 00 f4 f4 f4 f2 f2 f2 f2 00 00 00 00 0x3ff02d80: 00 00 00 00 00 00 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 0x3ff02d90: f2 f2 f2 f2 00 00 00 f4 f2 f2 f2 f2 00 00 00 f4 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap righ redzone: fb Freed Heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 ASan internal: fe This cannot be detected with Valgrind. Marking sec-high based on invalid read on the stack. If this is harmless, feel free to downgrade.
Keywords: csec-bounds,
sec-high
Whiteboard: [asan]
![]() |
||
Comment 3•9 years ago
|
||
This also looks like one of these empty-IonActivation issues.
Assignee | ||
Updated•9 years ago
|
Assignee: general → hv1989
Assignee | ||
Comment 4•9 years ago
|
||
This fixes the problem for me. But I'm not 100% sure about the fix. Gonna ask jandem tomorrow to explain ionTop better.
Assignee | ||
Updated•9 years ago
|
status-firefox24:
--- → affected
status-firefox25:
--- → affected
Assignee | ||
Comment 5•9 years ago
|
||
I think this is the better patch. setActive(false), didn't reset the ionTop to prevIonTop. Just like the deconstructor of JitActivation would do.
Attachment #766908 -
Attachment is obsolete: true
Attachment #767057 -
Flags: review?(jdemooij)
Comment 6•9 years ago
|
||
Comment on attachment 767057 [details] [diff] [review] Patch Review of attachment 767057 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Stack.cpp @@ +1303,5 @@ > } > #endif > > +void > +Activation::setActive(bool active) { Nit: { on its own line @@ +1322,5 @@ > prevIonJSContext_(cx->mainThread().ionJSContext), > firstFrameIsConstructing_(firstFrameIsConstructing) > { > + // Unsupported to change context for inactive activation. > + JS_ASSERT_IF(!active, cx->mainThread().ionJSContext == cx); This assert will fail if there's no other JitActivation on the stack. I think you can just remove it?
Attachment #767057 -
Flags: review?(jdemooij) → review+
Updated•9 years ago
|
tracking-firefox24:
--- → ?
tracking-firefox25:
--- → ?
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec1974330bfc
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ec1974330bfc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 767057 [details] [diff] [review] Patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 860838 User impact if declined: High chance of asm.js code not working properly and crashing. Testing completed (on m-c, etc.): m-c for a day, m-i for 2 days Risk to taking this patch (and alternatives if risky): Very low risk. String or IDL/UUID changes made by this patch: /
Attachment #767057 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #767057 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter | ||
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 12•9 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Comment 13•9 years ago
|
||
This is Firefox 23 unaffected based on Bug 860838 checkin date of 6/13.
status-firefox23:
--- → unaffected
Updated•9 years ago
|
Blocks: 860838
status-b2g18:
--- → unaffected
status-firefox-esr17:
--- → unaffected
Keywords: regression
Updated•8 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•