Bug 239338

Summary:	False positive warning when building RefCounter test with GCC 12
Product:	WebKit	Reporter:	Michael Catanzaro <mcatanzaro>
Component:	Tools / Tests	Assignee:	Michael Catanzaro <mcatanzaro>
Status:	RESOLVED FIXED
Severity:	Normal	CC:	mcatanzaro, webkit-bug-importer
Priority:	P2	Keywords:	InRadar
Version:	WebKit Nightly Build
Hardware:	PC
OS:	Linux

Description Michael Catanzaro 2022-04-14 08:55:31 PDT

TODO: I should investigate this more closely

[1736/6690] Building CXX object Tools/TestWebKitAPI/CMakeFiles/TestWTF.dir/Tests/WTF/RefCounter.cpp.o
In file included from /home/mcatanzaro/Projects/WebKit/Tools/TestWebKitAPI/Tests/WTF/RefCounter.cpp:29:
In member function ‘void WTF::RefCounter<T>::Count::deref() [with T = TestWebKitAPI::TestCounterType]’,
    inlined from ‘static void WTF::DefaultRefDerefTraits< <template-parameter-1-1> >::derefIfNotNull(T*) [with T = WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefPtr.h:42:23,
    inlined from ‘WTF::RefPtr<T, PtrTraits, RefDerefTraits>& WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::operator=(std::nullptr_t) [with T = WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count; _PtrTraits = WTF::RawPtrTraits<WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count>; _RefDerefTraits = WTF::DefaultRefDerefTraits<WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count>]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefPtr.h:160:22,
    inlined from ‘virtual void TestWebKitAPI::WTF_RefCounter_Test::TestBody()’ at /home/mcatanzaro/Projects/WebKit/Tools/TestWebKitAPI/Tests/WTF/RefCounter.cpp:152:19:
/home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefCounter.h:101:9: warning: pointer used after ‘void operator delete(void*, std::size_t)’ [-Wuse-after-free]
  101 |     if (m_refCounter && m_refCounter->m_valueDidChange) {
      |         ^~~~~~~~~~~~
In member function ‘void WTF::RefCounter<T>::Count::refCounterWasDeleted() [with T = TestWebKitAPI::TestCounterType]’,
    inlined from ‘void WTF::RefCounter<T>::Count::deref() [with T = TestWebKitAPI::TestCounterType]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefCounter.h:115:13,
    inlined from ‘static void WTF::DefaultRefDerefTraits< <template-parameter-1-1> >::derefIfNotNull(T*) [with T = WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefPtr.h:42:23,
    inlined from ‘WTF::RefPtr<T, PtrTraits, RefDerefTraits>& WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::operator=(std::nullptr_t) [with T = WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count; _PtrTraits = WTF::RawPtrTraits<WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count>; _RefDerefTraits = WTF::DefaultRefDerefTraits<WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count>]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefPtr.h:160:22,
    inlined from ‘virtual void TestWebKitAPI::WTF_RefCounter_Test::TestBody()’ at /home/mcatanzaro/Projects/WebKit/Tools/TestWebKitAPI/Tests/WTF/RefCounter.cpp:150:19:
/home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefCounter.h:128:9: note: call to ‘void operator delete(void*, std::size_t)’ here
  128 |         delete this;
      |         ^~~~~~~~~~~
In member function ‘void WTF::RefCounter<T>::Count::deref() [with T = TestWebKitAPI::TestCounterType]’,
    inlined from ‘static void WTF::DefaultRefDerefTraits< <template-parameter-1-1> >::derefIfNotNull(T*) [with T = WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefPtr.h:42:23,
    inlined from ‘WTF::RefPtr<T, PtrTraits, RefDerefTraits>& WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::operator=(std::nullptr_t) [with T = WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count; _PtrTraits = WTF::RawPtrTraits<WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count>; _RefDerefTraits = WTF::DefaultRefDerefTraits<WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count>]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefPtr.h:160:22,
    inlined from ‘virtual void TestWebKitAPI::WTF_RefCounter_Test::TestBody()’ at /home/mcatanzaro/Projects/WebKit/Tools/TestWebKitAPI/Tests/WTF/RefCounter.cpp:152:19:
/home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefCounter.h:99:7: warning: pointer used after ‘void operator delete(void*, std::size_t)’ [-Wuse-after-free]
   99 |     --m_value;
      |       ^~~~~~~
In member function ‘void WTF::RefCounter<T>::Count::refCounterWasDeleted() [with T = TestWebKitAPI::TestCounterType]’,
    inlined from ‘void WTF::RefCounter<T>::Count::deref() [with T = TestWebKitAPI::TestCounterType]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefCounter.h:115:13,
    inlined from ‘static void WTF::DefaultRefDerefTraits< <template-parameter-1-1> >::derefIfNotNull(T*) [with T = WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefPtr.h:42:23,
    inlined from ‘WTF::RefPtr<T, PtrTraits, RefDerefTraits>& WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::operator=(std::nullptr_t) [with T = WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count; _PtrTraits = WTF::RawPtrTraits<WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count>; _RefDerefTraits = WTF::DefaultRefDerefTraits<WTF::RefCounter<TestWebKitAPI::TestCounterType>::Count>]’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefPtr.h:160:22,
    inlined from ‘virtual void TestWebKitAPI::WTF_RefCounter_Test::TestBody()’ at /home/mcatanzaro/Projects/WebKit/Tools/TestWebKitAPI/Tests/WTF/RefCounter.cpp:150:19:
/home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME-gtk3/WTF/Headers/wtf/RefCounter.h:128:9: note: call to ‘void operator delete(void*, std::size_t)’ here
  128 |         delete this;
      |         ^~~~~~~~~~~

Comment 1 Michael Catanzaro 2022-05-12 15:12:37 PDT

This warning is a false positive. m_refCounter is nullptr at this point. RefCounter<T>::~RefCounter calls m_count->refCounterWasDeleted(), which sets m_refCounter = nullptr. It's a poor man's WeakPtr, basically. I instrumented the code with some WTFLogAlways and it looks like everything is going fine, so I will suppress this warning.

Comment 2 Radar WebKit Bug Importer 2022-05-12 15:12:46 PDT

<rdar://problem/93204514>

Comment 3 Michael Catanzaro 2022-05-12 15:20:19 PDT

Actually wait, I only checked the first of the two warnings. As usual, I commented too soon.

The second warning relates to m_value, not m_refCounter. The only way this could go wrong is if the Counter object itself is already invalid when its member function is called. I highly doubt that is happening, but will add more instrumentation to check this case as well, just to be completely sure. (If that does happen, then it would explain the first warning as well.)

Comment 4 Michael Catanzaro 2022-05-12 16:05:18 PDT

I think we're fine:

// Testing (3g) - deref with callback from 1 -> 0 AFTER RefCounter has been destroyed.
deref: this=0x20cdd10
~Count this=0x20cdd10
// Testing (1b) - Construction without a callback.

The use after free would happen if ~Count were somehow destroyed before Count::deref is called. The opposite is happening. Seems good.

Comment 5 Michael Catanzaro 2022-05-12 17:34:35 PDT

Pull request: https://github.com/WebKit/WebKit/pull/605

Comment 6 EWS 2022-05-13 07:54:52 PDT

Committed r294157 (250527@main): <https://commits.webkit.org/250527@main>

Reviewed commits have been landed. Closing PR #605 and removing active labels.