Regression(r107058): Use-after-free in SerializedScriptValue::deserialize
authorharaken@chromium.org <haraken@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 Jan 2013 00:40:50 +0000 (00:40 +0000)
committerharaken@chromium.org <haraken@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 Jan 2013 00:40:50 +0000 (00:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=107792

Reviewed by Abhishek Arya.

Source/WebCore:

Imagine the following call path:

(1) history.state is accessed.
(2) V8History::stateAccessorGetter() calls History::state(), which calls
HistoryItem::stateObject().
(3) HistoryItem holds m_stateObject as RefPtr<SerializedScriptValue>,
but HistoryItem::stateObject() returns SerializedScriptValue*.
(4) V8History::stateAccessorGetter calls SerializedScriptValue::deserialize()
for the SerializedScriptValue* obtained in (3).
(5) SerializedScriptValue::deserialize() can call history.replaceState()
in its deserialization process (See the test case in the Chromium bug).
(6) history.replaceState() replaces HistoryItem::m_stateObject.
This replacement destructs the original HistoryItem::m_stateObject.
(7) The current deserialization process can crash due to the premature destruction.

To avoid the problem, we have to pass PassRefPtr<SerializedScriptValue> around
instead of SerializedScriptValue*.

Test: fast/history/replacestate-nocrash.html

* bindings/v8/custom/V8HistoryCustom.cpp:
(WebCore::V8History::stateAccessorGetter):
* history/HistoryItem.h:
(WebCore):
(WebCore::HistoryItem::stateObject):
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadInSameDocument):
* loader/FrameLoader.h:
* page/History.cpp:
(WebCore::History::state):
(WebCore::History::stateInternal):
* page/History.h:
(History):

LayoutTests:

Added a test that demonstrated a crash due to use-after-free
of SerializedScriptValue.

Test: fast/history/replacestate-nocrash.html

* fast/history/replacestate-nocrash-expected.txt: Added.
* fast/history/replacestate-nocrash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/history/replacestate-nocrash-expected.txt [new file with mode: 0644]
LayoutTests/fast/history/replacestate-nocrash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/bindings/v8/custom/V8HistoryCustom.cpp
Source/WebCore/history/HistoryItem.h
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoader.h
Source/WebCore/page/History.cpp
Source/WebCore/page/History.h

index 58a106f..03f511c 100644 (file)
@@ -1,3 +1,18 @@
+2013-01-24  Kentaro Hara  <haraken@chromium.org>
+
+        Regression(r107058): Use-after-free in SerializedScriptValue::deserialize
+        https://bugs.webkit.org/show_bug.cgi?id=107792
+
+        Reviewed by Abhishek Arya.
+
+        Added a test that demonstrated a crash due to use-after-free
+        of SerializedScriptValue.
+
+        Test: fast/history/replacestate-nocrash.html
+
+        * fast/history/replacestate-nocrash-expected.txt: Added.
+        * fast/history/replacestate-nocrash.html: Added.
+
 2013-01-24  Arko Saha  <arko@motorola.com>
 
         Microdata: itemtype attribute must update correctly on adding or removing tokens
diff --git a/LayoutTests/fast/history/replacestate-nocrash-expected.txt b/LayoutTests/fast/history/replacestate-nocrash-expected.txt
new file mode 100644 (file)
index 0000000..0383162
--- /dev/null
@@ -0,0 +1 @@
+Test passes if it does not crash.
diff --git a/LayoutTests/fast/history/replacestate-nocrash.html b/LayoutTests/fast/history/replacestate-nocrash.html
new file mode 100644 (file)
index 0000000..5c85e70
--- /dev/null
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html>
+Test passes if it does not crash.
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+Object.prototype.__defineSetter__("foo",function(){history.replaceState("")});
+history.replaceState({foo:1,zzz:Array(1<<22).join("a")});
+history.state.length;
+</script>
+</html>
index 97dcce3..533a392 100644 (file)
@@ -1,3 +1,44 @@
+2013-01-24  Kentaro Hara  <haraken@chromium.org>
+
+        Regression(r107058): Use-after-free in SerializedScriptValue::deserialize
+        https://bugs.webkit.org/show_bug.cgi?id=107792
+
+        Reviewed by Abhishek Arya.
+
+        Imagine the following call path:
+
+        (1) history.state is accessed.
+        (2) V8History::stateAccessorGetter() calls History::state(), which calls
+        HistoryItem::stateObject().
+        (3) HistoryItem holds m_stateObject as RefPtr<SerializedScriptValue>,
+        but HistoryItem::stateObject() returns SerializedScriptValue*.
+        (4) V8History::stateAccessorGetter calls SerializedScriptValue::deserialize()
+        for the SerializedScriptValue* obtained in (3).
+        (5) SerializedScriptValue::deserialize() can call history.replaceState()
+        in its deserialization process (See the test case in the Chromium bug).
+        (6) history.replaceState() replaces HistoryItem::m_stateObject.
+        This replacement destructs the original HistoryItem::m_stateObject.
+        (7) The current deserialization process can crash due to the premature destruction.
+
+        To avoid the problem, we have to pass PassRefPtr<SerializedScriptValue> around
+        instead of SerializedScriptValue*.
+
+        Test: fast/history/replacestate-nocrash.html
+
+        * bindings/v8/custom/V8HistoryCustom.cpp:
+        (WebCore::V8History::stateAccessorGetter):
+        * history/HistoryItem.h:
+        (WebCore):
+        (WebCore::HistoryItem::stateObject):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadInSameDocument):
+        * loader/FrameLoader.h:
+        * page/History.cpp:
+        (WebCore::History::state):
+        (WebCore::History::stateInternal):
+        * page/History.h:
+        (History):
+
 2013-01-24  Beth Dakin  <bdakin@apple.com>
 
         Some formerly-fixed objects scroll as if they are still fixed
