Bug 255678 – [GStreamer] GstGLContext use-after-free

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKitGTK

[Patrick Griffis]

When closing the application this triggers ASAN:

    #0 0x7fef0bd3dc03 in WTF::GRefPtr<_GstGLContext>::operator=(_GstGLContext*) /home/tingping/Projects/WebKit/_build/WTF/Headers/wtf/glib/GRefPtr.h:157:14
    #1 0x7fef0bd3dc03 in WebCore::PlatformDisplay::terminateEGLDisplay() /home/tingping/Projects/WebKit/Source/WebCore/platform/graphics/PlatformDisplay.cpp:355:20
    #2 0x7fef0bd3dc03 in WebCore::PlatformDisplay::initializeEGLDisplay()::$_4::operator()() const /home/tingping/Projects/WebKit/Source/WebCore/platform/graphics/PlatformDisplay.cpp:344:26
    #3 0x7fef0bd3dc03 in WebCore::PlatformDisplay::initializeEGLDisplay()::$_4::__invoke() /home/tingping/Projects/WebKit/Source/WebCore/platform/graphics/PlatformDisplay.cpp:341:21
    #4 0x7feef75820b4 in __run_exit_handlers (/lib64/libc.so.6+0x3f0b4) (BuildId: 81daba31ee66dbd63efdc4252a872949d874d136)
    #5 0x7feef758222f in exit (/lib64/libc.so.6+0x3f22f) (BuildId: 81daba31ee66dbd63efdc4252a872949d874d136)
    #6 0x7feef756a516 in __libc_start_call_main (/lib64/libc.so.6+0x27516) (BuildId: 81daba31ee66dbd63efdc4252a872949d874d136)
    #7 0x7feef756a5c8 in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x275c8) (BuildId: 81daba31ee66dbd63efdc4252a872949d874d136)
    #8 0x255454 in _start (/home/tingping/Projects/WebKit/_build/bin/WebKitWebProcess+0x255454) (BuildId: 9f6dd881b3dab5d1)

0x60f00003eaf8 is located 136 bytes inside of 168-byte region [0x60f00003ea70,0x60f00003eb18)
freed by thread T0 here:
    #0 0x309218 in __interceptor_free.part.0 (/home/tingping/Projects/WebKit/_build/bin/WebKitWebProcess+0x309218) (BuildId: 9f6dd881b3dab5d1)
    #1 0x7feeffe4d986 in bmalloc::DebugHeap::free(void*) /home/tingping/Projects/WebKit/Source/bmalloc/bmalloc/DebugHeap.cpp:140:5
    #2 0x7feeffe4d986 in pas_debug_heap_free /home/tingping/Projects/WebKit/Source/bmalloc/bmalloc/DebugHeap.cpp:239:31
    #3 0x7feeffea1625 in bmalloc_heap_config_specialized_try_deallocate_not_small_exclusive_segregated /home/tingping/Projects/WebKit/Source/bmalloc/libpas/src/libpas/pas_deallocate.h:104:9
    #4 0x7feeffc424c8 in pas_try_deallocate_impl(pas_thread_local_cache*, void*, pas_heap_config, pas_deallocation_mode) /home/tingping/Projects/WebKit/_build/bmalloc/Headers/bmalloc/pas_deallocate.h:171:12
    #5 0x7feeffc424c8 in pas_try_deallocate(void*, pas_heap_config, pas_deallocation_mode) /home/tingping/Projects/WebKit/_build/bmalloc/Headers/bmalloc/pas_deallocate.h:207:12
    #6 0x7feeffc424c8 in pas_deallocate(void*, pas_heap_config) /home/tingping/Projects/WebKit/_build/bmalloc/Headers/bmalloc/pas_deallocate.h:213:5
    #7 0x7feeffc424c8 in bmalloc_deallocate_inline(void*) /home/tingping/Projects/WebKit/_build/bmalloc/Headers/bmalloc/bmalloc_heap_inlines.h:572:5
    #8 0x7feeffc424c8 in bmalloc::api::free(void*, bmalloc::HeapKind) /home/tingping/Projects/WebKit/_build/bmalloc/Headers/bmalloc/bmalloc.h:145:5
    #9 0x7feeffc424c8 in WTF::fastFree(void*) /home/tingping/Projects/WebKit/Source/WTF/wtf/FastMalloc.cpp:566:5
    #10 0x7fef0be8b545 in WebCore::PlatformDisplay::operator delete(void*) /home/tingping/Projects/WebKit/Source/WebCore/platform/graphics/PlatformDisplay.h:68:44
    #11 0x7fef0be8b545 in WebCore::PlatformDisplayWayland::~PlatformDisplayWayland() /home/tingping/Projects/WebKit/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:95:1
    #12 0x7fef0bd3deaa in std::default_delete<WebCore::PlatformDisplay>::operator()(WebCore::PlatformDisplay*) const /usr/bin/../lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/unique_ptr.h:95:2
    #13 0x7fef0bd3deaa in std::unique_ptr<WebCore::PlatformDisplay, std::default_delete<WebCore::PlatformDisplay>>::~unique_ptr() /usr/bin/../lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/unique_ptr.h:396:4
    #14 0x7feef75820b4 in __run_exit_handlers (/lib64/libc.so.6+0x3f0b4) (BuildId: 81daba31ee66dbd63efdc4252a872949d874d136)

