<https://bugs.webkit.org/show_bug.cgi?id=23049> [jsfunfuzz] With blocks do not correc...

<rdar://problem/6469742> Crash in JSC::TypeInfo::hasStandardGetOwnPropertySlot() running jsfunfuzz

Reviewed by Darin Adler

The problem that caused this was that with nodes were not correctly protecting
the final object that was placed in the scope chain.  We correct this by forcing
the use of a temporary register (which stops us relying on a local register
protecting the scope) and changing the behaviour of op_push_scope so that it
will store the final scope object.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@39524 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog
index 52d93e7..55ee52b 100644 (file)
--- a/JavaScriptCore/ChangeLog
+++ b/JavaScriptCore/ChangeLog
@@ -1,3 +1,27 @@
+2008-12-30  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Darin Adler.
+
+        <https://bugs.webkit.org/show_bug.cgi?id=23049> [jsfunfuzz] With blocks do not correctly protect their scope object
+        <rdar://problem/6469742> Crash in JSC::TypeInfo::hasStandardGetOwnPropertySlot() running jsfunfuzz
+
+        The problem that caused this was that with nodes were not correctly protecting
+        the final object that was placed in the scope chain.  We correct this by forcing
+        the use of a temporary register (which stops us relying on a local register
+        protecting the scope) and changing the behaviour of op_push_scope so that it
+        will store the final scope object.
+
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::emitPushScope):
+        * interpreter/Interpreter.cpp:
+        (JSC::Interpreter::privateExecute):
+        (JSC::Interpreter::cti_op_push_scope):
+        * interpreter/Interpreter.h:
+        * jit/JIT.cpp:
+        (JSC::JIT::privateCompileMainPass):
+        * parser/Nodes.cpp:
+        (JSC::WithNode::emitBytecode):
+
 2008-12-30  Cameron Zwarich  <cwzwarich@uwaterloo.ca>
 
         Reviewed by Sam Weinig.

