Bugzilla@Mozilla – Bug 558531
Typo in JS_ResolveStandardClass causes built-in prototypes to be overwritten
Last modified: 2011-03-29 19:23:57 PDT
Summon comment box
function f2(c) { return { g: c.match(/:/) & c.match(/:/) & c.match(/f/) & c.m & c.a & c.match(/f/) & c.match(/c/) & c.match(/a/) & (/s/) & c.match(/e/) & c.match(/s/) & (c.match(/./) & c.i) & (c.match(/\*/)) & (c.match(/n/)), y: c.a & c.a & c.match(/\)/) & c.match(/\)/) & c.match(/\)/) & c.match(/\\/), c: c.match(/f/) } } function f1(c) { t = f2(c.replace(/s/)) try { eval(c) } catch(e) {} f4(); if (c.indexOf("<") == -1 || c.indexOf()) try {} catch(e) {} try { try { l } catch(e) {} if ("unwatch" in this) {} g } catch(e) {} try {} catch(p) {} } function f4() { try {} catch(e) {} { try { eval(s + "") } catch(e) {} } } [{}] s = [{},{}]; (function(){}()) s[{},{},{}] = [function(){}] a = [{},{},{}].concat([{},{}]) f1("") f1("") f1("n") f1("") f1("(__proto__=null)") f1("for(var z=0;z<1;z++){gc()(*::*)}") f1("gc()") f1("<") crashes js debug shell on JM changeset 0b7f385f3f59 with -m at 0xdadadade with js::methodjit::JaegerShot near the top of the stack. I tested this on 32-bit Mac debug build off changeset 0b7f385f3f59 because tip didn't compile for me. :( (Not exactly the smallest) regression window: http://hg.mozilla.org/users/danderson_mozilla.com/jaegermonkey/pushloghtml?fromchange=0075ab7ae8fe&tochange=0b7f385f3f59 Stack: Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x00000000dadadade Crashed Thread: 0 Dispatch queue: com.apple.main-thread Thread 0 Crashed: Dispatch queue: com.apple.main-thread 0 ??? 0x003ec4b9 0 + 4113593 1 js-dbg-32-jm-darwin 0x001e2e82 js::methodjit::JaegerShot(JSContext*) + 408 2 js-dbg-32-jm-darwin 0x000a0f04 js_RunScript + 158 3 js-dbg-32-jm-darwin 0x000a1453 js_Execute + 1253 4 js-dbg-32-jm-darwin 0x00012255 JS_ExecuteScript + 54 5 js-dbg-32-jm-darwin 0x0000aaf5 Process(JSContext*, JSObject*, char*, int) + 458 (js.cpp:450) 6 js-dbg-32-jm-darwin 0x0000b862 ProcessArgs(JSContext*, JSObject*, char**, int) + 2326 (js.cpp:870) 7 js-dbg-32-jm-darwin 0x0000bc2f main + 953 (js.cpp:4975) 8 js-dbg-32-jm-darwin 0x00002881 _start + 208 9 js-dbg-32-jm-darwin 0x000027b0 start + 40
Reproducible with JM tip.
The cause: For the |indexOf| operations on line 17, we bake in the identity of the String class prototype. Then, GC apparently collects it and we start navigating through an invalid object the next time we use the PIC on line 17. This could be solved by invalidating any primitive-string PICs on every GC. That's painful, though. It seems like it would be better to bake in the pointer to the string method, and then have some guard or invalidation to detect monkey-patching of String.prototype over standard methods.
This seems to causing most of the current js::methodjit::JaegerShot crashes in JM now, when running jsfunfuzz. :(
OK, I'll bump up the priority on this. I do want to finish an investigation of the micro-level perf of the PICs first.
Created attachment 440080 [details] [review] Patch The bug is in jsapi.cpp, so I want to land the fix on TM. It's pretty clear the bug is just a typo. My earlier guess that |String.prototype| was getting GC'd was correct. The detailed cause is this: 1. At some point, String.prototype is initialized and stored in slot 12 of the global object, which is its normal home. 2. Evaluating |"unwatch" in this| calls the global resolver hook. This in turn calls JS_ResolveStandardClass. JS_ResolveStandardClass finds the right entry, but due to a typo it reads out the wrong name element, which causes it to call the init function for the string standard class. This overwrites the global slot where String.prototype is stored. 3. A GC gets run, and the old String.prototype gets collected. 4. A JM PIC stub that assumes that String.prototype cannot change (per ECMA-262 5e, 15.5.3.1) crashes.
Comment on attachment 440080 [details] [review] Patch Goes back to 2006 at least, definitely in stable releases with TM, guessing probably triggerable somehow...should be fixed on branches too.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsapi.c&rev=3.453 http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/js/src&command=DIFF_FRAMESET&file=jsapi.c&rev2=3.266&rev1=3.265
http://hg.mozilla.org/tracemonkey?pushloghtml
http://hg.mozilla.org/mozilla-central/rev/d0e7da895292
Nominating for branches based on comment 6. Can't reproduce the crash using the given testcase but the code has the same problem.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d1179bed506f http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cc28ca4d7ca2
(In reply to comment #10) > Nominating for branches based on comment 6. Can't reproduce the crash using the > given testcase but the code has the same problem. No way for QA to verify this fix in the branch releases.