886266 - Stack out-of-bounds read [@ js::ion::IonFrameIterator::prevType]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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();

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

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.

Comment 3

[Luke Wagner [:luke]]

This also looks like one of these empty-IonActivation issues.

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Possible patch

This fixes the problem for me. But I'm not 100% sure about the fix. Gonna ask jandem tomorrow to explain ionTop better.

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Patch

I think this is the better patch.
setActive(false), didn't reset the ionTop to prevIonTop. Just like the deconstructor of JitActivation would do.

Comment 6

[Jan de Mooij [:jandem]]

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?

Comment 7

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec1974330bfc

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Hannes Verschore [:h4writer]]

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: /

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/29f62a025aae

Comment 12

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 13

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

This is Firefox 23 unaffected based on Bug 860838 checkin date of 6/13.