[webkit-changes] [WebKit/WebKit] 711a78: RefCounted should have precise tracking for RefPtr...

geoffreygaren noreply at github.com
Sun Sep 15 13:36:26 PDT 2024 

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 711a78ba787296a441d0805b854da8f602bfd66d
      https://github.com/WebKit/WebKit/commit/711a78ba787296a441d0805b854da8f602bfd66d
  Author: Geoffrey Garen <ggaren at apple.com>
  Date:   2024-09-15 (Sun, 15 Sep 2024)

  Changed paths:
    M Source/WTF/wtf/RefCounted.cpp
    M Source/WTF/wtf/RefCounted.h
    M Source/WTF/wtf/SizeLimits.cpp
    M Source/WTF/wtf/ThreadSafeRefCounted.h
    M Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp

  Log Message:
  -----------
  RefCounted should have precise tracking for RefPtrs that escape from destructors
https://bugs.webkit.org/show_bug.cgi?id=279723
rdar://136026234

Reviewed by Ryosuke Niwa.

Inside an object's destructor, if we call ref() on the object without a
balancing call to deref(), we 'escape' a RefPtr to the object. When such
a RefPtr is dereferenced or destroyed, it will use-after-free the object.

Our current approach to detecting this error is to ASSERT when we call
ref() on an object inside its destructor, and then, in cases we believe
to be false positives, silence that ASSERT using
RefPtrAllowingPartiallyDestroyed.

There are downsides to this approach:

1. RefPtrAllowingPartiallyDestroyed complex-ifies our refcounting rules,
making them hard to document and explain, and hard to write static
analyses for; and even causing confusion in software update merges.

2. In the RefPtrAllowingPartiallyDestroyed case, our detection of
use-after-free is cleverly probabilistic, but not deterministic.

3. The approach is not compatible with Swift-C++.

This patch replaces our ASSERT in ref(), stating that deletion must not
have begun, with an ASSERT after destruction is over, stating that there
must not be any outstanding references left.

To aid debugging when the ASSERT fires, we keep a lock-free global log
of all ref() calls that happen during destruction. Before we ASSERT,
we dump the log.

This should allow use to remove RefPtrAllowingPartiallyDestroyed in a
follow-up patch.