diff --git a/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp b/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
index b3dcdd2..7409cfe 100644 (file)
--- a/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
+++ b/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
@@ -1395,6 +1395,7 @@ RegisterID* BytecodeGenerator::emitConstruct(RegisterID* dst, RegisterID* func,
 
 RegisterID* BytecodeGenerator::emitPushScope(RegisterID* scope)
 {
+    ASSERT(scope->isTemporary());
     ControlFlowContext context;
     context.isFinallyBlock = false;
     m_scopeContextStack.append(context);

diff --git a/JavaScriptCore/interpreter/Interpreter.cpp b/JavaScriptCore/interpreter/Interpreter.cpp
index 5caffc3..e93cd3f 100644 (file)
--- a/JavaScriptCore/interpreter/Interpreter.cpp
+++ b/JavaScriptCore/interpreter/Interpreter.cpp
@@ -3639,13 +3639,15 @@ JSValue* Interpreter::privateExecute(ExecutionFlag flag, RegisterFile* registerF
         /* push_scope scope(r)
 
            Converts register scope to object, and pushes it onto the top
-           of the current scope chain.
+           of the current scope chain.  The contents of the register scope
+           are replaced by the result of toObject conversion of the scope.
         */
         int scope = (++vPC)->u.operand;
         JSValue* v = callFrame[scope].jsValue(callFrame);
         JSObject* o = v->toObject(callFrame);
         CHECK_FOR_EXCEPTION();
 
+        callFrame[scope] = o;
         callFrame->setScopeChain(callFrame->scopeChain()->push(o));
 
         ++vPC;
@@ -5738,13 +5740,14 @@ JSValue* Interpreter::cti_op_next_pname(STUB_ARGS)
     return temp;
 }
 
-void Interpreter::cti_op_push_scope(STUB_ARGS)
+JSObject* Interpreter::cti_op_push_scope(STUB_ARGS)
 {
     BEGIN_STUB_FUNCTION();
 
     JSObject* o = ARG_src1->toObject(ARG_callFrame);
-    CHECK_FOR_EXCEPTION_VOID();
+    CHECK_FOR_EXCEPTION();
     ARG_callFrame->setScopeChain(ARG_callFrame->scopeChain()->push(o));
+    return o;
 }
 
 void Interpreter::cti_op_pop_scope(STUB_ARGS)

diff --git a/JavaScriptCore/interpreter/Interpreter.h b/JavaScriptCore/interpreter/Interpreter.h
index 8b71bd0..f90b23b 100644 (file)
--- a/JavaScriptCore/interpreter/Interpreter.h
+++ b/JavaScriptCore/interpreter/Interpreter.h
@@ -255,7 +255,7 @@ namespace JSC {
         static JSValue* JIT_STUB cti_op_throw(STUB_ARGS);
         static JSPropertyNameIterator* JIT_STUB cti_op_get_pnames(STUB_ARGS);
         static JSValue* JIT_STUB cti_op_next_pname(STUB_ARGS);
-        static void JIT_STUB cti_op_push_scope(STUB_ARGS);
+        static JSObject* JIT_STUB cti_op_push_scope(STUB_ARGS);
         static void JIT_STUB cti_op_pop_scope(STUB_ARGS);
         static JSValue* JIT_STUB cti_op_typeof(STUB_ARGS);
         static JSValue* JIT_STUB cti_op_is_undefined(STUB_ARGS);

diff --git a/JavaScriptCore/jit/JIT.cpp b/JavaScriptCore/jit/JIT.cpp
index 30ea338..3573f6a 100644 (file)
--- a/JavaScriptCore/jit/JIT.cpp
+++ b/JavaScriptCore/jit/JIT.cpp
@@ -1038,6 +1038,7 @@ void JIT::privateCompileMainPass()
         case op_push_scope: {
             emitPutJITStubArgFromVirtualRegister(currentInstruction[1].u.operand, 1, X86::ecx);
             emitCTICall(Interpreter::cti_op_push_scope);
+            emitPutVirtualRegister(currentInstruction[1].u.operand);
             NEXT_OPCODE(op_push_scope);
         }
         case op_pop_scope: {

diff --git a/JavaScriptCore/parser/Nodes.cpp b/JavaScriptCore/parser/Nodes.cpp
index bb88482..4d4406d 100644 (file)
--- a/JavaScriptCore/parser/Nodes.cpp
+++ b/JavaScriptCore/parser/Nodes.cpp
@@ -2038,7 +2038,8 @@ void WithNode::releaseNodes(NodeReleaser& releaser)
 
 RegisterID* WithNode::emitBytecode(BytecodeGenerator& generator, RegisterID* dst)
 {
-    RefPtr<RegisterID> scope = generator.emitNode(m_expr.get()); // scope must be protected until popped
+    RefPtr<RegisterID> scope = generator.newTemporary();
+    generator.emitNode(scope.get(), m_expr.get()); // scope must be protected until popped
     generator.emitExpressionInfo(m_divot, m_expressionLength, 0);
     generator.emitPushScope(scope.get());
     RegisterID* result = generator.emitNode(dst, m_statement.get());

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 7325bbb..0b598a3 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2008-12-30  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Darin Adler.
+
+        <https://bugs.webkit.org/show_bug.cgi?id=23049> [jsfunfuzz] With blocks do not correctly protect their scope object
+        <rdar://problem/6469742> Crash in JSC::TypeInfo::hasStandardGetOwnPropertySlot() running jsfunfuzz
+
+        Tests to ensure we correctly protect the scope object from GC.
+
+        * fast/js/resources/with-scope-gc.js: Added.
+        * fast/js/with-scope-gc-expected.txt: Added.
+        * fast/js/with-scope-gc.html: Added.
+
 2008-12-30  Simon Fraser  <simon.fraser@apple.com>
 
         Fix spurious test failures by rounding floating point values to

diff --git a/LayoutTests/fast/js/resources/with-scope-gc.js b/LayoutTests/fast/js/resources/with-scope-gc.js
new file mode 100644 (file)
index 0000000..82ca1cf
--- /dev/null
+++ b/LayoutTests/fast/js/resources/with-scope-gc.js
@@ -0,0 +1,49 @@
+description(
+'Tests to make sure that dynamic scope objects are correctly protected from GC.  To pass we need to not crash.'
+);
+
+function gc()
+{
+    if (this.GCController)
+        GCController.collect();
+    else
+        for (var i = 0; i < 10000; ++i) // Allocate a sufficient number of objects to force a GC.
+            ({});
+}
+
+(function() {
+    try {
+        // Immediate value for scope
+        with(1) { gc(); a; }
+    } catch(e) {
+    }
+})();
+
+(function() {
+    try {
+        // Real object for scope
+        var local;
+        with (local = {}) {
+            z=null;
+            {}; {}; [1,2,3,4*{}]; // Clobber any temporaries the scope may exist in
+            gc(); 
+            b;
+        }
+    } catch (e) {
+    }
+})();
+
+(function() {
+    try {
+        // Test catch blocks for the heck of it
+        try {
+            throw 1;
+        } catch(e) {
+            gc();
+            b;
+        }
+    } catch (e) {
+    }
+})();
+
+var successfullyParsed = true;

diff --git a/LayoutTests/fast/js/with-scope-gc-expected.txt b/LayoutTests/fast/js/with-scope-gc-expected.txt
new file mode 100644 (file)
index 0000000..38cc3cf
--- /dev/null
+++ b/LayoutTests/fast/js/with-scope-gc-expected.txt
@@ -0,0 +1,9 @@
+Tests to make sure that dynamic scope objects are correctly protected from GC. To pass we need to not crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

diff --git a/LayoutTests/fast/js/with-scope-gc.html b/LayoutTests/fast/js/with-scope-gc.html
new file mode 100644 (file)
index 0000000..248806c
--- /dev/null
+++ b/LayoutTests/fast/js/with-scope-gc.html
@@ -0,0 +1,13 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<link rel="stylesheet" href="resources/js-test-style.css">
+<script src="resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script src="resources/with-scope-gc.js"></script>
+<script src="resources/js-test-post.js"></script>
+</body>
+</html>