Title: [184316] trunk/Source
- Revision
- 184316
- Author
- oli...@apple.com
- Date
- 2015-05-13 16:18:56 -0700 (Wed, 13 May 2015)
Log Message
Source/_javascript_Core:
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 _javascript_Core. 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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (184315 => 184316)
--- trunk/Source/_javascript_Core/ChangeLog 2015-05-13 23:09:13 UTC (rev 184315)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-05-13 23:18:56 UTC (rev 184316)
@@ -1,3 +1,18 @@
+2015-05-13 Oliver Hunt <oli...@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 _javascript_Core. 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):
+
2015-05-13 Alexandr Skachkov <gskach...@gmail.com>
Small refactoring before ES6 Arrow function implementation.
Modified: trunk/Source/_javascript_Core/inspector/JSInjectedScriptHost.cpp (184315 => 184316)
--- trunk/Source/_javascript_Core/inspector/JSInjectedScriptHost.cpp 2015-05-13 23:09:13 UTC (rev 184315)
+++ trunk/Source/_javascript_Core/inspector/JSInjectedScriptHost.cpp 2015-05-13 23:18:56 UTC (rev 184316)
@@ -85,10 +85,8 @@
void JSInjectedScriptHost::releaseImpl()
{
- if (m_impl) {
- m_impl->deref();
- m_impl = nullptr;
- }
+ if (auto impl = std::exchange(m_impl, nullptr))
+ impl->deref();
}
JSInjectedScriptHost::~JSInjectedScriptHost()
Modified: trunk/Source/_javascript_Core/inspector/JSJavaScriptCallFrame.cpp (184315 => 184316)
--- trunk/Source/_javascript_Core/inspector/JSJavaScriptCallFrame.cpp 2015-05-13 23:09:13 UTC (rev 184315)
+++ trunk/Source/_javascript_Core/inspector/JSJavaScriptCallFrame.cpp 2015-05-13 23:18:56 UTC (rev 184316)
@@ -64,10 +64,8 @@
void JSJavaScriptCallFrame::releaseImpl()
{
- if (m_impl) {
- m_impl->deref();
- m_impl = nullptr;
- }
+ if (auto impl = std::exchange(m_impl, nullptr))
+ impl->deref();
}
JSJavaScriptCallFrame::~JSJavaScriptCallFrame()
Modified: trunk/Source/WTF/ChangeLog (184315 => 184316)
--- 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)
@@ -1,3 +1,56 @@
+2015-05-13 Oliver Hunt <oli...@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):
+
2015-05-12 Michael Saboff <msab...@apple.com>
If JSC cannot get executable memory, it shouldn't call madvise
Modified: trunk/Source/WTF/wtf/PassRefPtr.h (184315 => 184316)
--- 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)
@@ -55,7 +55,7 @@
PassRefPtr(const PassRefPtr& o) : m_ptr(o.leakRef()) { }
template<typename U> PassRefPtr(const PassRefPtr<U>& o) : m_ptr(o.leakRef()) { }
- ALWAYS_INLINE ~PassRefPtr() { derefIfNotNull(m_ptr); }
+ ALWAYS_INLINE ~PassRefPtr() { derefIfNotNull(std::exchange(m_ptr, nullptr)); }
template<typename U> PassRefPtr(const RefPtr<U>&);
template<typename U> PassRefPtr(Ref<U>&& reference) : m_ptr(&reference.leakRef()) { }
Modified: trunk/Source/WTF/wtf/RefPtr.h (184315 => 184316)
--- 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)
@@ -56,7 +56,7 @@
RefPtr(HashTableDeletedValueType) : m_ptr(hashTableDeletedValue()) { }
bool isHashTableDeletedValue() const { return m_ptr == hashTableDeletedValue(); }
- ALWAYS_INLINE ~RefPtr() { derefIfNotNull(m_ptr); }
+ ALWAYS_INLINE ~RefPtr() { derefIfNotNull(std::exchange(m_ptr, nullptr)); }
T* get() const { return m_ptr; }
Modified: trunk/Source/WTF/wtf/RetainPtr.h (184315 => 184316)
--- 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)
@@ -71,7 +71,7 @@
RetainPtr(HashTableDeletedValueType) : m_ptr(hashTableDeletedValue()) { }
bool isHashTableDeletedValue() const { return m_ptr == hashTableDeletedValue(); }
- ~RetainPtr() { if (StorageType ptr = m_ptr) CFRelease(ptr); }
+ ~RetainPtr();
template<typename U> RetainPtr(const RetainPtr<U>&);
@@ -135,6 +135,12 @@
StorageType m_ptr;
};
+template<typename T> inline RetainPtr<T>::~RetainPtr()
+{
+ if (StorageType ptr = std::exchange(m_ptr, nullptr))
+ CFRelease(ptr);
+}
+
// Helper function for creating a RetainPtr using template argument deduction.
template<typename T> inline RetainPtr<T> retainPtr(T) WARN_UNUSED_RETURN;
@@ -147,10 +153,8 @@
template<typename T> inline void RetainPtr<T>::clear()
{
- if (StorageType ptr = m_ptr) {
- m_ptr = nullptr;
+ if (StorageType ptr = std::exchange(m_ptr, nullptr))
CFRelease(ptr);
- }
}
template<typename T> inline typename RetainPtr<T>::PtrType RetainPtr<T>::leakRef()
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
