<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[184316] trunk/Source</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/184316">184316</a></dd>
<dt>Author</dt> <dd>oliver@apple.com</dd>
<dt>Date</dt> <dd>2015-05-13 16:18:56 -0700 (Wed, 13 May 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>Source/JavaScriptCore:
Ensure that all the smart pointer types in WTF clear their pointer before deref
https://bugs.webkit.org/show_bug.cgi?id=143789
Reviewed by Ryosuke Niwa.
One of the simpler cases of this in JavaScriptCore. There
are other cases where we need to guard the derefs but they
are more complex cases.
* inspector/JSInjectedScriptHost.cpp:
(Inspector::JSInjectedScriptHost::releaseImpl):
* inspector/JSJavaScriptCallFrame.cpp:
(Inspector::JSJavaScriptCallFrame::releaseImpl):
Source/WTF:
Ensure that all the smart pointer types in WTF clear their pointer before deref
https://bugs.webkit.org/show_bug.cgi?id=143789
Reviewed by Ryosuke Niwa.
In order to prevent use after free bugs caused by destructors
that end up trying to access the smart pointer itself, we should
make sure we always clear the m_ptr field before calling deref.
Essentially the UaF path is:
struct Foo : RefCounted<Foo> {
Wibble* m_wibble;
void doSomething();
~Foo() { m_wibble->doSomethingLikeCleanup(); }
};
struct Wibble {
void doSomethingLikeCleanup()
{
if (m_foo) {
/* if this branch is not here we get a null deref */
m_foo->doSomething();
}
}
void replaceFoo(Foo* foo) { m_foo = foo; }
RefPtr<Foo> m_foo;
};
Wibble* someWibble = /* a Wibble with m_foo->m_refCount == 1 */;
/* and m_foo points to someWibble */;
someWibble->replaceFoo(someOtherFoo);
+ someWibble->m_foo->m_ptr->deref();
+ someWibble->m_foo->m_ptr->~Foo()
+ someWibble->m_foo->m_ptr->m_wibble->doSomethingLikeCleanup()
+ someWibble->m_foo->m_ptr->m_wibble /* someWibble */ ->m_foo->m_ptr /*logically dead*/ ->doSomething()
By clearing m_ptr first we either force a null pointer deref or
we force our code down a path that does not use the dead smart
pointer.
* wtf/PassRefPtr.h:
(WTF::PassRefPtr::~PassRefPtr):
* wtf/Ref.h:
(WTF::Ref::~Ref):
(WTF::Ref::operator=):
* wtf/RefPtr.h:
(WTF::RefPtr::~RefPtr):
* wtf/RetainPtr.h:
(WTF::RetainPtr::~RetainPtr):
(WTF::RetainPtr<T>::clear):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreinspectorJSInjectedScriptHostcpp">trunk/Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreinspectorJSJavaScriptCallFramecpp">trunk/Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.cpp</a></li>
<li><a href="#trunkSourceWTFChangeLog">trunk/Source/WTF/ChangeLog</a></li>
<li><a href="#trunkSourceWTFwtfPassRefPtrh">trunk/Source/WTF/wtf/PassRefPtr.h</a></li>
<li><a href="#trunkSourceWTFwtfRefPtrh">trunk/Source/WTF/wtf/RefPtr.h</a></li>
<li><a href="#trunkSourceWTFwtfRetainPtrh">trunk/Source/WTF/wtf/RetainPtr.h</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (184315 => 184316)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-05-13 23:09:13 UTC (rev 184315)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-05-13 23:18:56 UTC (rev 184316)
</span><span class="lines">@@ -1,3 +1,18 @@
</span><ins>+2015-05-13 Oliver Hunt <oliver@apple.com>
+ Ensure that all the smart pointer types in WTF clear their pointer before deref
+ https://bugs.webkit.org/show_bug.cgi?id=143789
+
+ Reviewed by Ryosuke Niwa.
+
+ One of the simpler cases of this in JavaScriptCore. There
+ are other cases where we need to guard the derefs but they
+ are more complex cases.
+
+ * inspector/JSInjectedScriptHost.cpp:
+ (Inspector::JSInjectedScriptHost::releaseImpl):
+ * inspector/JSJavaScriptCallFrame.cpp:
+ (Inspector::JSJavaScriptCallFrame::releaseImpl):
+
</ins><span class="cx"> 2015-05-13 Alexandr Skachkov <gskachkov@gmail.com>
</span><span class="cx">
</span><span class="cx"> Small refactoring before ES6 Arrow function implementation.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreinspectorJSInjectedScriptHostcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp (184315 => 184316)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp 2015-05-13 23:09:13 UTC (rev 184315)
+++ trunk/Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp 2015-05-13 23:18:56 UTC (rev 184316)
</span><span class="lines">@@ -85,10 +85,8 @@
</span><span class="cx">
</span><span class="cx"> void JSInjectedScriptHost::releaseImpl()
</span><span class="cx"> {
</span><del>- if (m_impl) {
- m_impl->deref();
- m_impl = nullptr;
- }
</del><ins>+ if (auto impl = std::exchange(m_impl, nullptr))
+ impl->deref();
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> JSInjectedScriptHost::~JSInjectedScriptHost()
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreinspectorJSJavaScriptCallFramecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.cpp (184315 => 184316)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.cpp 2015-05-13 23:09:13 UTC (rev 184315)
+++ trunk/Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.cpp 2015-05-13 23:18:56 UTC (rev 184316)
</span><span class="lines">@@ -64,10 +64,8 @@
</span><span class="cx">
</span><span class="cx"> void JSJavaScriptCallFrame::releaseImpl()
</span><span class="cx"> {
</span><del>- if (m_impl) {
- m_impl->deref();
- m_impl = nullptr;
- }
</del><ins>+ if (auto impl = std::exchange(m_impl, nullptr))
+ impl->deref();
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> JSJavaScriptCallFrame::~JSJavaScriptCallFrame()
</span></span></pre></div>
<a id="trunkSourceWTFChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/ChangeLog (184315 => 184316)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/ChangeLog 2015-05-13 23:09:13 UTC (rev 184315)
+++ trunk/Source/WTF/ChangeLog 2015-05-13 23:18:56 UTC (rev 184316)
</span><span class="lines">@@ -1,3 +1,56 @@
</span><ins>+2015-05-13 Oliver Hunt <oliver@apple.com>
+ Ensure that all the smart pointer types in WTF clear their pointer before deref
+ https://bugs.webkit.org/show_bug.cgi?id=143789
+
+ Reviewed by Ryosuke Niwa.
+
+ In order to prevent use after free bugs caused by destructors
+ that end up trying to access the smart pointer itself, we should
+ make sure we always clear the m_ptr field before calling deref.
+
+ Essentially the UaF path is:
+ struct Foo : RefCounted<Foo> {
+ Wibble* m_wibble;
+ void doSomething();
+ ~Foo() { m_wibble->doSomethingLikeCleanup(); }
+ };
+
+ struct Wibble {
+ void doSomethingLikeCleanup()
+ {
+ if (m_foo) {
+ /* if this branch is not here we get a null deref */
+ m_foo->doSomething();
+ }
+ }
+ void replaceFoo(Foo* foo) { m_foo = foo; }
+ RefPtr<Foo> m_foo;
+ };
+
+ Wibble* someWibble = /* a Wibble with m_foo->m_refCount == 1 */;
+ /* and m_foo points to someWibble */;
+
+ someWibble->replaceFoo(someOtherFoo);
+ + someWibble->m_foo->m_ptr->deref();
+ + someWibble->m_foo->m_ptr->~Foo()
+ + someWibble->m_foo->m_ptr->m_wibble->doSomethingLikeCleanup()
+ + someWibble->m_foo->m_ptr->m_wibble /* someWibble */ ->m_foo->m_ptr /*logically dead*/ ->doSomething()
+
+ By clearing m_ptr first we either force a null pointer deref or
+ we force our code down a path that does not use the dead smart
+ pointer.
+
+ * wtf/PassRefPtr.h:
+ (WTF::PassRefPtr::~PassRefPtr):
+ * wtf/Ref.h:
+ (WTF::Ref::~Ref):
+ (WTF::Ref::operator=):
+ * wtf/RefPtr.h:
+ (WTF::RefPtr::~RefPtr):
+ * wtf/RetainPtr.h:
+ (WTF::RetainPtr::~RetainPtr):
+ (WTF::RetainPtr<T>::clear):
+
</ins><span class="cx"> 2015-05-12 Michael Saboff <msaboff@apple.com>
</span><span class="cx">
</span><span class="cx"> If JSC cannot get executable memory, it shouldn't call madvise
</span></span></pre></div>
<a id="trunkSourceWTFwtfPassRefPtrh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/PassRefPtr.h (184315 => 184316)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/PassRefPtr.h 2015-05-13 23:09:13 UTC (rev 184315)
+++ trunk/Source/WTF/wtf/PassRefPtr.h 2015-05-13 23:18:56 UTC (rev 184316)
</span><span class="lines">@@ -55,7 +55,7 @@
</span><span class="cx"> PassRefPtr(const PassRefPtr& o) : m_ptr(o.leakRef()) { }
</span><span class="cx"> template<typename U> PassRefPtr(const PassRefPtr<U>& o) : m_ptr(o.leakRef()) { }
</span><span class="cx">
</span><del>- ALWAYS_INLINE ~PassRefPtr() { derefIfNotNull(m_ptr); }
</del><ins>+ ALWAYS_INLINE ~PassRefPtr() { derefIfNotNull(std::exchange(m_ptr, nullptr)); }
</ins><span class="cx">
</span><span class="cx"> template<typename U> PassRefPtr(const RefPtr<U>&);
</span><span class="cx"> template<typename U> PassRefPtr(Ref<U>&& reference) : m_ptr(&reference.leakRef()) { }
</span></span></pre></div>
<a id="trunkSourceWTFwtfRefPtrh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/RefPtr.h (184315 => 184316)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/RefPtr.h 2015-05-13 23:09:13 UTC (rev 184315)
+++ trunk/Source/WTF/wtf/RefPtr.h 2015-05-13 23:18:56 UTC (rev 184316)
</span><span class="lines">@@ -56,7 +56,7 @@
</span><span class="cx"> RefPtr(HashTableDeletedValueType) : m_ptr(hashTableDeletedValue()) { }
</span><span class="cx"> bool isHashTableDeletedValue() const { return m_ptr == hashTableDeletedValue(); }
</span><span class="cx">
</span><del>- ALWAYS_INLINE ~RefPtr() { derefIfNotNull(m_ptr); }
</del><ins>+ ALWAYS_INLINE ~RefPtr() { derefIfNotNull(std::exchange(m_ptr, nullptr)); }
</ins><span class="cx">
</span><span class="cx"> T* get() const { return m_ptr; }
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceWTFwtfRetainPtrh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/RetainPtr.h (184315 => 184316)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/RetainPtr.h 2015-05-13 23:09:13 UTC (rev 184315)
+++ trunk/Source/WTF/wtf/RetainPtr.h 2015-05-13 23:18:56 UTC (rev 184316)
</span><span class="lines">@@ -71,7 +71,7 @@
</span><span class="cx"> RetainPtr(HashTableDeletedValueType) : m_ptr(hashTableDeletedValue()) { }
</span><span class="cx"> bool isHashTableDeletedValue() const { return m_ptr == hashTableDeletedValue(); }
</span><span class="cx">
</span><del>- ~RetainPtr() { if (StorageType ptr = m_ptr) CFRelease(ptr); }
</del><ins>+ ~RetainPtr();
</ins><span class="cx">
</span><span class="cx"> template<typename U> RetainPtr(const RetainPtr<U>&);
</span><span class="cx">
</span><span class="lines">@@ -135,6 +135,12 @@
</span><span class="cx"> StorageType m_ptr;
</span><span class="cx"> };
</span><span class="cx">
</span><ins>+template<typename T> inline RetainPtr<T>::~RetainPtr()
+{
+ if (StorageType ptr = std::exchange(m_ptr, nullptr))
+ CFRelease(ptr);
+}
+
</ins><span class="cx"> // Helper function for creating a RetainPtr using template argument deduction.
</span><span class="cx"> template<typename T> inline RetainPtr<T> retainPtr(T) WARN_UNUSED_RETURN;
</span><span class="cx">
</span><span class="lines">@@ -147,10 +153,8 @@
</span><span class="cx">
</span><span class="cx"> template<typename T> inline void RetainPtr<T>::clear()
</span><span class="cx"> {
</span><del>- if (StorageType ptr = m_ptr) {
- m_ptr = nullptr;
</del><ins>+ if (StorageType ptr = std::exchange(m_ptr, nullptr))
</ins><span class="cx"> CFRelease(ptr);
</span><del>- }
</del><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> template<typename T> inline typename RetainPtr<T>::PtrType RetainPtr<T>::leakRef()
</span></span></pre>
</div>
</div>
</body>
</html>