previously allocated by thread T0 here:
    #0 0x30a1e7 in malloc (/home/tingping/Projects/WebKit/_build/bin/WebKitWebProcess+0x30a1e7) (BuildId: 9f6dd881b3dab5d1)
    #1 0x7feeffe4d606 in bmalloc::DebugHeap::malloc(unsigned long, bmalloc::FailureAction) /home/tingping/Projects/WebKit/Source/bmalloc/bmalloc/DebugHeap.cpp:118:20
    #2 0x7feeffe4d606 in pas_debug_heap_malloc /home/tingping/Projects/WebKit/Source/bmalloc/bmalloc/DebugHeap.cpp:224:38
    #3 0x7feeffe56a58 in bmalloc_allocate_impl_casual_case(unsigned long, unsigned long) /home/tingping/Projects/WebKit/Source/bmalloc/libpas/src/libpas/pas_local_allocator_inlines.h
    #4 0x7feeffe56558 in bmalloc_allocate_casual /home/tingping/Projects/WebKit/Source/bmalloc/libpas/src/libpas/bmalloc_heap.c:64:19
    #5 0x7feeffc3fdf2 in bmalloc_allocate_inline(unsigned long) /home/tingping/Projects/WebKit/_build/bmalloc/Headers/bmalloc/bmalloc_heap_inlines.h:120:12
    #6 0x7feeffc3fdf2 in bmalloc::api::malloc(unsigned long, bmalloc::HeapKind) /home/tingping/Projects/WebKit/_build/bmalloc/Headers/bmalloc/bmalloc.h:72:16
    #7 0x7feeffc3fdf2 in WTF::fastMalloc(unsigned long) /home/tingping/Projects/WebKit/Source/WTF/wtf/FastMalloc.cpp:533:20
    #8 0x7fef0be8b179 in WebCore::PlatformDisplay::operator new(unsigned long) /home/tingping/Projects/WebKit/Source/WebCore/platform/graphics/PlatformDisplay.h:68:44
    #9 0x7fef0be8b179 in WebCore::PlatformDisplayWayland::create(_GdkDisplay*) /home/tingping/Projects/WebKit/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:75:68
    #10 0x7fef0bd37b79 in WebCore::PlatformDisplay::createPlatformDisplay() /home/tingping/Projects/WebKit/Source/WebCore/platform/graphics/PlatformDisplay.cpp
    #11 0x7fef0bd3dd2c in WebCore::PlatformDisplay::sharedDisplay()::$_2::operator()() const /home/tingping/Projects/WebKit/Source/WebCore/platform/graphics/PlatformDisplay.cpp:175:19
    #12 0x7fef0bd3dd2c in void std::__invoke_impl<void, WebCore::PlatformDisplay::sharedDisplay()::$_2>(std::__invoke_other, WebCore::PlatformDisplay::sharedDisplay()::$_2&&) /usr/bin/../lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/invoke.h:61:14
    #13 0x7fef0bd3dd2c in std::__invoke_result<WebCore::PlatformDisplay::sharedDisplay()::$_2>::type std::__invoke<WebCore::PlatformDisplay::sharedDisplay()::$_2>(WebCore::PlatformDisplay::sharedDisplay()::$_2&&) /usr/bin/../lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/invoke.h:96:14
    #14 0x7fef0bd3dd2c in void std::call_once<WebCore::PlatformDisplay::sharedDisplay()::$_2>(std::once_flag&, WebCore::PlatformDisplay::sharedDisplay()::$_2&&)::'lambda'()::operator()() const /usr/bin/../lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/mutex:852:4
    #15 0x7fef0bd3dd2c in std::once_flag::_Prepare_execution::_Prepare_execution<void std::call_once<WebCore::PlatformDisplay::sharedDisplay()::$_2>(std::once_flag&, WebCore::PlatformDisplay::sharedDisplay()::$_2&&)::'lambda'()>(WebCore::PlatformDisplay::sharedDisplay()::$_2&)::'lambda'()::operator()() const /usr/bin/../lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/mutex:788:21
    #16 0x7fef0bd3dd2c in std::once_flag::_Prepare_execution::_Prepare_execution<void std::call_once<WebCore::PlatformDisplay::sharedDisplay()::$_2>(std::once_flag&, WebCore::PlatformDisplay::sharedDisplay()::$_2&&)::'lambda'()>(WebCore::PlatformDisplay::sharedDisplay()::$_2&)::'lambda'()::__invoke() /usr/bin/../lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/mutex:788:16
    #17 0x7feef75d3086 in __pthread_once_slow (/lib64/libc.so.6+0x90086) (BuildId: 81daba31ee66dbd63efdc4252a872949d874d136)

