Heap-use-after-free read of size 4 in JavaScriptCore: WTF::StringImpl::isSymbol(...
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Jun 2015 17:36:16 +0000 (17:36 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Jun 2015 17:36:16 +0000 (17:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145532

Reviewed by Geoffrey Garen.

Source/JavaScriptCore:

AtomicStringImpl::lookUp returns AtomicStringImpl*,
it doesn't give any ownership to the caller.
Originally, this is ok because the ownership is taken
by AtomicStringImpl's table (& the register side).

But if we would like to use this returned AtomicStringImpl*,
we should take its ownership immediately.
Because if the register side releases its ownership (ref count),
it will be destroyed.

In JSString::toExistingAtomicString, it returns AtomicStringImpl*.
But it's not appropriate.
If the owner of AtomicStringImpl* is always JSString*, it is ok.
But it looks up the table-registered AtomicStringImpl* from
the AtomicStringImpl table. So JSString* may not have the ownership
of the returned AtomicStringImpl*.

The failure situation is the following.

1. A creates AtomicStringImpl. A has its ownership.
   And A registers it to AtomicStringImpl table.
2. JSString looks up the AtomicStringImpl from the table.
   It gets AtomicStringImpl*. And JSString doesn't have its ownership.
   It returns the raw pointer immediately to the users
3. A is released. There's no owner for AtomicStringImpl*.
   So it's also destroyed.
4. Use looked up AtomicStringImpl in (2). It becomes use-after-free.

This patch fixes it by the following changes.

1. Change the signature of `AtomicStringImpl* AtomicStringImpl::lookUp(...)`
   to `RefPtr<AtomicStringImpl> AtomicStringImpl::lookUp(..)`.
   Use `RefPtr` because it may return `nullptr`.
2. Change the signature of `AtomicStringImpl* JSString::toExistingAtomicString(...)`
   to `RefPtr<AtomicStringImpl> JSString::toExistingAtomicString(...)`.
   Using `RefPtr` is the same reason.
3. Receive the result with `RefPtr<AtomicStringImpl>` in the caller side.

* dfg/DFGOperations.cpp:
* jit/JITOperations.cpp:
(JSC::getByVal):
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::getByVal):
* runtime/JSString.cpp:
(JSC::JSRopeString::resolveRopeToExistingAtomicString):
* runtime/JSString.h:
(JSC::JSString::toExistingAtomicString):

Source/WebCore:

Hold the ownership of AtomicStringImpl*.

* bindings/scripts/CodeGeneratorJS.pm:
(GenerateParametersCheck):
* dom/TreeScope.cpp:
(WebCore::TreeScope::getElementById):

Source/WTF:

Return `RefPtr<AtomicStringImpl>` instead of `AtomicStringImpl*`.

* wtf/text/AtomicStringImpl.cpp:
(WTF::AtomicStringImpl::lookUpSlowCase):
(WTF::AtomicStringImpl::lookUpInternal):
* wtf/text/AtomicStringImpl.h:
(WTF::AtomicStringImpl::lookUp):

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

12 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGOperations.cpp
Source/JavaScriptCore/jit/JITOperations.cpp
Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
Source/JavaScriptCore/runtime/JSString.cpp
Source/JavaScriptCore/runtime/JSString.h
Source/WTF/ChangeLog
Source/WTF/wtf/text/AtomicStringImpl.cpp
Source/WTF/wtf/text/AtomicStringImpl.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
Source/WebCore/dom/TreeScope.cpp

index 50ac111..81e74b5 100644 (file)
@@ -1,3 +1,58 @@
+2015-06-02  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Heap-use-after-free read of size 4 in JavaScriptCore: WTF::StringImpl::isSymbol() (StringImpl.h:496)
+        https://bugs.webkit.org/show_bug.cgi?id=145532
+
+        Reviewed by Geoffrey Garen.
+
+        AtomicStringImpl::lookUp returns AtomicStringImpl*,
+        it doesn't give any ownership to the caller.
+        Originally, this is ok because the ownership is taken
+        by AtomicStringImpl's table (& the register side).
+
+        But if we would like to use this returned AtomicStringImpl*,
+        we should take its ownership immediately.
+        Because if the register side releases its ownership (ref count),
+        it will be destroyed.
+
+        In JSString::toExistingAtomicString, it returns AtomicStringImpl*.
+        But it's not appropriate.
+        If the owner of AtomicStringImpl* is always JSString*, it is ok.
+        But it looks up the table-registered AtomicStringImpl* from
+        the AtomicStringImpl table. So JSString* may not have the ownership
+        of the returned AtomicStringImpl*.
+
+        The failure situation is the following.
+
+        1. A creates AtomicStringImpl. A has its ownership.
+           And A registers it to AtomicStringImpl table.
+        2. JSString looks up the AtomicStringImpl from the table.
+           It gets AtomicStringImpl*. And JSString doesn't have its ownership.
+           It returns the raw pointer immediately to the users
+        3. A is released. There's no owner for AtomicStringImpl*.
+           So it's also destroyed.
+        4. Use looked up AtomicStringImpl in (2). It becomes use-after-free.
+
+        This patch fixes it by the following changes.
+
+        1. Change the signature of `AtomicStringImpl* AtomicStringImpl::lookUp(...)`
+           to `RefPtr<AtomicStringImpl> AtomicStringImpl::lookUp(..)`.
+           Use `RefPtr` because it may return `nullptr`.
+        2. Change the signature of `AtomicStringImpl* JSString::toExistingAtomicString(...)`
+           to `RefPtr<AtomicStringImpl> JSString::toExistingAtomicString(...)`.
+           Using `RefPtr` is the same reason.
+        3. Receive the result with `RefPtr<AtomicStringImpl>` in the caller side.
+
+        * dfg/DFGOperations.cpp:
+        * jit/JITOperations.cpp:
+        (JSC::getByVal):
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::getByVal):
+        * runtime/JSString.cpp:
+        (JSC::JSRopeString::resolveRopeToExistingAtomicString):
+        * runtime/JSString.h:
+        (JSC::JSString::toExistingAtomicString):
+
 2015-05-30  Filip Pizlo  <fpizlo@apple.com>
 
         Any exit from any JIT due to profiling for an inline cache should force all future compilations to be wary
index cf3a34b..a99a3fa 100644 (file)
@@ -297,8 +297,8 @@ EncodedJSValue JIT_OPERATION operationGetByVal(ExecState* exec, EncodedJSValue e
         } else if (property.isString()) {
             Structure& structure = *base->structure(vm);
             if (JSCell::canUseFastGetOwnProperty(structure)) {
-                if (AtomicStringImpl* existingAtomicString = asString(property)->toExistingAtomicString(exec)) {
-                    if (JSValue result = base->fastGetOwnProperty(vm, structure, existingAtomicString))
+                if (RefPtr<AtomicStringImpl> existingAtomicString = asString(property)->toExistingAtomicString(exec)) {
+                    if (JSValue result = base->fastGetOwnProperty(vm, structure, existingAtomicString.get()))
                         return JSValue::encode(result);
                 }
             }
@@ -331,8 +331,8 @@ EncodedJSValue JIT_OPERATION operationGetByValCell(ExecState* exec, JSCell* base
     } else if (property.isString()) {
         Structure& structure = *base->structure(vm);
         if (JSCell::canUseFastGetOwnProperty(structure)) {
-            if (AtomicStringImpl* existingAtomicString = asString(property)->toExistingAtomicString(exec)) {
-                if (JSValue result = base->fastGetOwnProperty(vm, structure, existingAtomicString))
+            if (RefPtr<AtomicStringImpl> existingAtomicString = asString(property)->toExistingAtomicString(exec)) {
+                if (JSValue result = base->fastGetOwnProperty(vm, structure, existingAtomicString.get()))
                     return JSValue::encode(result);
             }
         }
index 8450ead..114c7b7 100644 (file)
@@ -1463,8 +1463,8 @@ static JSValue getByVal(ExecState* exec, JSValue baseValue, JSValue subscript, R
         VM& vm = exec->vm();
         Structure& structure = *baseValue.asCell()->structure(vm);
         if (JSCell::canUseFastGetOwnProperty(structure)) {
-            if (AtomicStringImpl* existingAtomicString = asString(subscript)->toExistingAtomicString(exec)) {
-                if (JSValue result = baseValue.asCell()->fastGetOwnProperty(vm, structure, existingAtomicString))
+            if (RefPtr<AtomicStringImpl> existingAtomicString = asString(subscript)->toExistingAtomicString(exec)) {
+                if (JSValue result = baseValue.asCell()->fastGetOwnProperty(vm, structure, existingAtomicString.get()))
                     return result;
             }
         }
index f5865c0..8fb2cc9 100644 (file)
@@ -728,8 +728,8 @@ inline JSValue getByVal(ExecState* exec, JSValue baseValue, JSValue subscript)
         VM& vm = exec->vm();
         Structure& structure = *baseValue.asCell()->structure(vm);
         if (JSCell::canUseFastGetOwnProperty(structure)) {
-            if (AtomicStringImpl* existingAtomicString = asString(subscript)->toExistingAtomicString(exec)) {
-                if (JSValue result = baseValue.asCell()->fastGetOwnProperty(vm, structure, existingAtomicString))
+            if (RefPtr<AtomicStringImpl> existingAtomicString = asString(subscript)->toExistingAtomicString(exec)) {
+                if (JSValue result = baseValue.asCell()->fastGetOwnProperty(vm, structure, existingAtomicString.get()))
                     return result;
             }
         }
index 4b7ef7a..8c7ec08 100644 (file)
@@ -190,11 +190,11 @@ void JSRopeString::clearFibers() const
         u[i].number = 0;
 }
 
-AtomicStringImpl* JSRopeString::resolveRopeToExistingAtomicString(ExecState* exec) const
+RefPtr<AtomicStringImpl> JSRopeString::resolveRopeToExistingAtomicString(ExecState* exec) const
 {
     if (m_length > maxLengthForOnStackResolve) {
         resolveRope(exec);
-        if (AtomicStringImpl* existingAtomicString = AtomicStringImpl::lookUp(m_value.impl())) {
+        if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(m_value.impl())) {
             m_value = *existingAtomicString;
             setIs8Bit(m_value.impl()->is8Bit());
             clearFibers();
@@ -206,7 +206,7 @@ AtomicStringImpl* JSRopeString::resolveRopeToExistingAtomicString(ExecState* exe
     if (is8Bit()) {
         LChar buffer[maxLengthForOnStackResolve];
         resolveRopeInternal8(buffer);
-        if (AtomicStringImpl* existingAtomicString = AtomicStringImpl::lookUp(buffer, m_length)) {
+        if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(buffer, m_length)) {
             m_value = *existingAtomicString;
             setIs8Bit(m_value.impl()->is8Bit());
             clearFibers();
@@ -215,7 +215,7 @@ AtomicStringImpl* JSRopeString::resolveRopeToExistingAtomicString(ExecState* exe
     } else {
         UChar buffer[maxLengthForOnStackResolve];
         resolveRopeInternal16(buffer);
-        if (AtomicStringImpl* existingAtomicString = AtomicStringImpl::lookUp(buffer, m_length)) {
+        if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(buffer, m_length)) {
             m_value = *existingAtomicString;
             setIs8Bit(m_value.impl()->is8Bit());
             clearFibers();
index 34fe91d..d4969db 100644 (file)
@@ -142,7 +142,7 @@ public:
 
     Identifier toIdentifier(ExecState*) const;
     AtomicString toAtomicString(ExecState*) const;
-    AtomicStringImpl* toExistingAtomicString(ExecState*) const;
+    RefPtr<AtomicStringImpl> toExistingAtomicString(ExecState*) const;
     StringView view(ExecState*) const;
     const String& value(ExecState*) const;
     const String& tryGetValue() const;
@@ -361,7 +361,7 @@ private:
 
     JS_EXPORT_PRIVATE void resolveRope(ExecState*) const;
     JS_EXPORT_PRIVATE void resolveRopeToAtomicString(ExecState*) const;
-    JS_EXPORT_PRIVATE AtomicStringImpl* resolveRopeToExistingAtomicString(ExecState*) const;
+    JS_EXPORT_PRIVATE RefPtr<AtomicStringImpl> resolveRopeToExistingAtomicString(ExecState*) const;
     void resolveRopeSlowCase8(LChar*) const;
     void resolveRopeSlowCase(UChar*) const;
     void outOfMemory(ExecState*) const;
@@ -467,7 +467,7 @@ ALWAYS_INLINE AtomicString JSString::toAtomicString(ExecState* exec) const
     return AtomicString(m_value);
 }
 
-ALWAYS_INLINE AtomicStringImpl* JSString::toExistingAtomicString(ExecState* exec) const
+ALWAYS_INLINE RefPtr<AtomicStringImpl> JSString::toExistingAtomicString(ExecState* exec) const
 {
     if (isRope())
         return static_cast<const JSRopeString*>(this)->resolveRopeToExistingAtomicString(exec);
index bf2bf46..5d63dd2 100644 (file)
@@ -1,3 +1,18 @@
+2015-06-02  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Heap-use-after-free read of size 4 in JavaScriptCore: WTF::StringImpl::isSymbol() (StringImpl.h:496)
+        https://bugs.webkit.org/show_bug.cgi?id=145532
+
+        Reviewed by Geoffrey Garen.
+
+        Return `RefPtr<AtomicStringImpl>` instead of `AtomicStringImpl*`.
+
+        * wtf/text/AtomicStringImpl.cpp:
+        (WTF::AtomicStringImpl::lookUpSlowCase):
+        (WTF::AtomicStringImpl::lookUpInternal):
+        * wtf/text/AtomicStringImpl.h:
+        (WTF::AtomicStringImpl::lookUp):
+
 2015-06-01  Anders Carlsson  <andersca@apple.com>
 
         Use xpc_connection_set_oneshot_instance where possible
index b1fe7e9..af595a6 100644 (file)
@@ -454,7 +454,7 @@ void AtomicStringImpl::remove(AtomicStringImpl* string)
     atomicStringTable.remove(iterator);
 }
 
-AtomicStringImpl* AtomicStringImpl::lookUpSlowCase(StringImpl& string)
+RefPtr<AtomicStringImpl> AtomicStringImpl::lookUpSlowCase(StringImpl& string)
 {
     ASSERT_WITH_MESSAGE(!string.isAtomic(), "AtomicStringImpls should return from the fast case.");
 
@@ -487,7 +487,7 @@ RefPtr<AtomicStringImpl> AtomicStringImpl::addUTF8(const char* charactersStart,
     return addToStringTable<HashAndUTF8Characters, HashAndUTF8CharactersTranslator>(buffer);
 }
 
-AtomicStringImpl* AtomicStringImpl::lookUpInternal(const LChar* characters, unsigned length)
+RefPtr<AtomicStringImpl> AtomicStringImpl::lookUpInternal(const LChar* characters, unsigned length)
 {
     AtomicStringTableLocker locker;
     auto& table = stringTable();
@@ -499,7 +499,7 @@ AtomicStringImpl* AtomicStringImpl::lookUpInternal(const LChar* characters, unsi
     return nullptr;
 }
 
-AtomicStringImpl* AtomicStringImpl::lookUpInternal(const UChar* characters, unsigned length)
+RefPtr<AtomicStringImpl> AtomicStringImpl::lookUpInternal(const UChar* characters, unsigned length)
 {
     AtomicStringTableLocker locker;
     auto& table = stringTable();
index 7734490..c14d07f 100644 (file)
@@ -29,15 +29,15 @@ class AtomicStringTable;
 
 class AtomicStringImpl : public UniquedStringImpl {
 public:
-    static AtomicStringImpl* lookUp(LChar* characters, unsigned length)
+    static RefPtr<AtomicStringImpl> lookUp(LChar* characters, unsigned length)
     {
         return lookUpInternal(characters, length);
     }
-    static AtomicStringImpl* lookUp(UChar* characters, unsigned length)
+    static RefPtr<AtomicStringImpl> lookUp(UChar* characters, unsigned length)
     {
         return lookUpInternal(characters, length);
     }
-    static AtomicStringImpl* lookUp(StringImpl* string)
+    static RefPtr<AtomicStringImpl> lookUp(StringImpl* string)
     {
         if (!string || string->isAtomic())
             return static_cast<AtomicStringImpl*>(string);
@@ -104,10 +104,10 @@ private:
     WTF_EXPORT_STRING_API static Ref<AtomicStringImpl> addSlowCase(StringImpl&);
     WTF_EXPORT_STRING_API static Ref<AtomicStringImpl> addSlowCase(AtomicStringTable&, StringImpl&);
 
-    WTF_EXPORT_STRING_API static AtomicStringImpl* lookUpSlowCase(StringImpl&);
+    WTF_EXPORT_STRING_API static RefPtr<AtomicStringImpl> lookUpSlowCase(StringImpl&);
 
-    WTF_EXPORT_STRING_API static AtomicStringImpl* lookUpInternal(const LChar*, unsigned length);
-    WTF_EXPORT_STRING_API static AtomicStringImpl* lookUpInternal(const UChar*, unsigned length);
+    WTF_EXPORT_STRING_API static RefPtr<AtomicStringImpl> lookUpInternal(const LChar*, unsigned length);
+    WTF_EXPORT_STRING_API static RefPtr<AtomicStringImpl> lookUpInternal(const UChar*, unsigned length);
 };
 
 #if !ASSERT_DISABLED
index df9a576..ec6302f 100644 (file)
@@ -1,3 +1,17 @@
+2015-06-02  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Heap-use-after-free read of size 4 in JavaScriptCore: WTF::StringImpl::isSymbol() (StringImpl.h:496)
+        https://bugs.webkit.org/show_bug.cgi?id=145532
+
+        Reviewed by Geoffrey Garen.
+
+        Hold the ownership of AtomicStringImpl*.
+
+        * bindings/scripts/CodeGeneratorJS.pm:
+        (GenerateParametersCheck):
+        * dom/TreeScope.cpp:
+        (WebCore::TreeScope::getElementById):
+
 2015-06-02  Youenn Fablet  <youenn.fablet@crf.canon.fr>
 
         SharedBuffer::copy should return a Ref<SharedBuffer>
index 70df14b..844f630 100644 (file)
@@ -3328,10 +3328,10 @@ sub GenerateParametersCheck
             }
 
             if ($parameter->extendedAttributes->{"RequiresExistingAtomicString"}) {
-                push(@$outputArray, "    AtomicStringImpl* existing_$name = exec->argument($argsIndex).isEmpty() ? nullptr : exec->argument($argsIndex).toString(exec)->toExistingAtomicString(exec);\n");
+                push(@$outputArray, "    RefPtr<AtomicStringImpl> existing_$name = exec->argument($argsIndex).isEmpty() ? nullptr : exec->argument($argsIndex).toString(exec)->toExistingAtomicString(exec);\n");
                 push(@$outputArray, "    if (!existing_$name)\n");
                 push(@$outputArray, "        return JSValue::encode(jsNull());\n");
-                push(@$outputArray, "    const AtomicString $name(existing_$name);\n");
+                push(@$outputArray, "    const AtomicString $name(*existing_$name);\n");
             } else {
                 push(@$outputArray, "    " . GetNativeTypeFromSignature($parameter) . " $name(" . JSValueToNative($parameter, $optional && $defaultAttribute && $defaultAttribute eq "NullString" ? "argumentOrNull(exec, $argsIndex)" : "exec->argument($argsIndex)", $function->signature->extendedAttributes->{"Conditional"}) . ");\n");
             }
index af11a13..26e9fbb 100644 (file)
@@ -112,7 +112,7 @@ Element* TreeScope::getElementById(const String& elementId) const
     if (!m_elementsById)
         return nullptr;
 
-    if (AtomicStringImpl* atomicElementId = AtomicStringImpl::lookUp(elementId.impl()))
+    if (RefPtr<AtomicStringImpl> atomicElementId = AtomicStringImpl::lookUp(elementId.impl()))
         return m_elementsById->getElementById(*atomicElementId, *this);
 
     return nullptr;