Example output from unit tests:

    TestWTF.WTF_RefPtr.DanglingRefPtr
        Error: Dangling RefPtr: 0x10f0041d0
        This means that a ref() during destruction was not balanced by a deref() before destruction ended.
        Below are the most recent ref()'s during destruction for this address.

        1   0x10110724c WTF::RefCountedBase::refAllowingPartiallyDestroyed() const
        2   0x101107218 WTF::RefCountedBase::ref() const
        3   0x1016ae820 TestWebKitAPI::DanglingRefPtrTest::~DanglingRefPtrTest()
        4   0x1016ae790 TestWebKitAPI::DanglingRefPtrTest::~DanglingRefPtrTest()
        5   0x1016ae754 WTF::RefCounted<TestWebKitAPI::DanglingRefPtrTest>::deref() const
        6   0x1016ae6d4 WTF::DefaultRefDerefTraits<TestWebKitAPI::DanglingRefPtrTest>::derefIfNotNull(TestWebKitAPI::DanglingRefPtrTest*)
        7   0x1016a95ac WTF::RefPtr<TestWebKitAPI::DanglingRefPtrTest, WTF::RawPtrTraits<TestWebKitAPI::DanglingRefPtrTest>, WTF::DefaultRefDerefTraits<TestWebKitAPI::DanglingRefPtrTest>>::operator=(std::nullptr_t)
        8   0x1016a94c0 TestWebKitAPI::WTF_RefPtr_DanglingRefPtr_Test::TestBody()
        9   0x101b5ea8c void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        10  0x101b0c254 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        11  0x101b0c188 testing::Test::Run()
        12  0x101b0d388 testing::TestInfo::Run()

        1   0x10110724c WTF::RefCountedBase::refAllowingPartiallyDestroyed() const
        2   0x101107218 WTF::RefCountedBase::ref() const
        3   0x1016ae7e8 TestWebKitAPI::DanglingRefPtrTest::~DanglingRefPtrTest()
        4   0x1016ae790 TestWebKitAPI::DanglingRefPtrTest::~DanglingRefPtrTest()
        5   0x1016ae754 WTF::RefCounted<TestWebKitAPI::DanglingRefPtrTest>::deref() const
        6   0x1016ae6d4 WTF::DefaultRefDerefTraits<TestWebKitAPI::DanglingRefPtrTest>::derefIfNotNull(TestWebKitAPI::DanglingRefPtrTest*)
        7   0x1016a95ac WTF::RefPtr<TestWebKitAPI::DanglingRefPtrTest, WTF::RawPtrTraits<TestWebKitAPI::DanglingRefPtrTest>, WTF::DefaultRefDerefTraits<TestWebKitAPI::DanglingRefPtrTest>>::operator=(std::nullptr_t)
        8   0x1016a94c0 TestWebKitAPI::WTF_RefPtr_DanglingRefPtr_Test::TestBody()
        9   0x101b5ea8c void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        10  0x101b0c254 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        11  0x101b0c188 testing::Test::Run()
        12  0x101b0d388 testing::TestInfo::Run()

    TestWTF.WTF_RefPtr.DanglingRefPtrThreadSafe
        Error: Dangling RefPtr: 0x1110041d0
        This means that a ref() during destruction was not balanced by a deref() before destruction ended.
        Below are the most recent ref()'s during destruction for this address.

        1   0x1026b6320 WTF::ThreadSafeRefCountedBase::refAllowingPartiallyDestroyed() const
        2   0x1026b62f4 WTF::ThreadSafeRefCountedBase::ref() const
        3   0x102cfad70 TestWebKitAPI::DanglingRefPtrTestThreadSafe::~DanglingRefPtrTestThreadSafe()
        4   0x102cfad08 TestWebKitAPI::DanglingRefPtrTestThreadSafe::~DanglingRefPtrTestThreadSafe()
        5   0x102cfacd0 WTF::ThreadSafeRefCounted<TestWebKitAPI::DanglingRefPtrTestThreadSafe, (WTF::DestructionThread)0>::deref() const
        6   0x102cfac4c WTF::DefaultRefDerefTraits<TestWebKitAPI::DanglingRefPtrTestThreadSafe>::derefIfNotNull(TestWebKitAPI::DanglingRefPtrTestThreadSafe*)
        7   0x102cf577c WTF::RefPtr<TestWebKitAPI::DanglingRefPtrTestThreadSafe, WTF::RawPtrTraits<TestWebKitAPI::DanglingRefPtrTestThreadSafe>, WTF::DefaultRefDerefTraits<TestWebKitAPI::DanglingRefPtrTestThreadSafe>>::operator=(std::nullptr_t)
        8   0x102cf5690 TestWebKitAPI::WTF_RefPtr_DanglingRefPtrThreadSafe_Test::TestBody()
        9   0x1031aaa6c void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        10  0x103158234 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        11  0x103158168 testing::Test::Run()
        12  0x103159368 testing::TestInfo::Run()

        1   0x1026b6320 WTF::ThreadSafeRefCountedBase::refAllowingPartiallyDestroyed() const
        2   0x1026b62f4 WTF::ThreadSafeRefCountedBase::ref() const
        3   0x102cfad38 TestWebKitAPI::DanglingRefPtrTestThreadSafe::~DanglingRefPtrTestThreadSafe()
        4   0x102cfad08 TestWebKitAPI::DanglingRefPtrTestThreadSafe::~DanglingRefPtrTestThreadSafe()
        5   0x102cfacd0 WTF::ThreadSafeRefCounted<TestWebKitAPI::DanglingRefPtrTestThreadSafe, (WTF::DestructionThread)0>::deref() const
        6   0x102cfac4c WTF::DefaultRefDerefTraits<TestWebKitAPI::DanglingRefPtrTestThreadSafe>::derefIfNotNull(TestWebKitAPI::DanglingRefPtrTestThreadSafe*)
        7   0x102cf577c WTF::RefPtr<TestWebKitAPI::DanglingRefPtrTestThreadSafe, WTF::RawPtrTraits<TestWebKitAPI::DanglingRefPtrTestThreadSafe>, WTF::DefaultRefDerefTraits<TestWebKitAPI::DanglingRefPtrTestThreadSafe>>::operator=(std::nullptr_t)
        8   0x102cf5690 TestWebKitAPI::WTF_RefPtr_DanglingRefPtrThreadSafe_Test::TestBody()
        9   0x1031aaa6c void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        10  0x103158234 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        11  0x103158168 testing::Test::Run()
        12  0x103159368 testing::TestInfo::Run()

