memory: fix a few UAF cases, add asan to test

Author: vaxerski

CMakeLists.txt

@@ -62,16 +62,18 @@ if(BUILD_TESTING)
   file(GLOB_RECURSE TESTFILES CONFIGURE_DEPENDS "tests/*.cpp")
   add_executable(hyprutils_tests ${TESTFILES})
 
-  target_compile_options(hyprutils_tests PRIVATE --coverage)
+  target_compile_options(hyprutils_tests PRIVATE --coverage -fsanitize=address)
   target_link_options(hyprutils_tests PRIVATE --coverage)
 
   target_include_directories(
     hyprutils_tests
     PUBLIC "./include"
     PRIVATE "./src" "./src/include" "./protocols" "${CMAKE_BINARY_DIR}")
-  target_link_libraries(hyprutils_tests PRIVATE hyprutils GTest::gtest_main
+  target_link_libraries(hyprutils_tests PRIVATE asan hyprutils GTest::gtest_main
                                                        PkgConfig::deps)
-  gtest_discover_tests(hyprutils_tests)
+  gtest_discover_tests(hyprutils_tests
+    PROPERTIES ENVIRONMENT "ASAN_OPTIONS=detect_leaks=0"
+  )
 
   # Add coverage to hyprutils for test builds
   target_compile_options(hyprutils PRIVATE --coverage)

include/hyprutils/memory/SharedPtr.hpp

@@ -67,15 +67,15 @@ namespace Hyprutils {
             }
 
             ~CSharedPointer() {
-                decrement();
+                decrement(impl_);
             }
 
             template <typename U>
             validHierarchy<const CSharedPointer<U>&> operator=(const CSharedPointer<U>& rhs) {
                 if (impl_ == rhs.impl_)
                     return *this;
 
-                decrement();
+                decrement(impl_);
                 impl_  = rhs.impl_;
                 m_data = rhs.m_data;
                 increment();
@@ -86,7 +86,7 @@ namespace Hyprutils {
                 if (impl_ == rhs.impl_)
                     return *this;
 
-                decrement();
+                decrement(impl_);
                 impl_  = rhs.impl_;
                 m_data = rhs.m_data;
                 increment();
@@ -133,9 +133,10 @@ namespace Hyprutils {
             }
 
             void reset() {
-                decrement();
-                impl_  = nullptr;
-                m_data = nullptr;
+                auto ptr = impl_;
+                impl_    = nullptr;
+                m_data   = nullptr;
+                decrement(ptr);
             }
 
             T* get() const {
@@ -161,15 +162,15 @@ namespace Hyprutils {
                 may delete the stored object if ref == 0
                 may delete and reset impl_ if ref == 0 and weak == 0
             */
-            void decrement() {
-                if (!impl_)
+            void decrement(Impl_::impl_base* base) {
+                if (!base)
                     return;
 
-                impl_->dec();
+                base->dec();
 
                 // if ref == 0, we can destroy impl
-                if (impl_->ref() == 0)
-                    destroyImpl();
+                if (base->ref() == 0)
+                    destroyImpl(base);
             }
             /* no-op if there is no impl_ */
             void increment() {
@@ -181,15 +182,13 @@ namespace Hyprutils {
 
             /* destroy the pointed-to object
                if able, will also destroy impl */
-            void destroyImpl() {
-                // destroy the impl contents
-                impl_->destroy();
-
-                // check for weak refs, if zero, we can also delete impl_
-                if (impl_->wref() == 0) {
-                    delete impl_;
-                    impl_ = nullptr;
-                }
+            void destroyImpl(Impl_::impl_base* base) {
+                // this call can destroy this, so we need to not use thisptr anymore
+                base->destroy();
+
+                // check for weak refs, if zero, we can also delete base
+                if (base->wref() == 0)
+                    delete base;
             }
         };
 

src/signal/Signal.cpp

@@ -10,6 +10,10 @@ using namespace Hyprutils::Memory;
 #define WP CWeakPointer
 
 void Hyprutils::Signal::CSignalBase::emitInternal(void* args) {
+    
+    // Save, an event can destroy thisptr
+    const auto STATICS = m_vStaticListeners;
+    
     if (!m_vListeners.empty()) {
         std::vector<SP<CSignalListener>> listeners;
         listeners.reserve(m_vListeners.size());
@@ -34,10 +38,8 @@ void Hyprutils::Signal::CSignalBase::emitInternal(void* args) {
         listeners.clear();
     }
 
-    if (!m_vStaticListeners.empty()) {
-        const auto statics = m_vStaticListeners;
-
-        for (const auto& l : statics) {
+    if (!STATICS.empty()) {
+        for (const auto& l : STATICS) {
             l->emitInternal(args);
         }
     }

tests/memory/Memory.cpp

@@ -236,6 +236,27 @@ static void testHierarchy() {
     }
 }
 
+class CSelfDestruct {
+  public:
+    SP<CSelfDestruct> self;
+
+    //
+    void youShouldKysNOW() {
+        self.reset();
+    }
+};
+
+static void testSelfDestruct() {
+    auto x                 = makeShared<CSelfDestruct>();
+    x->self                = x;
+    WP<CSelfDestruct> weak = x;
+    x.reset();
+
+    // this has no EXPECT, because all we check is if there isn't a UAF here.
+    // if there is, asan will abort us
+    weak->youShouldKysNOW();
+}
+
 TEST(Memory, memory) {
     SP<int> intPtr    = makeShared<int>(10);
     SP<int> intPtr2   = makeShared<int>(-1337);
@@ -291,4 +312,6 @@ TEST(Memory, memory) {
     testAtomicImpl();
 
     testHierarchy();
+
+    testSelfDestruct();
 }