874974 - Crash [@ setStringThis] with controllable invalid read

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision c21ef3664c67 (no options required):


var gTestcases = new Array();
var x = '';
function TestCase(n, d, e, a) {
  this.bugnumber = x;
  gTestcases[gTc++] = this;
  TestCase.prototype.dump = function () {
    this.bugnumber.toString();
  }
};
function reportCompare () {
  var testcase = new TestCase();
}
for ( gTc=0; gTc < gTestcases.length; gTc++ ) {}
function jsTestDriverEnd() {
  for (var i = 0; i < gTestcases.length; i++)
    gTestcases[i].dump();
}
for (var i = 0x0020; i < 0x007e; i++ ) {
  new TestCase();
  new TestCase();
  new TestCase();
}
gc();
jsTestDriverEnd();
jsTestDriverEnd();
jsTestDriverEnd();
var x = 0xbeef; // Yummy!
jsTestDriverEnd();
reportCompare();
jsTestDriverEnd();

Comment 1

[Christian Holler (:decoder)]

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

Comment 2

[Christian Holler (:decoder)]

I took the liberty to enhance this test with a little beef...



Program received signal SIGSEGV, Segmentation fault.
setStringThis (str=<error reading variable: Cannot access memory at address 0xbeef>, this=(js::StringObject * const) 0x7ffff653b7c0 [object String]) at js/src/vm/StringObject.h:51
51              setFixedSlot(LENGTH_SLOT, Int32Value(int32_t(str->length())));
#0  setStringThis (str=<error reading variable: Cannot access memory at address 0xbeef>, this=(js::StringObject * const) 0x7ffff653b7c0 [object String]) at js/src/vm/StringObject.h:51
#1  js::StringObject::init (this=<optimized out>, cx=<optimized out>, str=...) at js/src/vm/StringObject-inl.h:36
#2  0x000000000054d32f in create (newKind=js::GenericObject, str=..., cx=<optimized out>) at js/src/vm/StringObject-inl.h:48
#3  PrimitiveToObject (cx=<optimized out>, v=...) at js/src/jsobj.cpp:4848
#4  0x000000000054d44d in js::ToObjectSlow (cx=<optimized out>, val=..., reportScanStack=<optimized out>) at js/src/jsobj.cpp:4903
#5  0x000000000051118b in ToObjectFromStack (vp=$jsval(<error reading variable: Cannot access memory at address 0xbeef>), cx=0x1851730) at ../jsobj.h:1440
#6  js::GetProperty (cx=0x1851730, v=$jsval(<error reading variable: Cannot access memory at address 0xbeef>), name="toString", vp=...) at js/src/jsinterp.cpp:3284
#7  0x00007ffff67c2d5c in ?? ()
r12     0xbeef  48879
rip     0x53b037 <js::StringObject::init(JSContext*, JS::Handle<JSString*>)+263>
=> 0x53b037 <js::StringObject::init(JSContext*, JS::Handle<JSString*>)+263>:    mov    (%r12),%rax


This doesn't look good at all. Marking sec-critical unless shown to be less severe.

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   131119:0f91553baf96
user:        Jan de Mooij
date:        Tue May 07 16:31:25 2013 +0200
summary:     Bug 866050 - Fix UsesBeforeIonRecompile. r=bhackett

This iteration took 327.746 seconds to run.

Comment 4

[Christian Holler (:decoder)]

Needinfo from Jan based on comment 3 :)

Comment 5

[Jan de Mooij [:jandem]]

autoBisect is wrong, the underlying bug is older.

What's happening is that in IonBuilder::getPropTryDefiniteSlot we call GetDefiniteSlot => propertyTypes->isOwnProperty => CheckNewScriptProperties and this adds an extra type (int32) to the property's heap TypeSet.

When we return to IonBuilder::getPropTryDefiniteSlot, it still thinks the GETPROP's type is definitely string and the code it generates crashes because it treats an int32 value as string pointer.

Brian, how is this supposed to work? Do we have a way to guard against this somehow?

Comment 6

[Andrew McCreight [:mccr8]]

Marking flags under the assumption that this is a regression from the IonMonkey landing.  Adjust as appropriate.

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> autoBisect is wrong, the underlying bug is older.
> 
> What's happening is that in IonBuilder::getPropTryDefiniteSlot we call
> GetDefiniteSlot => propertyTypes->isOwnProperty => CheckNewScriptProperties
> and this adds an extra type (int32) to the property's heap TypeSet.
> 
> When we return to IonBuilder::getPropTryDefiniteSlot, it still thinks the
> GETPROP's type is definitely string and the code it generates crashes
> because it treats an int32 value as string pointer.
> 
> Brian, how is this supposed to work? Do we have a way to guard against this
> somehow?

