The following testcase crashes on mozilla-central revision b6acf4d4fc20 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2): var s = ''; for (var i = 0; i < 70000; i++) s += 'function x' + i + '() { x' + i + '(); }\n'; eval("(function() { " + s + " })();"); Backtrace: Program received signal SIGSEGV, Segmentation fault. 0x0000000000467d12 in js::SetAliasedVarOperation (checkLexical=js::CheckLexical, val=..., sc=..., obj=..., pc=0x7fffef2b1ff1 "\211", script=0x7ffff7e6e300, cx=0x7ffff6907800) at js/src/vm/Interpreter-inl.h:245 #0 0x0000000000467d12 in js::SetAliasedVarOperation (checkLexical=js::CheckLexical, val=..., sc=..., obj=..., pc=0x7fffef2b1ff1 "\211", script=0x7ffff7e6e300, cx=0x7ffff6907800) at js/src/vm/Interpreter-inl.h:245 #1 0x0000000000ab3fc5 in SetAliasedVarOperation (checkLexical=js::CheckLexical, val=..., sc=..., obj=..., pc=0x7fffef2b1ff1 "\211", script=0x7ffff7e6e300, cx=0x7ffff6907800) at js/src/vm/Interpreter-inl.h:245 #2 Interpret (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:3166 #3 0x0000000000ab8828 in js::RunScript (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:428 #4 0x0000000000aba373 in js::ExecuteKernel (cx=cx@entry=0x7ffff6907800, script=..., script@entry=..., scopeChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=<optimized out>) at js/src/vm/Interpreter.cpp:684 #5 0x00000000005e6e7d in EvalKernel (cx=cx@entry=0x7ffff6907800, args=..., evalType=evalType@entry=DIRECT_EVAL, caller=..., scopeobj=..., scopeobj@entry=..., pc=<optimized out>) at js/src/builtin/Eval.cpp:332 #6 0x00000000005e7806 in js::DirectEval (cx=cx@entry=0x7ffff6907800, args=...) at js/src/builtin/Eval.cpp:439 #7 0x00000000006152bf in js::jit::DoCallFallback (cx=0x7ffff6907800, frame=0x7fffffffc548, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffc4f8, res=...) at js/src/jit/BaselineIC.cpp:6125 #8 0x00007ffff7ff1abf in ?? () [...] #30 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x7ffff6907800 140737330051072 rcx 0x7ffff6ca588d 140737333844109 rdx 0x0 0 rsi 0x7ffff6f7a9d0 140737336814032 rdi 0x7ffff6f791c0 140737336807872 rbp 0x7fffffffb080 140737488334976 rsp 0x7fffffffb080 140737488334976 r8 0x7ffff7fdf7c0 140737354004416 r9 0x6372732f736a2f6c 7165916604736876396 r10 0x7fffffffae40 140737488334400 r11 0x7ffff6c27ee0 140737333329632 r12 0x7ffff7e69100 140737352470784 r13 0x7ffff6907830 140737330051120 r14 0x1be1d20 29236512 r15 0x4000000 67108864 rip 0x467d12 <js::SetAliasedVarOperation(js::MaybeCheckLexical, JS::Value const&, js::ScopeCoordinate, js::ScopeObject&, jsbytecode*, JSScript*, JSContext*)+28> => 0x467d12 <js::SetAliasedVarOperation(js::MaybeCheckLexical, JS::Value const&, js::ScopeCoordinate, js::ScopeObject&, jsbytecode*, JSScript*, JSContext*)+28>: movl $0xf5,0x0 0x467d1d <js::SetAliasedVarOperation(js::MaybeCheckLexical, JS::Value const&, js::ScopeCoordinate, js::ScopeObject&, jsbytecode*, JSScript*, JSContext*)+39>: callq 0x4a6f30 <abort()> Note: This test might run up to 20 seconds. I'm marking this s-s until investigated because I remember that at some point we had a security bug with this assertion.

JSBugMon: Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/0fd815999686 user: Shu-yu Guo date: Wed Oct 29 19:41:42 2014 -0700 summary: Bug 1089761 - Fix initializing lexicals to throw on touch on CallObject. (r=jandem,Waldo) This iteration took 181.768 seconds to run.

I haven't had time to track this down yet, but I can report that it happens when initializing the 65534th (or thereabouts) function, and the deal is, the CallObject's slots were only initialized to UndefinedValue from slot 0 to slot 65535 (inclusive). The next slot is still poisoned. Not sure if this is security-sensitive.

I don't think this is s-s. The worst that can happen is we crash on accessing a magic value as a pointer, in this case, JS_UNINITIALIZED_LEXICAL, which is 0x10 or something.

If I'm reading the code right, on little-endian systems Value is going to be { JSWhyMagic; uint32_t tag }. The latter is going to be a constant. The former is also a constant. If we access the overall thing as a pointer, it's going to be a constant pointer. But the combo of the two isn't going to produce a low-valued pointer, so *in theory* the overall value could be addressable memory. Right? Unless we *know* that overall combo, interpreted as a pointer, is low, I don't think we're quite out of the woods here.

