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)
commit4b6c423c3aadabecb73140880cdcfd10b5d13161
treefeaf994ee9a457a70a82d6b4a3cf4310ab3e697e
parent54b37de49c856344de6ac512c2485e4d922c34f6
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):

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