The freeze constraint added under PropertyReadNeedsTypeBarrier is supposed to ensure that the compilation is invalidated immediately after it finishes.  It doesn't look like that is happening, and from ~AutoEnterCompilation it doesn't look like we are ever able to invalidate the currently active compilation.  Nicolas, thoughts?

Comment 8

[Andrew McCreight [:mccr8]]

Assigning this to jandem, as it looks like he's investigating.

Comment 9

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision cc35f8929768).

Comment 10

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision cc35f8929768).
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/0f91553baf96
user:        Jan de Mooij
date:        Tue May 07 16:31:25 2013 +0200
summary:     Bug 866050 - Fix UsesBeforeIonRecompile. r=bhackett

This iteration took 0.918 seconds to run.

Comment 11

[Christian Holler (:decoder)]

JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/1684c32be328
user:        Kannan Vijayan
date:        Tue Jun 11 15:49:51 2013 -0400
summary:     Bug 859609 - Inline functions that use the scope chain, and also inline call sites with monomorphic cloned lambdas. r=h4writer

This iteration took 317.084 seconds to run.

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

djvj, is bug 859609 a possible fix?

Comment 13

[Kannan Vijayan [:djvj]]

Nope.  This patch is unrelated, and simply masks the issue in this bug.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Reverting needinfo to nbp as per comment 7.

Comment 15

[Jesse Ruderman]

Kannan, do you know how to transform the testcase into one that still shows the bug? (Just in case fuzzing doesn't turn one up easily.)

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Patch

There are two functions used by TI code to invalidate scripts:

(1) TypeCompartment::addPendingRecompile(JSContext *cx, const RecompileInfo &info)
(2) AddPendingRecompile(JSContext *cx, JSScript *script)

The second one correctly handles invalidation during compilation by checking if there's a compiler running and if so it calls CompilerOutput::invalidate(). Ion's CodeGenerator::link checks this and aborts compilation if that's set.

However, the first method is basically a no-op during compilation, because AutoEnterCompilation sets pendingRecompilation to |true| during compilation and addPendingRecompile returns immediately in that case.

Brian, do you have any idea why we're only seeing this now? I think it should also affect aurora, beta and release.

Comment 17

[Jan de Mooij [:jandem]]

And btw, we're running code based on outdated type information so this is definitely sec-critical.

Comment 18

[Brian Hackett [Laid off!]]

Comment on attachment 762008 [details] [diff] [review]
Patch

Review of attachment 762008 [details] [diff] [review]:
-----------------------------------------------------------------

This is definitely an older bug, though I don't know why it would start showing up now.  Maybe 804676 made this easier to expose?

Comment 19

[Jan de Mooij [:jandem]]

Comment on attachment 762008 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? It's possible for somebody who knows what he/she is doing, but not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No tests. There are some short comments but you could get the same info from the code with little more effort.

Which older supported branches are affected by this flaw? Release, beta, aurora. This is the first reported (fuzz) bug that triggers it though so for some reason it may be hard/impossible to trigger on older branches.

If not all supported branches, which bug introduced the flaw? -

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Patch won't apply to branches, but we need to backport only a small part of it, should be easy.

How likely is this patch to cause regressions; how much testing does it need? Unlikely - usual fuzz testing + a few nightlies should be enough.

Comment 20

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

I'm unclear of how far back we need to take this but I'll give sec-approval+ for trunk to get it onto 24 before we branch. If 23 is the only affected version (effectively), we should get a patch for it prepared.

Comment 21

[Jan de Mooij [:jandem]]

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

I will also prepare a patch for 23.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically confirmed to be still valid on branch mozilla-aurora  (reproduced on revision 6b40d5226c13).
JSBugMon: The testcase found in this bug does not reproduce on branch mozilla-beta (tried revision 9f0d4fe5db55).
JSBugMon: The testcase found in this bug does not reproduce on branch mozilla-release (tried revision 88ff4d1b205f).
JSBugMon: The testcase found in this bug does not reproduce on branch mozilla-esr17 (tried revision 468f2568342c).

Comment 24

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 25

[Jan de Mooij [:jandem]]

Attachment: Patch for aurora

Minimal fix for aurora. This fixes the crash for me.

Comment 26

[Jan de Mooij [:jandem]]

Comment on attachment 764086 [details] [diff] [review]
Patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown, we think bug 804676 may have exposed an older bug.
User impact if declined: Security bugs, crashes.
Testing completed (on m-c, etc.): A similar patch landed on m-c.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.

Comment 27

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dbd00c459693

Comment 28

[u279076]

Marking status-firefox24:verified based on comment 24.