Closed Bug 799803 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox16 --- unaffected
firefox17 --- unaffected
firefox18 --- affected
firefox19 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: decoder, Assigned: efaust)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [ion:p1:fx19] [jsbugmon:] [adv-main19+])

Crash Data

Attachments

(1 file)

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 });
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
Blocks: IonFuzz
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
cc'ing efaust, since it pointed at his commit.
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.
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.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1]
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.
Whiteboard: [jsbugmon:update][ion:p1] → [jsbugmon:update][ion:p1:fx19]
Attached patch FixSplinter Review
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.
Assignee: general → efaustbmo
Status: NEW → ASSIGNED
Attachment #675472 - Flags: review?(nicolas.b.pierron)
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.
Attachment #675472 - Flags: review?(nicolas.b.pierron) → review+
Whiteboard: [jsbugmon:update][ion:p1:fx19] → [ion:p1:fx19] [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 71290adea0c1).
Whiteboard: [ion:p1:fx19] [jsbugmon:update,ignore] → [ion:p1:fx19] [jsbugmon:bisectfix]
Whiteboard: [ion:p1:fx19] [jsbugmon:bisectfix] → [ion:p1:fx19] [jsbugmon:]
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.
Is the revision in comment 10 a fix for this issue and this bug a duplicate, or is that modification just hiding the bug?
Flags: needinfo?(nicolas.b.pierron)
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.
Attachment #675472 - Flags: sec-approval?
Comment on attachment 675472 [details] [diff] [review]
Fix

Be sure to ask for approval for 18.
Attachment #675472 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/f75007880173
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Status: RESOLVED → VERIFIED
Flags: needinfo?(nicolas.b.pierron)
JSBugMon: This bug has been automatically verified fixed.
Can this be put in testsuite?
Flags: in-testsuite?
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.
(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.
Whiteboard: [ion:p1:fx19] [jsbugmon:] → [ion:p1:fx19] [jsbugmon:] [adv-main19+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.