[vlc-commits] [Git][videolan/vlc][master] 2 commits: qt: catch std::bad_alloc in WindowOpen
Felix Paul Kühne (@fkuehne)
gitlab at videolan.org
Sat Jun 21 10:34:01 UTC 2025
Felix Paul Kühne pushed to branch master at VideoLAN / VLC
Commits:
2e53fbfd by Alexandre Janniaux at 2025-06-21T09:25:09+00:00
qt: catch std::bad_alloc in WindowOpen
When binding the window to the compositor, there is no error code to
signal that allocation failed, but the code is already exception safe,
so handle it through exception handlers to avoid specific memory
handling path.
- - - - -
8013ec70 by Alexandre Janniaux at 2025-06-21T09:25:09+00:00
qt: compositor: fix UAF when destroying window
The Qt window is created when code from qt.cpp's WindowOpen request the
initialisation of the vlc_window instance to the compositor, using
p_intf->p_compositor->setupVoutWindow( p_wnd, &WindowCloseCb );
WindowOpen, and the code in qt.cpp in general, handles the reference
counting of Qt's resources, hence why a destructor callback, WindowCloseCb,
is forwarded to the compositor. The compositor is unable to handle the
reference counting by itself.
When the window is destroyed, it goes through CompositorVideo callbacks
first, leading to WindowCloseCb being called from the compositor.
However, WindowCloseCb will release a reference and trigger the
destruction of the Compositor instance, and thus CompositorVideo, while
being in CompositorVideo::windowDestroy method.
In this case, it triggered use-after-free by writing m_wnd and m_destroyCb
on the CompositorVideo instance.
To prevent this, we can separate the window state from the
CompositorVideo state and modify the window destroy operation to first
notify the CompositorVideo that the window has been destroyed by its
client, and only then trigger the WindowCloseCB function from callback.
The separation of the window state is made by introducing a new
CompositorWindow structure, which is currently allocated statically in
the file since there is only ever a single window allocated, but we can
move to a dynamically allocated window state as soon as a dynamically
defined number of window is required just by changing the initialization
site and release the state in the window destroy operation.
This change simplify the constraints on CompositorVideo which doesn't
need to handle re-entrancy through its destructor, or state corruption
in the middle of the call to CompositorVideo::windowDestroy. It also
align the state handling so that a window being allocated triggers
reference increase from the window, and its destruction triggers
reference decrease from the window operation and state directly,
instead of another component tied to the reference count.
Co-authored-by: Fatih Uzunoglu <fuzun54 at outlook.com>
Original discovery and fix for the UAF in MR !7241.
- - - - -
3 changed files:
- modules/gui/qt/maininterface/compositor.cpp
- modules/gui/qt/maininterface/compositor.hpp
- modules/gui/qt/qt.cpp
Changes:
=====================================
modules/gui/qt/maininterface/compositor.cpp
=====================================
@@ -103,17 +103,28 @@ Compositor* CompositorFactory::createCompositor()
return nullptr;
}
+namespace {
+
+struct CompositorWindow {
+ /* The CompositorVideo instance that created the window */
+ vlc::CompositorVideo *compositor;
+
+ /* Callback setup from commonSetupVoutWindow. It usually comes from the main qt.cpp
+ * file so as to handle the reference counting of Qt's resources. */
+ void (*destroy_cb)(struct vlc_window *wnd);
+};
+
+}
extern "C"
{
-
static int windowEnableCb(vlc_window_t* p_wnd, const vlc_window_cfg_t * cfg)
{
assert(p_wnd->sys);
- auto that = static_cast<vlc::CompositorVideo*>(p_wnd->sys);
+ auto that = static_cast<CompositorWindow*>(p_wnd->sys);
int ret = VLC_EGENERIC;
- QMetaObject::invokeMethod(that, [&](){
- ret = that->windowEnable(cfg);
+ QMetaObject::invokeMethod(that->compositor, [&](){
+ ret = that->compositor->windowEnable(cfg);
}, Qt::BlockingQueuedConnection);
return ret;
}
@@ -121,45 +132,53 @@ static int windowEnableCb(vlc_window_t* p_wnd, const vlc_window_cfg_t * cfg)
static void windowDisableCb(vlc_window_t* p_wnd)
{
assert(p_wnd->sys);
- auto that = static_cast<vlc::CompositorVideo*>(p_wnd->sys);
- QMetaObject::invokeMethod(that, [that](){
- that->windowDisable();
+ auto that = static_cast<CompositorWindow*>(p_wnd->sys);
+ QMetaObject::invokeMethod(that->compositor, [that](){
+ that->compositor->windowDisable();
}, Qt::BlockingQueuedConnection);
}
static void windowResizeCb(vlc_window_t* p_wnd, unsigned width, unsigned height)
{
assert(p_wnd->sys);
- auto that = static_cast<vlc::CompositorVideo*>(p_wnd->sys);
- that->windowResize(width, height);
+ auto that = static_cast<CompositorWindow*>(p_wnd->sys);
+ that->compositor->windowResize(width, height);
}
static void windowDestroyCb(struct vlc_window * p_wnd)
{
assert(p_wnd->sys);
- auto that = static_cast<vlc::CompositorVideo*>(p_wnd->sys);
- that->windowDestroy();
+ auto that = static_cast<CompositorWindow*>(p_wnd->sys);
+ that->compositor->windowDestroy();
+ that->compositor = nullptr;
+
+ if (that->destroy_cb != nullptr)
+ that->destroy_cb(p_wnd);
+
+ /* Destroy p_wnd->sys */
+ delete that;
+ p_wnd->sys = nullptr;
}
static void windowSetStateCb(vlc_window_t* p_wnd, unsigned state)
{
assert(p_wnd->sys);
- auto that = static_cast<vlc::CompositorVideo*>(p_wnd->sys);
- that->windowSetState(state);
+ auto that = static_cast<CompositorWindow*>(p_wnd->sys);
+ that->compositor->windowSetState(state);
}
static void windowUnsetFullscreenCb(vlc_window_t* p_wnd)
{
assert(p_wnd->sys);
- auto that = static_cast<vlc::CompositorVideo*>(p_wnd->sys);
- that->windowUnsetFullscreen();
+ auto that = static_cast<CompositorWindow*>(p_wnd->sys);
+ that->compositor->windowUnsetFullscreen();
}
static void windowSetFullscreenCb(vlc_window_t* p_wnd, const char *id)
{
assert(p_wnd->sys);
- auto that = static_cast<vlc::CompositorVideo*>(p_wnd->sys);
- that->windowSetFullscreen(id);
+ auto that = static_cast<CompositorWindow*>(p_wnd->sys);
+ that->compositor->windowSetFullscreen(id);
}
}
@@ -193,9 +212,17 @@ void CompositorVideo::commonSetupVoutWindow(vlc_window_t* p_wnd, VoutDestroyCb d
nullptr, //window_set_title
};
+ /* Allocate state that will be tracked by the window through vlc_window::sys */
+ // NOTE: no std::nothrow here since no error code, forward the exception
+ auto *compositor_window = new CompositorWindow;
+ compositor_window->compositor = this;
+ compositor_window->destroy_cb = destroyCb;
+
+ /* We currently support only a single window in the compositor. */
+ assert(m_wnd == nullptr);
m_wnd = p_wnd;
- m_destroyCb = destroyCb;
- p_wnd->sys = this;
+
+ p_wnd->sys = compositor_window;
p_wnd->ops = &ops;
p_wnd->info.has_double_click = true;
@@ -223,11 +250,7 @@ void CompositorVideo::windowDestroy()
disconnect(m_videoSurfaceProvider.get(), &VideoSurfaceProvider::surfaceScaleChanged,
this, &CompositorVideo::onSurfaceScaleChanged);
- if (m_destroyCb)
- m_destroyCb(m_wnd);
-
m_wnd = nullptr;
- m_destroyCb = nullptr;
}
void CompositorVideo::windowResize(unsigned width, unsigned height)
=====================================
modules/gui/qt/maininterface/compositor.hpp
=====================================
@@ -188,7 +188,6 @@ protected:
MainCtx* m_mainCtx = nullptr;
- VoutDestroyCb m_destroyCb = nullptr;
std::unique_ptr<VideoWindowHandler> m_videoWindowHandler;
std::unique_ptr<InterfaceWindowHandler> m_interfaceWindowHandler;
=====================================
modules/gui/qt/qt.cpp
=====================================
@@ -1245,7 +1245,7 @@ static void WindowCloseCb( vlc_window_t * )
/**
* Video output window provider
*/
-static int WindowOpen( vlc_window_t *p_wnd )
+static int WindowOpen( vlc_window_t *p_wnd ) try
{
if( !var_InheritBool( p_wnd, "embedded-video" ) )
return VLC_EGENERIC;
@@ -1283,3 +1283,7 @@ static int WindowOpen( vlc_window_t *p_wnd )
return ret ? VLC_SUCCESS : VLC_EGENERIC;
}
}
+catch (std::bad_alloc&)
+{
+ return VLC_ENOMEM;
+}
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/5489cf3a5c309c8d944b1e4d2da63ef7eec8ec1b...8013ec70ca49ff53a86d45a3d40ddef8bb177385
--
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/5489cf3a5c309c8d944b1e4d2da63ef7eec8ec1b...8013ec70ca49ff53a86d45a3d40ddef8bb177385
You're receiving this email because of your account on code.videolan.org.
VideoLAN code repository instance
More information about the vlc-commits
mailing list