Comment on attachment 8731910 [details] [diff] [review] Make aliasedBodyLevelLexicalBegin a uint32. Review of attachment 8731910 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/parser/bug-1254164.js @@ +1,1 @@ > +// |jit-test| slow; How slow is this? I wouldn't expect a function with 70k nested functions would take that long to parse/execute. I'd say you should be hitting at least 30s or so in a debug build (if not more) before applying this. ::: js/src/jsscript.cpp @@ +130,5 @@ > // aliased variables. While the debugger may observe any scope object at > // any time, such accesses are mediated by DebugScopeProxy (see > // DebugScopeProxy::handleUnaliasedAccess). > uint32_t nslots = CallObject::RESERVED_SLOTS; > + uint32_t aliasedBodyLevelLexicalBegin = LOCALNO_LIMIT; A comment like "// an impossible slot number, such that no lexicals will be recognized" or something would be nice here, referring to the frontend's restriction on the number of locals in a function, and referring to initAliasedLexicalsToThrowOnTouch, would be nice. Not sure if comment should be in this, or in a separate patch. I might go for separate patch, for paranoia. ::: js/src/jsscript.h @@ +231,2 @@ > uint16_t numUnaliasedBodyLevelLexicals_; > + uint32_t aliasedBodyLevelLexicalBegin_; Not here, but we should probably reorganize these from biggest to smallest type at some point. Given we had five 16-bits in a row, this shouldn't throw off existing alignment at all, I'll assume. @@ +357,5 @@ > }; > > +// If this fails, add/remove padding within Bindings. > +static_assert(sizeof(Bindings) % js::gc::CellSize == 0, > + "Size of Bindings must be an integral multiple of js::gc::CellSize"); \o/ and /o\ we didn't have this earlier.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6) > Comment on attachment 8731910 [details] [diff] [review] > Make aliasedBodyLevelLexicalBegin a uint32. > > Review of attachment 8731910 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit-test/tests/parser/bug-1254164.js > @@ +1,1 @@ > > +// |jit-test| slow; > > How slow is this? I wouldn't expect a function with 70k nested functions > would take that long to parse/execute. I'd say you should be hitting at > least 30s or so in a debug build (if not more) before applying this. > On what machine though? It's less than 30s on mine but it'll be worse on our CI instances. > ::: js/src/jsscript.cpp > @@ +130,5 @@ > > // aliased variables. While the debugger may observe any scope object at > > // any time, such accesses are mediated by DebugScopeProxy (see > > // DebugScopeProxy::handleUnaliasedAccess). > > uint32_t nslots = CallObject::RESERVED_SLOTS; > > + uint32_t aliasedBodyLevelLexicalBegin = LOCALNO_LIMIT; > > A comment like "// an impossible slot number, such that no lexicals will be > recognized" or something would be nice here, referring to the frontend's > restriction on the number of locals in a function, and referring to > initAliasedLexicalsToThrowOnTouch, would be nice. Not sure if comment > should be in this, or in a separate patch. I might go for separate patch, > for paranoia. The sentinel logic is already there, just the sentinel was wrong. I think the comment is fine. > > ::: js/src/jsscript.h > @@ +231,2 @@ > > uint16_t numUnaliasedBodyLevelLexicals_; > > + uint32_t aliasedBodyLevelLexicalBegin_; > > Not here, but we should probably reorganize these from biggest to smallest > type at some point. Given we had five 16-bits in a row, this shouldn't > throw off existing alignment at all, I'll assume. > > @@ +357,5 @@ > > }; > > > > +// If this fails, add/remove padding within Bindings. > > +static_assert(sizeof(Bindings) % js::gc::CellSize == 0, > > + "Size of Bindings must be an integral multiple of js::gc::CellSize"); > > \o/ and /o\ we didn't have this earlier. Yeah :(

Comment on attachment 8731910 [details] [diff] [review] Make aliasedBodyLevelLexicalBegin a uint32. [Security approval request comment] How easily could an exploit be constructed based on the patch? Don't know. I don't think it's exploitable by itself but could be used as a widget maybe? Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? I'm unclear on whether it's a security problem. But the patch itself makes clear what the problem is. Which older supported branches are affected by this flaw? All branches. If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No, and haven't looked. Should be easy. How likely is this patch to cause regressions; how much testing does it need? Very unlikely to cause regressions.

Thank you for stealing.

Sec-approval+ for trunk. We'll want Aurora and Beta patches made and nominated for sure. We'll probably want patches on both of the ESR branches as well.

JSBugMon: This bug has been automatically verified fixed.

Comment on attachment 8731910 [details] [diff] [review] Make aliasedBodyLevelLexicalBegin a uint32. Approval Request Comment [Feature/regressing bug #]: 1089761 [User impact if declined]: Crashes when trying to create scripts with more than 2^15 block-local bindings. [Describe test coverage new/current, TreeHerder]: on TH. [Risks and why]: low, bug fix. [String/UUID change made/needed]: none.

Shu, could you also nominate the patch for uplift to esr38 and esr45? Thanks!

Comment on attachment 8731910 [details] [diff] [review] Make aliasedBodyLevelLexicalBegin a uint32. Crash fix, sec-high, please uplift to aurora and beta.

JSBugMon: This bug has been automatically verified fixed on Fx47

Comment on attachment 8731910 [details] [diff] [review] Make aliasedBodyLevelLexicalBegin a uint32. [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: see comment 8 Fix Landed on Version: nightly, aurora, beta Risk to taking this patch (and alternatives if risky): see comment 8 String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment on attachment 8731910 [details] [diff] [review] Make aliasedBodyLevelLexicalBegin a uint32. Sec-high fix that also needs to land on ESR branches.

has problems uplifting to esr38 grafting 336841:aa0b35910b3a "Bug 1254164 - Make aliasedBodyLevelLexicalBegin a uint32. r=Waldo, a=lizzard" merging js/src/jsscript.cpp merging js/src/jsscript.h warning: conflicts while merging js/src/jsscript.h! (edit, then use 'hg resolve --mark')

https://hg.mozilla.org/releases/mozilla-esr45/rev/1883abc8ca83

Patch for ESR38 posted.

https://hg.mozilla.org/releases/mozilla-esr38/rev/3d2b62083a6a thanks Shu-yu