Closed Bug 792944 Opened 7 years ago Closed 7 years ago

IonMonkey: Assertion failure: Crash at heap with RegExp and GC

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox17 --- unaffected
firefox18 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update][ion:p1:fx18][adv-main18-])

Attachments

(1 file)

The following testcase asserts on mozilla-central revision e4757379b99a (run with --ion-eager):


function whoo() {
  /foo.*baz/.test('foobarbaz')
}
var orig_test = RegExp.prototype.test;
whoo();
eval('RegExp.prototype.test = function(str) { return orig_test.call(this, str) }')
whoo();
RegExp.prototype.test = ((1).volatile);
gc();
whoo();
Valgrind shows:

==37517== Invalid read of size 8
==37517==    at 0x403BD57: ???
==37517==    by 0x6498EB: EnterIon(JSContext*, js::StackFrame*, void*) (Ion.cpp:1325)
==37517==    by 0x495791: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2483)
==37517==    by 0x49659C: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:324)
==37517==    by 0x497531: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (jsinterp.cpp:509)
==37517==    by 0x418F54: JS_ExecuteScript (jsapi.cpp:5689)
==37517==    by 0x4088DC: Process(JSContext*, JSObject*, char const*, bool) (js.cpp:439)
==37517==    by 0x40BB53: Shell(JSContext*, js::cli::OptionParser*, char**) (js.cpp:4711)
==37517==    by 0x40C205: main (js.cpp:4955)
==37517==  Address 0x0 is not stack'd, malloc'd or (recently) free'd


This looks like a null-deref but the test has a mandatory gc() call in it, so marking s-s until investigated.
Blocks: IonFuzz
Keywords: assertioncrash
Whiteboard: [jsbugmon:update]
Jan, I'm not sure how to proceed with this bug.

We generate an idempotent GetPropertyCacheT. The unary types are in=Object, out=Object. There is no barrier, and the typeset for Regexp.prototype.test is "missing". Later, ion::GetPropertyCache runs, but does not call TypeScript::Monitor because the cache is idempotent. It returns an "undefined", but the code is compiled expecting an Object, so we crash.

I don't totally understand the gc() interaction, but it seems to throw out some of the typesets. Anyway, the gc itself is not what makes this security critical, but the fact that an integer could be shoved into an object pointer.

As I understand it, this is a situation where we must call TypeScript::Monitor, but we can't do that from idempotent ICs, and the idempotency is important for performance. Is the bug here that we're missing a barrier, or that we actually do need to call TypeScript::Monitor?
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1:fx18]
Here's a slightly simpler testcase that winds up passing UndefinedValue to LCallGeneric:

function whoo() {
  (new Object()).foo()
}
Object.prototype.foo = function() { return undefined };
whoo();
Object.prototype.foo = undefined;
gc();
whoo();

Can we just freeze on the type of that slot?
Adding security rating per comment 2.
(In reply to Sean Stangl from comment #3)
> Can we just freeze on the type of that slot?

Stack slot types are always frozen now. But that actually makes me wonder why there is no barrier.
TI accounts for properties on native objects, but assumes the property exists. If it's missing, the type ("undefined") has to be dynamically monitored. Idempotent caches do this by invalidating the cache.

It looks like the testcase here only fails if the property becomes "undefined", not if it's null or a primitive, so it seems like we also need to invalidate on "undefined" values. Digging deeper to find out what TI is doing here exactly and if this assumption makes sense.
What happens is that there's indeed an "undefined" barrier on the property read, but the GC destroys all this. After the GC, we try to reconstruct the property type set, hitting this edge case (jsinfer.h):

     *
     *    There is an exception for properties of singleton JS objects which
     *    are undefined at the point where the property was (lazily) generated.
     *    In such cases the property type set will remain empty, and the
     *    'undefined' type will only be added after a subsequent assignment or
     *    deletion. After these properties have been assigned a defined value,
     *    the only way they can become undefined again is after such an assign
     *    or deletion.
     *

The easiest fix is probably to just invalidate the idempotent cache if we see such a singleton object with an |undefined| property.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
(In reply to Jan de Mooij [:jandem] from comment #7)
> 
> The easiest fix is probably to just invalidate the idempotent cache if we
> see such a singleton object with an |undefined| property.

This works fine for the test in this bug, but the case I'm not sure about is that we attach a stub for shape S and later have a singleton object with the same shape S and unexpected "undefined" type. It seems like this would also bite JM+TI so it's probably not possible, but I want to look into it first.
Attached patch PatchSplinter Review
Not very happy with this patch, but in Brian's absence I'd just like to go with the least-complicated approach. I was worried about the case I mentioned in comment 8, but my many attempts today to break it like that failed. Also note that JM+TI would have the same problem if it were a problem.
Attachment #664170 - Flags: review?(dvander)
Comment on attachment 664170 [details] [diff] [review]
Patch

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

Heh.
Attachment #664170 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/640ffc5bc79c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update][ion:p1:fx18] → [jsbugmon:update][ion:p1:fx18][adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.