SUMMARY: AddressSanitizer: heap-use-after-free /home/tingping/Projects/WebKit/_build/WTF/Headers/wtf/glib/GRefPtr.h:157:14 in WTF::GRefPtr<_GstGLContext>::operator=(_GstGLContext*)

[Radar WebKit Bug Importer]

<rdar://problem/108283764>

[Michael Catanzaro]

Ehh, I guess there's no way for attackers to abuse this if it only happens when closing, so probably not a security problem.

[Michael Catanzaro]

So my opinion is we should give up on calling terminateEGLDisplay() due to the long history of bugs associated with attempting to do this (bug #145832, bug #174789, bug #176490, and surely more), and because it's not necessary unless there are global resource leaks in low-quality graphics drivers, which should be fixed in affected drivers rather than worked around in WebKit.

I also understand that would be inconvenient for certain downstream purposes, so... I guess let's try using std::atexit to destroy the shared display instead of leaving it up to chance; maybe that will work? Our current code expects exit handlers manually registered using std::atexit to always execute before static variable destructors, but I suppose that's a bad assumption. Also, PlatformDisplay is used on Windows now, where order of destruction of static variables is undefined, so it's unsafe regardless. (Only Linux-specific code is allowed to put types that are not trivially-destructible into static variables.)

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #3)
> Also, PlatformDisplay is used on Windows now, where order of
> destruction of static variables is undefined, so it's unsafe regardless.
> (Only Linux-specific code is allowed to put types that are not
> trivially-destructible into static variables.)

In fairness, PlatformDisplay::sharedDisplay already has a preprocessor guard to ensure the shared display does not get destroyed on Windows, so that's good.

[Michael Catanzaro]

Since you have an asan build handy, please test this:

diff --git a/Source/WebCore/platform/graphics/PlatformDisplay.cpp b/Source/WebCore/platform/graphics/PlatformDisplay.cpp
index cdd10a7d1351..c645522892ca 100644
--- a/Source/WebCore/platform/graphics/PlatformDisplay.cpp
+++ b/Source/WebCore/platform/graphics/PlatformDisplay.cpp
@@ -168,12 +168,18 @@ PlatformDisplay& PlatformDisplay::sharedDisplay()
     return *display;
 #else
     static std::once_flag onceFlag;
-    IGNORE_CLANG_WARNINGS_BEGIN("exit-time-destructors")
-    static std::unique_ptr<PlatformDisplay> display;
-    IGNORE_CLANG_WARNINGS_END
+    static PlatformDisplay* display;
     std::call_once(onceFlag, []{
-        display = createPlatformDisplay();
+        display = createPlatformDisplay().release();
+        std::atexit([] {
+            // PlatformDisplay::sharedDisplay should always be called before the first call to
+            // PlatformDisplay::initializeEGLDisplay, so this exit handler should always run after
+            // the exit handler registered in that function.
+            delete display;
+            display = nullptr;
+        });
     });
+    ASSERT(display);
     return *display;
 #endif
 }

