From 2e3ddf55b78782ee0db5fba538f2d051cc7adc3a Mon Sep 17 00:00:00 2001 From: "oliver@apple.com" Date: Sun, 17 Aug 2008 23:38:36 +0000 Subject: [PATCH] In Gmail, a crash occurs at KJS::Machine::privateExecute() when applying list styling to text after a quote had been removed Reviewed by Cameron Zwarich. This crash was caused by "depth()" incorrectly determining the scope depth of a 0 depth function without a full scope chain. Because such a function would not have an activation the depth function would return the scope depth of the parent frame, thus triggering an incorrect unwind. Any subsequent look up that walked the scope chain would result in incorrect behaviour, leading to a crash or incorrect variable resolution. This can only actually happen in try...finally statements as that's the only path that can result in the need to unwind the scope chain, but not force the function to need a full scope chain. git-svn-id: https://svn.webkit.org/repository/webkit/trunk@35812 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- JavaScriptCore/ChangeLog | 23 ++++++++++++++++++++++ JavaScriptCore/VM/Machine.cpp | 6 ++++-- LayoutTests/ChangeLog | 13 ++++++++++++ .../exception-try-finally-scope-error-expected.txt | 10 ++++++++++ .../fast/js/exception-try-finally-scope-error.html | 13 ++++++++++++ .../resources/exception-try-finally-scope-error.js | 10 ++++++++++ 6 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 LayoutTests/fast/js/exception-try-finally-scope-error-expected.txt create mode 100644 LayoutTests/fast/js/exception-try-finally-scope-error.html create mode 100644 LayoutTests/fast/js/resources/exception-try-finally-scope-error.js diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog index 431a961..4f6e8a9 100644 --- a/JavaScriptCore/ChangeLog +++ b/JavaScriptCore/ChangeLog @@ -1,3 +1,26 @@ +2008-08-17 Oliver Hunt + + Reviewed by Cameron Zwarich. + + In Gmail, a crash occurs at KJS::Machine::privateExecute() when applying list styling to text after a quote had been removed + + + This crash was caused by "depth()" incorrectly determining the scope depth + of a 0 depth function without a full scope chain. Because such a function + would not have an activation the depth function would return the scope depth + of the parent frame, thus triggering an incorrect unwind. Any subsequent + look up that walked the scope chain would result in incorrect behaviour, + leading to a crash or incorrect variable resolution. This can only actually + happen in try...finally statements as that's the only path that can result in + the need to unwind the scope chain, but not force the function to need a + full scope chain. + + The fix is simply to check for this case before attempting to walk the scope chain. + + * VM/Machine.cpp: + (KJS::depth): + (KJS::Machine::throwException): + 2008-08-17 Cameron Zwarich Reviewed by Maciej. diff --git a/JavaScriptCore/VM/Machine.cpp b/JavaScriptCore/VM/Machine.cpp index 915fca8e..5ae33c6 100644 --- a/JavaScriptCore/VM/Machine.cpp +++ b/JavaScriptCore/VM/Machine.cpp @@ -82,8 +82,10 @@ static void* op_call_indirect; #endif // Returns the depth of the scope chain within a given call frame. -static int depth(ScopeChain& sc) +static int depth(CodeBlock* codeBlock, ScopeChain& sc) { + if (!codeBlock->needsFullScopeChain) + return 0; int scopeDepth = 0; ScopeChainIterator iter = sc.begin(); ScopeChainIterator end = sc.end(); @@ -740,7 +742,7 @@ NEVER_INLINE Instruction* Machine::throwException(ExecState* exec, JSValue*& exc // Now unwind the scope chain within the exception handler's call frame. ScopeChain sc(scopeChain); - int scopeDelta = depth(sc) - scopeDepth; + int scopeDelta = depth(codeBlock, sc) - scopeDepth; ASSERT(scopeDelta >= 0); while (scopeDelta--) sc.pop(); diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 2685add..f05a460 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,3 +1,16 @@ +2008-08-17 Oliver Hunt + + Reviewed by Cameron Zwarich. + + In Gmail, a crash occurs at KJS::Machine::privateExecute() when applying list styling to text after a quote had been removed + + + Testcases for determining scope chain depth in a function with out a full scope chain. + + * fast/js/exception-try-finally-scope-error-expected.txt: Added. + * fast/js/exception-try-finally-scope-error.html: Added. + * fast/js/resources/exception-try-finally-scope-error.js: Added. + 2008-08-15 Dan Bernstein Rubber-stamped by Tim Hatcher. diff --git a/LayoutTests/fast/js/exception-try-finally-scope-error-expected.txt b/LayoutTests/fast/js/exception-try-finally-scope-error-expected.txt new file mode 100644 index 0000000..0b9bfa0 --- /dev/null +++ b/LayoutTests/fast/js/exception-try-finally-scope-error-expected.txt @@ -0,0 +1,10 @@ +This test makes sure stack unwinding works correctly when confronted with a 0-depth scope chain without an activation + +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". + + +PASS result is "inner scope" +PASS successfullyParsed is true + +TEST COMPLETE + diff --git a/LayoutTests/fast/js/exception-try-finally-scope-error.html b/LayoutTests/fast/js/exception-try-finally-scope-error.html new file mode 100644 index 0000000..912332a --- /dev/null +++ b/LayoutTests/fast/js/exception-try-finally-scope-error.html @@ -0,0 +1,13 @@ + + + + + + + +

+
+ + + + diff --git a/LayoutTests/fast/js/resources/exception-try-finally-scope-error.js b/LayoutTests/fast/js/resources/exception-try-finally-scope-error.js new file mode 100644 index 0000000..0ad889f --- /dev/null +++ b/LayoutTests/fast/js/resources/exception-try-finally-scope-error.js @@ -0,0 +1,10 @@ +description('This test makes sure stack unwinding works correctly when confronted with a 0-depth scope chain without an activation'); +var result; +function runTest() { + var test = "outer scope"; + with({test:"inner scope"}) + (function () { try { throw ""; } finally { result = test; shouldBe("result", '"inner scope"'); return;}})() +} +runTest(); + +var successfullyParsed = true; -- 1.8.3.1