commit bf27e9c6324f6ce2b32e3e19a8fe65d8d8aa9b7b
parent dfaf79ee2069c4f5c49761253f9102e8446f47f4
Author: cfillion <cfillion@users.noreply.github.com>
Date: Sat, 22 Sep 2018 20:56:36 -0400
fix use-after-free when closing modeless UI windows [p=2037784]
It was caused by a lambda attempting to access a captured value after having destroyed its parent object (thus also all captures).
Diffstat:
4 files changed, 30 insertions(+), 62 deletions(-)
diff --git a/src/dialog.cpp b/src/dialog.cpp
@@ -166,18 +166,6 @@ INT_PTR Dialog::init(REAPER_PLUGIN_HINSTANCE inst, HWND parent, Modality mode)
return false; // makes MSVC happy.
}
-void Dialog::Destroy(Dialog *dlg)
-{
- delete dlg;
-}
-
-void Dialog::DestroyAll()
-{
- const auto map = s_instances; // make an immutable copy
- for(Dialog *dlg : map | boost::adaptors::map_values)
- Destroy(dlg);
-}
-
void Dialog::setVisible(const bool visible, HWND handle)
{
if(!handle)
diff --git a/src/dialog.hpp b/src/dialog.hpp
@@ -22,6 +22,7 @@
#include <functional>
#include <map>
+#include <memory>
#include <set>
#include <vector>
@@ -52,27 +53,20 @@ public:
};
template<class T, class... Args>
- static T *Create(REAPER_PLUGIN_HINSTANCE instance, HWND parent, Args&&... args)
+ static std::unique_ptr<T> Create(REAPER_PLUGIN_HINSTANCE instance, HWND parent, Args&&... args)
{
- Dialog *dlg = new T(args...);
+ auto dlg = std::make_unique<T>(args...);
dlg->init(instance, parent, Dialog::Modeless);
- return static_cast<T *>(dlg);
+ return dlg;
}
template<class T, class... Args>
- static INT_PTR Show(REAPER_PLUGIN_HINSTANCE i, HWND parent, Args&&... args)
+ static INT_PTR Show(REAPER_PLUGIN_HINSTANCE instance, HWND parent, Args&&... args)
{
- Dialog *dlg = new T(args...);
- INT_PTR ret = dlg->init(i, parent, Dialog::Modal);
- Destroy(dlg);
-
- return ret;
+ return std::make_unique<T>(args...)->init(instance, parent, Dialog::Modal);
}
- static void Destroy(Dialog *);
- static void DestroyAll();
-
INT_PTR init(REAPER_PLUGIN_HINSTANCE, HWND, const Modality);
REAPER_PLUGIN_HINSTANCE instance() const { return m_instance; }
diff --git a/src/reapack.cpp b/src/reapack.cpp
@@ -19,7 +19,6 @@
#include "about.hpp"
#include "api.hpp"
-#include "browser.hpp"
#include "config.hpp"
#include "download.hpp"
#include "errors.hpp"
@@ -80,8 +79,7 @@ Path ReaPack::resourcePath()
ReaPack::ReaPack(REAPER_PLUGIN_HINSTANCE instance, HWND mainWindow)
: m_instance(instance), m_mainWindow(mainWindow),
- m_useRootPath(resourcePath()), m_config(Path::CONFIG.prependRoot()),
- m_tx{}, m_about{}, m_browser{}, m_manager{}, m_progress{}
+ m_useRootPath(resourcePath()), m_config(Path::CONFIG.prependRoot()), m_tx{}
{
assert(!s_instance);
s_instance = this;
@@ -104,7 +102,6 @@ ReaPack::ReaPack(REAPER_PLUGIN_HINSTANCE instance, HWND mainWindow)
ReaPack::~ReaPack()
{
- Dialog::DestroyAll();
DownloadContext::GlobalCleanup();
s_instance = nullptr;
@@ -210,11 +207,7 @@ void ReaPack::manageRemotes()
m_manager = Dialog::Create<Manager>(m_instance, m_mainWindow);
m_manager->show();
-
- m_manager->setCloseHandler([=] (INT_PTR) {
- Dialog::Destroy(m_manager);
- m_manager = nullptr;
- });
+ m_manager->setCloseHandler([=](INT_PTR) { m_manager.reset(); });
}
Remote ReaPack::remote(const string &name) const
@@ -247,35 +240,27 @@ void ReaPack::aboutSelf()
About *ReaPack::about(const bool instantiate)
{
if(m_about)
- return m_about;
+ return m_about.get();
else if(!instantiate)
return nullptr;
m_about = Dialog::Create<About>(m_instance, m_mainWindow);
+ m_about->setCloseHandler([=](INT_PTR) { m_about.reset(); });
- m_about->setCloseHandler([=] (INT_PTR) {
- Dialog::Destroy(m_about);
- m_about = nullptr;
- });
-
- return m_about;
+ return m_about.get();
}
Browser *ReaPack::browsePackages()
{
- if(m_browser) {
+ if(m_browser)
m_browser->setFocus();
- return m_browser;
+ else {
+ m_browser = Dialog::Create<Browser>(m_instance, m_mainWindow);
+ m_browser->refresh();
+ m_browser->setCloseHandler([=](INT_PTR) { m_browser.reset(); });
}
- m_browser = Dialog::Create<Browser>(m_instance, m_mainWindow);
- m_browser->refresh();
- m_browser->setCloseHandler([=] (INT_PTR) {
- Dialog::Destroy(m_browser);
- m_browser = nullptr;
- });
-
- return m_browser;
+ return m_browser.get();
}
Transaction *ReaPack::setupTransaction()
@@ -304,22 +289,21 @@ Transaction *ReaPack::setupTransaction()
m_progress = Dialog::Create<Progress>(m_instance, m_mainWindow, m_tx->threadPool());
m_tx->onFinish([=] {
- Dialog::Destroy(m_progress);
- m_progress = nullptr;
+ m_progress.reset();
if(!m_tx->isCancelled() && !m_tx->receipt()->empty()) {
- LockDialog managerLock(m_manager);
- LockDialog browserLock(m_browser);
+ LockDialog managerLock(m_manager.get());
+ LockDialog browserLock(m_browser.get());
Dialog::Show<Report>(m_instance, m_mainWindow, m_tx->receipt());
}
});
m_tx->setObsoleteHandler([=] (vector<Registry::Entry> &entries) {
- LockDialog aboutLock(m_about);
- LockDialog browserLock(m_browser);
- LockDialog managerLock(m_manager);
- LockDialog progressLock(m_progress);
+ LockDialog aboutLock(m_about.get());
+ LockDialog browserLock(m_browser.get());
+ LockDialog managerLock(m_manager.get());
+ LockDialog progressLock(m_progress.get());
return Dialog::Show<ObsoleteQuery>(m_instance, m_mainWindow,
&entries, &config()->install.promptObsolete) == IDOK;
diff --git a/src/reapack.hpp b/src/reapack.hpp
@@ -20,6 +20,8 @@
#include "action.hpp"
#include "api.hpp"
+#include "browser.hpp"
+#include "browser_entry.hpp"
#include "config.hpp"
#include "path.hpp"
@@ -86,10 +88,10 @@ private:
std::list<APIDef> m_api;
Transaction *m_tx;
- About *m_about;
- Browser *m_browser;
- Manager *m_manager;
- Progress *m_progress;
+ std::unique_ptr<About> m_about;
+ std::unique_ptr<Browser> m_browser;
+ std::unique_ptr<Manager> m_manager;
+ std::unique_ptr<Progress> m_progress;
};
#endif