* Source/WTF/wtf/RefCounted.cpp:
(WTF::RefLogStackShot::RefLogStackShot):
(WTF::RefLogStackShot::ptr):
(WTF::RefLogStackShot::print):
(WTF::RefLogSingleton::append):
(WTF::RefLogSingleton::forEachLIFO):
(WTF::RefCountedBase::logRefDuringDestruction):
(WTF::RefCountedBase::printRefDuringDestructionLogAndCrash): This is the
infrastructure for our log. It's a lock-free ring buffer. Very old entries do
get dropped, but that's OK because there aren't many ref() calls in the average
destructor -- and our regression test shows that it's actually a minor benefit
to keep a short-ish log in order to disambiguate addresses that have been
freed and then recycled.

* Source/WTF/wtf/RefCounted.h:
(WTF::RefCountedBase::refAllowingPartiallyDestroyed const):
(WTF::RefCountedBase::ref const):
(WTF::RefCountedBase::hasOneRef const):
(WTF::RefCountedBase::relaxAdoptionRequirement):
(WTF::RefCountedBase::applyRefDuringDestructionCheck const):
(WTF::RefCountedBase::derefAllowingPartiallyDestroyedBase const):
(WTF::RefCountedBase::derefBase const):
(WTF::RefCountedBase::deletionHasBegun const):
(WTF::RefCountedBase::~RefCountedBase):
(WTF::RefCountedBase::deletionHasEnded const): Deleted. No need to ASSERT
when using a pointer anymore. We check for pointers that have escaped
directly by checking the refcount after destruction is over.

* Source/WTF/wtf/SizeLimits.cpp: Smaller now.

* Source/WTF/wtf/ThreadSafeRefCounted.h:
(WTF::ThreadSafeRefCountedBase::ref const):
(WTF::ThreadSafeRefCountedBase::refAllowingPartiallyDestroyed const):
(WTF::ThreadSafeRefCountedBase::hasOneRef const):
(WTF::ThreadSafeRefCountedBase::derefBaseWithoutDeletionCheck const):
(WTF::ThreadSafeRefCountedBase::derefBase const):
(WTF::ThreadSafeRefCountedBase::applyRefDuringDestructionCheck const):
(WTF::ThreadSafeRefCountedBase::~ThreadSafeRefCountedBase):
(WTF::ThreadSafeRefCountedBase::deletionHasEnded const): Deleted. Same as RefCounted.

* Tools/TestWebKitAPI/Tests/WTF/RefPtr.cpp:
(TestWebKitAPI::DanglingRefPtrTest::create):
(TestWebKitAPI::DanglingRefPtrTest::~DanglingRefPtrTest):
(TestWebKitAPI::TEST(WTF_RefPtr, DanglingRefPtr)):
(TestWebKitAPI::DanglingRefPtrTestThreadSafe::create):
(TestWebKitAPI::DanglingRefPtrTestThreadSafe::~DanglingRefPtrTestThreadSafe):
(TestWebKitAPI::TEST(WTF_RefPtr, DanglingRefPtrThreadSafe)): Unit tests.

Canonical link: https://commits.webkit.org/283685@main



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications

More information about the webkit-changes mailing list