799803 - IonMonkey: Crash [@ JSString::isAtom] with invalid read

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 22d192c5d1fd (run with --ion-eager):


var gTestcases = new Array();
var gTc = gTestcases.length;
function TestCase(n, d, e, a) {
  this.bugnumber = typeof(BUGNUMER) != 'undefined' ? BUGNUMBER : '';
  gTestcases[gTc++] = this;
  TestCase.prototype.dump = function () {
	+ this.path + this.bugnumber + 'reason: '
  }
};
function reportCompare (expected, actual, description) {
  var testcase = new TestCase();
}
function exitFunc (funcName) {
  reportCompare();
}
function jsTestDriverEnd() {
  for (var i = 0; i < gTestcases.length; i++)
    gTestcases[i].dump();
}
evaluate("\
new TestCase ();\
new TestCase ();\
var BUGNUMBER = 359062;\
schedulegc(10);\
jsTestDriverEnd();\
print(expect = BUGNUMER  = 'Test skipped. Script object required.');\
exitFunc ('test');\
jsTestDriverEnd();\
", { noScriptRval : true });

Comment 1

[Christian Holler (:decoder)]

Crash trace:

==1819== Invalid read of size 4
==1819==    at 0x806DDE4: JSString::isAtom() const (String.h:373)
==1819==    by 0x832F6C1: js_ConcatStrings(JSContext*, JS::Handle<JSString*>, JS::Handle<JSString*>) (String.cpp:301)
==1819==    by 0x817397F: js::AddOperation(JSContext*, JS::Handle<JSScript*>, unsigned char*, JS::Value const&, JS::Value const&, JS::Value*) (jsinterpinlines.h:566)
==1819==    by 0x81906F4: js::AddValues(JSContext*, JS::Handle<JSScript*>, unsigned char*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Value*) (jsinterp.cpp:4130)
==1819==    by 0x7D8AE26: ???
==1819==  Address 0x57a96 is not stack'd, malloc'd or (recently) free'd

Comment 2

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   108213:19da37cf65ae
user:        Eric Faust
date:        Wed Sep 26 19:02:56 2012 -0400
summary:     Bug 793284 - Use non-freezing checks in TestCommonPropFunc() to avoid bogus invalidations. (r=djvj)

This iteration took 83.941 seconds to run.

Comment 3

[Marty Rosenberg [:mjrosenb]]

cc'ing efaust, since it pointed at his commit.

Comment 4

[Eric Faust [:efaust]]

This appears to continue to segfault without any of the common accessor code running at all. I verified this on a --enable-debug --disable-optimize stock shell build.

The revision pointed to changes the check for that optimization, forcing it to stop spewing stray ownProperty freezes on every inherited property, whether the properties had accessors or not, and whether the optimization was actually used or not.

As there doesn't appear to be any accessor redefinition in the test case, it appears that removing these random freezes has uncovered a freezing bug somewhere else, likely the this.bugnumber access is a good place to start.

Comment 5

[Eric Faust [:efaust]]

Crash is indeed involving |this.bugnumber|

gdb notes:

#0  JSString::isAtom (this=0x57a96) at String.h:373
#1  0x000000010037d5ea in js_ConcatStrings (cx=0x101915b90, left={<js::HandleBase<JSString *>> = {<No data fields>}, ptr = 0x7fff5fbfa158}, right={<js::HandleBase<JSString *>> = {<No data fields>}, ptr = 0x7fff5fbfa140}) at /Users/efaust/m-fuckme/js/src/vm/String.cpp:301
#2  0x000000010017e87c in AddOperation (cx=0x101915b90, script={<js::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x7fff5fbfa2d0}, pc=0x10191cca5 "\033=", lhs=@0x7fff5fbfa240, rhs=@0x7fff5fbfa238, res=0x7fff5fbfa2a8) at jsinterpinlines.h:566
#3  0x0000000100181302 in js::AddValues (cx=0x101915b90, script={<js::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x7fff5fbfa2d0}, pc=0x10191cca5 "\033=", lhs={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5fbfa2e0}, rhs={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5fbfa2e8}, res=0x7fff5fbfa2a8) at jsinterp.cpp:3991
#4  0x00000001018f0f8c in ?? ()

and:

(gdb) p this
$3 = (JSString *) 0x57a96
(gdb) p /d this
$4 = 359062

It looks like the set |print(expect = BUGNUMER  = 'Test skipped. Script object required.');| is not setting BUGNUMBER appropriately. If you print BUGNUMBER right after that print line, you see 359062 still there.

I am still struggling to understand how random slot property ownProperty freezes could save this. Scope object value sets, maybe, but it seems a long shot.

Comment 6

[Christian Holler (:decoder)]

Assuming at least sec-high due to invalid read with a bad pointer and comment 4 and 5 seem to indicate that there is some inconsistency in variables involved.

Comment 7

[Eric Faust [:efaust]]

Attachment: Fix

Good lord this was a fun one. Because we now regenerate TI after GC, we can enter situations where, mid-compilation, we generate TI that should invalidate the script we are actively compiling.

Many thanks to Brian Hackett for helping track this one down.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 675472 [details] [diff] [review]
Fix

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

Sounds good, still move the check to ensure that it is more resistant to other input paths.

::: js/src/jsinfer.cpp
@@ +2019,5 @@
> +     * inference information mid-compilation causes an invalidation of the
> +     * script being compiled.
> +     */
> +    RecompileInfo& info = cx->compartment->types.compiledInfo;
> +    if (info.outputIndex != RecompileInfo::NoCompilerRunning) {

Even if this comes later, I would prefer that this check happens in:

TypeCompartment::addPendingRecompile(JSContext *cx, const RecompileInfo &info)

and you should not check for Ion for because this can also appear in JM.

if (cx->compartment->types.compiledInfo.outputIndex != info.outputIndex)
  co->invalidate()

or something like that.

Comment 9

[Christian Holler (:decoder)]

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

Comment 10

[Christian Holler (:decoder)]

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

The first good revision is:
changeset:   111708:4a2c17905a17
user:        Nicolas B. Pierron
date:        Mon Oct 29 14:48:45 2012 -0700
summary:     Bug 792631 - Add IC for missing properties. r=dvander

This iteration took 92.693 seconds to run.

Comment 11

[Christian Holler (:decoder)]

Is the revision in comment 10 a fix for this issue and this bug a duplicate, or is that modification just hiding the bug?

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 675472 [details] [diff] [review]
Fix

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Not easily, not easily at all.

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?

Firefox 18, if it reproduces there.

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

N/A, came with IonMonkey

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Patch should apply to 18.

How likely is this patch to cause regressions; how much testing does it need?

Very low risk.

Comment 13

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

Comment on attachment 675472 [details] [diff] [review]
Fix

Be sure to ask for approval for 18.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 17

[u279076]

Can this be put in testsuite?

Comment 18

[Alex Keybl [:akeybl]]

Do we think that B2G is impacted here? If you know of a good rule for whether or not B2G is impacted by JS , please let us know.

Comment 19

[Nicolas B. Pierron [:nbp]]

(In reply to Alex Keybl [:akeybl] from comment #18)
> Do we think that B2G is impacted here? If you know of a good rule for
> whether or not B2G is impacted by JS , please let us know.

B2G should not be impacted because the constraint system is currently tie to the type system, which is not enabled on B2G for memory consumption issues.