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.

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?

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.

(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.

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.

Comment on attachment 664170 [details] [diff] [review] Patch Review of attachment 664170 [details] [diff] [review]: ----------------------------------------------------------------- Heh.

JSBugMon: This bug has been automatically verified fixed.