index 01ecdf0..8ed6904 100644 (file)
@@ -50,7 +50,7 @@ v8::Handle<v8::Value> V8History::stateAccessorGetter(v8::Local<v8::String> name,
     if (!value.IsEmpty() && !history->stateChanged())
         return value;
 
-    SerializedScriptValue* serialized = history->state();
+    RefPtr<SerializedScriptValue> serialized = history->state();
     value = serialized ? serialized->deserialize(info.GetIsolate()) : v8::Handle<v8::Value>(v8Null(info.GetIsolate()));
     info.Holder()->SetHiddenValue(V8HiddenPropertyName::state(), value);
 
index 79c43c9..5db4d97 100644 (file)
@@ -28,6 +28,7 @@
 #define HistoryItem_h
 
 #include "IntPoint.h"
+#include "SerializedScriptValue.h"
 #include <wtf/HashMap.h>
 #include <wtf/OwnPtr.h>
 #include <wtf/PassOwnPtr.h>
@@ -58,7 +59,6 @@ class HistoryItem;
 class Image;
 class KURL;
 class ResourceRequest;
-class SerializedScriptValue;
 
 typedef Vector<RefPtr<HistoryItem> > HistoryItemVector;
 
@@ -145,7 +145,7 @@ public:
     void setIsTargetItem(bool);
     
     void setStateObject(PassRefPtr<SerializedScriptValue> object);
-    SerializedScriptValue* stateObject() const { return m_stateObject.get(); }
+    PassRefPtr<SerializedScriptValue> stateObject() const { return m_stateObject; }
 
     void setItemSequenceNumber(long long number) { m_itemSequenceNumber = number; }
     long long itemSequenceNumber() const { return m_itemSequenceNumber; }
index 3691130..95b513b 100644 (file)
@@ -1003,7 +1003,7 @@ void FrameLoader::setFirstPartyForCookies(const KURL& url)
 
 // This does the same kind of work that didOpenURL does, except it relies on the fact
 // that a higher level already checked that the URLs match and the scrolling is the right thing to do.
-void FrameLoader::loadInSameDocument(const KURL& url, SerializedScriptValue* stateObject, bool isNewNavigation)
+void FrameLoader::loadInSameDocument(const KURL& url, PassRefPtr<SerializedScriptValue> stateObject, bool isNewNavigation)
 {
     // If we have a state object, we cannot also be a new navigation.
     ASSERT(!stateObject || (stateObject && !isNewNavigation));
index 6b59e8d..f329272 100644 (file)
@@ -359,7 +359,7 @@ private:
     void detachChildren();
     void closeAndRemoveChild(Frame*);
 
-    void loadInSameDocument(const KURL&, SerializedScriptValue* stateObject, bool isNewNavigation);
+    void loadInSameDocument(const KURL&, PassRefPtr<SerializedScriptValue> stateObject, bool isNewNavigation);
 
     void prepareForLoadStart();
     void provisionalLoadStarted();
index 66831c4..531434b 100644 (file)
@@ -55,13 +55,13 @@ unsigned History::length() const
     return m_frame->page()->backForward()->count();
 }
 
-SerializedScriptValue* History::state()
+PassRefPtr<SerializedScriptValue> History::state()
 {
     m_lastStateObjectRequested = stateInternal();
     return m_lastStateObjectRequested;
 }
 
-SerializedScriptValue* History::stateInternal() const
+PassRefPtr<SerializedScriptValue> History::stateInternal() const
 {
     if (!m_frame)
         return 0;
index d7c7a28..180907e 100644 (file)
@@ -45,7 +45,7 @@ public:
     static PassRefPtr<History> create(Frame* frame) { return adoptRef(new History(frame)); }
 
     unsigned length() const;
-    SerializedScriptValue* state();
+    PassRefPtr<SerializedScriptValue> state();
     void back();
     void forward();
     void go(int distance);
@@ -68,9 +68,9 @@ private:
 
     KURL urlForState(const String& url);
 
-    SerializedScriptValue* stateInternal() const;
+    PassRefPtr<SerializedScriptValue> stateInternal() const;
 
-    SerializedScriptValue* m_lastStateObjectRequested;
+    RefPtr<SerializedScriptValue> m_lastStateObjectRequested;
 };
 
 } // namespace WebCore