Changeset 185109 in webkit for trunk/Source/JavaScriptCore/jit/JITOperations.cpp

Timestamp:

Jun 2, 2015, 10:36:16 AM (10 years ago)

Author:

Yusuke Suzuki

Message:

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.

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):

File:

• trunk/Source/JavaScriptCore/jit/JITOperations.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/jit/JITOperations.cpp

@@ -1464 +1464 @@
         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;
             }