From b7e847e58b11f5f1e4594b17e14751a87ecb72b1 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 21 Apr 2023 13:36:32 +0200 Subject: [PATCH] AK: Fix crash during teardown of self-owning objects We now null out smart pointers *before* calling unref on the pointee. This ensures that the same smart pointer can't be used to acquire a new reference to the pointee after its destruction has begun. I ran into this when destroying a non-empty IntrusiveList of RefPtrs, but the problem was more general so this fixes it for all of RefPtr, NonnullRefPtr, OwnPtr and NonnullOwnPtr. --- AK/NonnullOwnPtr.h | 6 ++---- AK/NonnullRefPtr.h | 6 +++--- AK/OwnPtr.h | 4 ++-- AK/RefPtr.h | 6 +++--- Tests/AK/CMakeLists.txt | 1 + Tests/AK/TestIntrusiveList.cpp | 6 ++++++ Tests/AK/TestNonnullOwnPtr.cpp | 34 ++++++++++++++++++++++++++++++++++ Tests/AK/TestNonnullRefPtr.cpp | 27 ++++++++++++++++++++++++++- Tests/AK/TestOwnPtr.cpp | 12 ++++++++++++ Tests/AK/TestRefPtr.cpp | 14 +++++++++++++- 10 files changed, 102 insertions(+), 14 deletions(-) create mode 100644 Tests/AK/TestNonnullOwnPtr.cpp diff --git a/AK/NonnullOwnPtr.h b/AK/NonnullOwnPtr.h index 26f8d6b0f5..4c73a246eb 100644 --- a/AK/NonnullOwnPtr.h +++ b/AK/NonnullOwnPtr.h @@ -129,10 +129,8 @@ public: private: void clear() { - if (!m_ptr) - return; - delete m_ptr; - m_ptr = nullptr; + auto* ptr = exchange(m_ptr, nullptr); + delete ptr; } T* m_ptr = nullptr; diff --git a/AK/NonnullRefPtr.h b/AK/NonnullRefPtr.h index 107aedc4a9..b99fef6c4b 100644 --- a/AK/NonnullRefPtr.h +++ b/AK/NonnullRefPtr.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2020, Andreas Kling + * Copyright (c) 2018-2023, Andreas Kling * * SPDX-License-Identifier: BSD-2-Clause */ @@ -94,8 +94,8 @@ public: ALWAYS_INLINE ~NonnullRefPtr() { - unref_if_not_null(m_ptr); - m_ptr = nullptr; + auto* ptr = exchange(m_ptr, nullptr); + unref_if_not_null(ptr); #ifdef SANITIZE_PTRS m_ptr = reinterpret_cast(explode_byte(NONNULLREFPTR_SCRUB_BYTE)); #endif diff --git a/AK/OwnPtr.h b/AK/OwnPtr.h index 0dca19c336..e1c3e40d97 100644 --- a/AK/OwnPtr.h +++ b/AK/OwnPtr.h @@ -106,8 +106,8 @@ public: void clear() { - TDeleter {}(m_ptr); - m_ptr = nullptr; + auto* ptr = exchange(m_ptr, nullptr); + TDeleter {}(ptr); } bool operator!() const { return !m_ptr; } diff --git a/AK/RefPtr.h b/AK/RefPtr.h index 1c28a07d9a..4947fa547b 100644 --- a/AK/RefPtr.h +++ b/AK/RefPtr.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2020, Andreas Kling + * Copyright (c) 2018-2023, Andreas Kling * * SPDX-License-Identifier: BSD-2-Clause */ @@ -220,8 +220,8 @@ public: ALWAYS_INLINE void clear() { - unref_if_not_null(m_ptr); - m_ptr = nullptr; + auto* ptr = exchange(m_ptr, nullptr); + unref_if_not_null(ptr); } bool operator!() const { return !m_ptr; } diff --git a/Tests/AK/CMakeLists.txt b/Tests/AK/CMakeLists.txt index e26a38e41f..8a8622a316 100644 --- a/Tests/AK/CMakeLists.txt +++ b/Tests/AK/CMakeLists.txt @@ -51,6 +51,7 @@ set(AK_TEST_SOURCES TestMemory.cpp TestMemoryStream.cpp TestNeverDestroyed.cpp + TestNonnullOwnPtr.cpp TestNonnullRefPtr.cpp TestNumberFormat.cpp TestOptional.cpp diff --git a/Tests/AK/TestIntrusiveList.cpp b/Tests/AK/TestIntrusiveList.cpp index ef24015247..265d7d6794 100644 --- a/Tests/AK/TestIntrusiveList.cpp +++ b/Tests/AK/TestIntrusiveList.cpp @@ -155,3 +155,9 @@ TEST_CASE(intrusive_nonnull_ref_ptr_intrusive) EXPECT(nonnull_ref_list.is_empty()); } + +TEST_CASE(destroy_nonempty_intrusive_list) +{ + IntrusiveNonnullRefPtrList nonnull_ref_list; + nonnull_ref_list.append(adopt_ref(*new IntrusiveNonnullRefPtrItem)); +} diff --git a/Tests/AK/TestNonnullOwnPtr.cpp b/Tests/AK/TestNonnullOwnPtr.cpp new file mode 100644 index 0000000000..e67c4dde20 --- /dev/null +++ b/Tests/AK/TestNonnullOwnPtr.cpp @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2018-2023, Andreas Kling + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include + +#include +#include +#include + +TEST_CASE(destroy_self_owning_object) +{ + // This test is a little convoluted because SelfOwning can't own itself + // through a NonnullOwnPtr directly. We have to use an intermediate object ("Inner"). + struct SelfOwning { + SelfOwning() + { + } + struct Inner { + explicit Inner(NonnullOwnPtr self) + : self(move(self)) + { + } + NonnullOwnPtr self; + }; + OwnPtr inner; + }; + OwnPtr object = make(); + auto* object_ptr = object.ptr(); + object_ptr->inner = make(object.release_nonnull()); + object_ptr->inner = nullptr; +} diff --git a/Tests/AK/TestNonnullRefPtr.cpp b/Tests/AK/TestNonnullRefPtr.cpp index dfa26a9e2f..60621b4b48 100644 --- a/Tests/AK/TestNonnullRefPtr.cpp +++ b/Tests/AK/TestNonnullRefPtr.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2020, Andreas Kling + * Copyright (c) 2018-2023, Andreas Kling * * SPDX-License-Identifier: BSD-2-Clause */ @@ -8,6 +8,7 @@ #include #include +#include struct Object : public RefCounted { int x; @@ -58,3 +59,27 @@ TEST_CASE(swap_with_self) swap(object, object); EXPECT_EQ(object->ref_count(), 1u); } + +TEST_CASE(destroy_self_owning_refcounted_object) +{ + // This test is a little convoluted because SelfOwningRefCounted can't own itself + // through a NonnullRefPtr directly. We have to use an intermediate object ("Inner"). + struct SelfOwningRefCounted : public RefCounted { + SelfOwningRefCounted() + : inner(make(*this)) + { + } + struct Inner { + explicit Inner(SelfOwningRefCounted& self) + : self(self) + { + } + NonnullRefPtr self; + }; + OwnPtr inner; + }; + RefPtr object = make_ref_counted(); + auto* object_ptr = object.ptr(); + object = nullptr; + object_ptr->inner = nullptr; +} diff --git a/Tests/AK/TestOwnPtr.cpp b/Tests/AK/TestOwnPtr.cpp index 27ee97b5f1..1f73fbac8a 100644 --- a/Tests/AK/TestOwnPtr.cpp +++ b/Tests/AK/TestOwnPtr.cpp @@ -21,3 +21,15 @@ TEST_CASE(should_call_custom_deleter) ptr.clear(); EXPECT_EQ(1u, deleter_call_count); } + +TEST_CASE(destroy_self_owning_object) +{ + struct SelfOwning { + OwnPtr self; + }; + OwnPtr object = make(); + auto* object_ptr = object.ptr(); + object->self = move(object); + object = nullptr; + object_ptr->self = nullptr; +} diff --git a/Tests/AK/TestRefPtr.cpp b/Tests/AK/TestRefPtr.cpp index def65be2c3..ceac8e06ff 100644 --- a/Tests/AK/TestRefPtr.cpp +++ b/Tests/AK/TestRefPtr.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2020, Andreas Kling + * Copyright (c) 2018-2023, Andreas Kling * * SPDX-License-Identifier: BSD-2-Clause */ @@ -153,3 +153,15 @@ TEST_CASE(adopt_ref_if_nonnull) RefPtr failed_allocation = adopt_ref_if_nonnull(null_object); EXPECT_EQ(failed_allocation.is_null(), true); } + +TEST_CASE(destroy_self_owning_refcounted_object) +{ + struct SelfOwningRefCounted : public RefCounted { + RefPtr self; + }; + RefPtr object = make_ref_counted(); + auto* object_ptr = object.ptr(); + object->self = object; + object = nullptr; + object_ptr->self = nullptr; +}