From 8e7c01b5615f295611530e7cda61c5b10083370a Mon Sep 17 00:00:00 2001 From: Ilya Zhuravlev Date: Mon, 24 Dec 2018 01:50:58 -0500 Subject: [PATCH 1/2] loadingscreen: Fix UaF in loading screen. When the CopyFramebufferToTextureCallback callback is called, in its operator() it resets setInitialDrawCallback by providing a NULL pointer. However, this causes the callback to get deleted. In turn, the "this" pointer is invalidated. When execution returns to DrawCallback::run, it accesses a _nestedCallback member of deleted "this" which is UB. --- apps/openmw/mwgui/loadingscreen.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/apps/openmw/mwgui/loadingscreen.cpp b/apps/openmw/mwgui/loadingscreen.cpp index 7f641d408..68cf0439a 100644 --- a/apps/openmw/mwgui/loadingscreen.cpp +++ b/apps/openmw/mwgui/loadingscreen.cpp @@ -141,10 +141,6 @@ namespace MWGui int w = renderInfo.getCurrentCamera()->getViewport()->width(); int h = renderInfo.getCurrentCamera()->getViewport()->height(); mTexture->copyTexImage2D(*renderInfo.getState(), 0, 0, w, h); - - // Callback removes itself when done - if (renderInfo.getCurrentCamera()) - renderInfo.getCurrentCamera()->setInitialDrawCallback(nullptr); } private: @@ -308,6 +304,8 @@ namespace MWGui mGuiTexture.reset(new osgMyGUI::OSGTexture(mTexture)); } + // Notice that the next time this is called, the current CopyFramebufferToTextureCallback will be deleted + // so there's no memory leak as at most one object of type CopyFramebufferToTextureCallback is allocated at a time. mViewer->getCamera()->setInitialDrawCallback(new CopyFramebufferToTextureCallback(mTexture)); mBackgroundImage->setBackgroundImage(""); From 1bdec2399fab05dcd18b8bc8ec3423c9ab26b354 Mon Sep 17 00:00:00 2001 From: Ilya Zhuravlev Date: Tue, 25 Dec 2018 11:56:24 -0500 Subject: [PATCH 2/2] Make sure CopyFramebufferToTextureCallback is only called once and not every frame --- apps/openmw/mwgui/loadingscreen.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/apps/openmw/mwgui/loadingscreen.cpp b/apps/openmw/mwgui/loadingscreen.cpp index 68cf0439a..a9480f261 100644 --- a/apps/openmw/mwgui/loadingscreen.cpp +++ b/apps/openmw/mwgui/loadingscreen.cpp @@ -133,11 +133,15 @@ namespace MWGui public: CopyFramebufferToTextureCallback(osg::Texture2D* texture) : mTexture(texture) + , oneshot(true) { } virtual void operator () (osg::RenderInfo& renderInfo) const { + if (!oneshot) + return; + oneshot = false; int w = renderInfo.getCurrentCamera()->getViewport()->width(); int h = renderInfo.getCurrentCamera()->getViewport()->height(); mTexture->copyTexImage2D(*renderInfo.getState(), 0, 0, w, h); @@ -145,6 +149,7 @@ namespace MWGui private: osg::ref_ptr mTexture; + mutable bool oneshot; }; class DontComputeBoundCallback : public osg::Node::ComputeBoundingSphereCallback