Bug 283053 – [GLIB] Invalid write reported by Valgrind on GSocketMonitor::sourceSocketCallback

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Lauro Moura]

bug282910 originally reported an invalid memory access in GSocketMonitor::sourceSocketCallback. Originally I thought it was related to the SessionHost destructor being called prematurely, but even after ensuring we'd delete the SessionHost after it returns from the socket message handlers in https://github.com/WebKit/WebKit/pull/36533, the error continues.

From the trace, somehow erasing `monitor->m_callback` (added in 285339@main) is triggering the destruction of the monitor immediately while still inside its socketSourceCallback. Does not seem to bring issues in release builds, but running under valgrind shows an invalid write while updating m_shouldDestroyCallback

Maybe it's a matter of ensuring cleaning the callback is the last action in the sourceSocket callback.

```
** (WPEWebProcess:2182124): WARNING **: 23:33:30.451: RemoteInspector failed to connect to inspector server at: 127.0.0.1:40907: GDBus.Error:org.freedesktop.portal.Error.NotAllowed: This call is not available inside the sandbox
==2181500== Invalid write of size 1
==2181500==    at 0x451DC8: WTF::GSocketMonitor::socketSourceCallback(_GSocket*, GIOCondition, WTF::GSocketMonitor*) (GSocketMonitor.cpp:51)
==2181500==    by 0x4BCDBB0: socket_source_dispatch (gsocket.c:4267)
==2181500==    by 0x4DC148D: g_main_dispatch.lto_priv.0 (gmain.c:3344)
==2181500==    by 0x4E20716: UnknownInlinedFun (gmain.c:4152)
==2181500==    by 0x4E20716: g_main_context_iterate_unlocked.isra.0 (gmain.c:4217)
==2181500==    by 0x4DC1F76: g_main_loop_run (gmain.c:4419)
==2181500==    by 0x453161: WTF::RunLoop::run() (RunLoopGLib.cpp:108)
==2181500==    by 0x3781F5: WebDriver::WebDriverService::run(int, char**) (WebDriverService.cpp:186)
==2181500==    by 0x375AED: main (WebDriverMain.cpp:39)
==2181500==  Address 0x7aa7619 is 73 bytes inside a block of size 128 free'd
==2181500==    at 0x484988F: free (vg_replace_malloc.c:985)
==2181500==    by 0x45E459: bmalloc::DebugHeap::free(void*) (DebugHeap.cpp:139)
==2181500==    by 0x45E739: pas_debug_heap_free (DebugHeap.cpp:238)
==2181500==    by 0x4BAB8C: pas_try_deallocate_slow_no_cache (pas_deallocate.c:135)
==2181500==    by 0x3E0BEC: pas_try_deallocate (pas_deallocate.h:209)
==2181500==    by 0x3E0C75: pas_deallocate (pas_deallocate.h:221)
==2181500==    by 0x3E579F: bmalloc_deallocate_inline (bmalloc_heap_inlines.h:592)
==2181500==    by 0x3E5EFC: free (bmalloc.h:187)
==2181500==    by 0x3E5EFC: WTF::fastFree(void*) (FastMalloc.cpp:617)
==2181500==    by 0x3BD846: WTF::RefCounted<WTF::SocketConnection>::operator delete(void*) (RefCounted.h:199)
==2181500==    by 0x3BC218: WTF::RefCounted<WTF::SocketConnection>::deref() const (RefCounted.h:204)
==2181500==    by 0x3BA9D1: WTF::DefaultRefDerefTraits<WTF::SocketConnection>::derefIfNotNull(WTF::SocketConnection*) (Ref.h:62)
==2181500==    by 0x3B89BC: WTF::Ref<WTF::SocketConnection, WTF::RawPtrTraits<WTF::SocketConnection>, WTF::DefaultRefDerefTraits<WTF::SocketConnection> >::~Ref() (Ref.h:82)
==2181500==  Block was alloc'd at
==2181500==    at 0x4846828: malloc (vg_replace_malloc.c:442)
==2181500==    by 0x45E370: bmalloc::DebugHeap::malloc(unsigned long, bmalloc::FailureAction) (DebugHeap.cpp:118)
==2181500==    by 0x45E5C6: pas_debug_heap_malloc (DebugHeap.cpp:223)
==2181500==    by 0x462DF3: pas_debug_heap_allocate (pas_debug_heap.h:106)
==2181500==    by 0x46374E: pas_try_allocate_intrinsic_impl_casual_case (pas_try_allocate_intrinsic.h:105)
==2181500==    by 0x463F44: bmalloc_allocate_impl_casual_case (bmalloc_heap_inlines.h:69)
==2181500==    by 0x464441: bmalloc_allocate_casual (bmalloc_heap.c:64)
==2181500==    by 0x3E4A21: bmalloc_allocate_inline (bmalloc_heap_inlines.h:120)
==2181500==    by 0x3E5995: malloc (bmalloc.h:79)
==2181500==    by 0x3E5995: WTF::fastMalloc(unsigned long) (FastMalloc.cpp:559)
==2181500==    by 0x3B892B: WTF::RefCounted<WTF::SocketConnection>::operator new(unsigned long) (RefCounted.h:199)
==2181500==    by 0x3B51A7: WTF::SocketConnection::create(WTF::GRefPtr<_GSocketConnection>&&, WTF::HashMap<WTF::CString, std::pair<WTF::CString, void (*)(WTF::SocketConnection&, _GVariant*, void*)>, WTF::DefaultHash<WTF::CString>, WTF::HashTraits<WTF::CString>, WTF::HashTraits<std::pair<WTF::CString, void (*)(WTF::SocketConnection&, _GVariant*, void*)> >, WTF::HashTableTraits, (WTF::ShouldValidateKey)0> const&, void*) (SocketConnection.h:41)
==2181500==    by 0x3B6A91: WebDriver::SessionHost::connectToBrowser(std::unique_ptr<WebDriver::ConnectToBrowserAsyncData, std::default_delete<WebDriver::ConnectToBrowserAsyncData> >&&)::{lambda()#1}::operator()()::{lambda(_GObject*, _GAsyncResult*, void*)#1}::operator()(_GObject*, _GAsyncResult*, void*) const (SessionHostGlib.cpp:217)
==2181500== 
```

Tentative patch incoming.

[Michael Catanzaro]

(In reply to Lauro Moura from comment #0)
> Tentative patch incoming.

Thanks :)

[Lauro Moura]

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

[EWS]

Committed 286596@main (5d047d3c9925): <https://commits.webkit.org/286596@main>

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