Does that fix the problem? If so, I'll create a merge request.

[Carlos Garcia Campos]

hmm, does this mean that the display is destroyed before the at exit handlers? that's not expected to happen. I also think we should not initialize egl in the wl display unless explicitly requested (which shouldn't happen 99% of the times, because the shared display for compositing is either WPE or headless). Another thing we can do is to try to reuse the GTK EGL display instead of always creating a new one, and in that case we don't own the display so we don't need to terminate it either.

[Carlos Garcia Campos]

It's also unexpected that PlatformDisplayWayland has a GstGLContext, since it's only set in the shared display for compositing...

[Carlos Garcia Campos]

I don't understand how this can happen, but bug #255721 might work around this problem.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #6)
> hmm, does this mean that the display is destroyed before the at exit
> handlers? that's not expected to happen.

It's happening.

(In reply to Carlos Garcia Campos from comment #8)
> I don't understand how this can happen

We currently have nothing to guarantee the order of destruction. My suggestion in comment #5 should fix that.

[Patrick Griffis]

(In reply to Michael Catanzaro from comment #5)
> Does that fix the problem? If so, I'll create a merge request.

This did not.

[Patrick Griffis]

(In reply to Carlos Garcia Campos from comment #8)
> I don't understand how this can happen, but bug #255721 might work around
> this problem.

This PR seems to resolve it!

[Michael Catanzaro]

Huh, OK then.

*** This bug has been marked as a duplicate of bug 255721 ***

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #9)
> (In reply to Carlos Garcia Campos from comment #6)
> > hmm, does this mean that the display is destroyed before the at exit
> > handlers? that's not expected to happen.
> 
> It's happening.

Could it be that it happens only with ASAN?

> (In reply to Carlos Garcia Campos from comment #8)
> > I don't understand how this can happen
> 
> We currently have nothing to guarantee the order of destruction. My
> suggestion in comment #5 should fix that.

exit handlers should happen before static vars are destroyed, at least we assumed that and we have always had an assert in platform display destructor to ensure that.

[Carlos Garcia Campos]

(In reply to Patrick Griffis from comment #11)
> (In reply to Carlos Garcia Campos from comment #8)
> > I don't understand how this can happen, but bug #255721 might work around
> > this problem.
> 
> This PR seems to resolve it!

It actually woks around it. We still don't expect exit handlers to happen before the destructor, if that's a debug build you should be getting an assert.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #13)
> Could it be that it happens only with ASAN?

I don't think so.

> exit handlers should happen before static vars are destroyed, at least we
> assumed that and we have always had an assert in platform display destructor
> to ensure that.

static variables are themselves destroyed in exit handlers (as you can see from the backtrace above).

[Michael Catanzaro]

Seems worth changing that ASSERT to a RELEASE_ASSERT for some additional confidence.