Bug 282910 – [WebDriver][GLIB] Potential use after free due to early SessionHost destruction

• Status: RESOLVED
• Resolution: FIXED
• Component: WebDriver

[Lauro Moura]

Steps to reproduce

- Start the WPEWebDriver under valgrind
  - e.g.: valgrind --tool=memcheck WPEWebDriver --port=4444 --host=all --verbose
- Connect (creates a session) and disconnect (closes a session)

Expected result: No invalid write errors

Actual result: Invalid write on what looks like a freed region:

** (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== 


Inspecting, this is the chain of events:

- `WebDriverService::deleteSession` is called, which exchanges `WebDriverService.m_session` into a local variable, whose ownership is passed to the `session->close` completion handler.
- Closing the underlying browser triggers the message `DidClose` from the browser to the WebDriver SessionHost's `SocketConnection`, handled by `SessionHostGLIB.cpp`
- `SessionHost::connectionDidClose` (SessionHostGlib.cpp) in turn, calls `SessionHost::inspectorDisconnected()` (SessionHost.cpp), which will clean up the pending actions, which includes the mentioned session close operation.
- `session->close` completion handler returns and frees up the exchanged `Session` reference, destroying it.
- As `SessionHost` is an `unique_ptr` held by `Session`, it gets destroyed too, taking with it its reference to the socket connection, etc., all this potentially happening in the middle of the `inspectorDisconnected` call.
- In the case reported by valgrind, this also triggers an issue where the `SocketConnection` held by the SessionHost gets freed before properly cleaning up the related GSocketMonitors.

While this mostly works fine as we have a single session around not messing with the freed pointers contents, we need to fix this to ensure the proper lifetime management and other problems.

Maybe we could make SessionHost refcounted, adding a "protectedThis" while invoking the pending operations?

[Lauro Moura]

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

[EWS]

Committed 286523@main (623870b2aa3c): <https://commits.webkit.org/286523@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/139789036>