Fix unlikely memory leaks if Screen::show fails

The Screen::show takes ownership of the screen pointer. I decided to
switch the parameter to std::unique_ptr to make the pointer ownership
explicit. The unique_ptr then provides automatic screen destruction in
Screen::show unless pointer is inserted or is already in the linked list
that is managed by df.
develop
Pauli 2018-06-14 17:12:06 +03:00
parent 2c106fa7a5
commit 9c59b7ff48
14 changed files with 58 additions and 37 deletions

@ -2242,11 +2242,7 @@ int screen_show(lua_State *L)
df::viewscreen *screen = dfhack_lua_viewscreen::get_pointer(L, 1, true);
bool ok = Screen::show(screen, before);
// If it is a table, get_pointer created a new object. Don't leak it.
if (!ok && lua_istable(L, 1))
delete screen;
bool ok = Screen::show(std::unique_ptr<df::viewscreen>{screen}, before);
lua_pushboolean(L, ok);
return 1;

@ -31,6 +31,7 @@ distribution.
#include <vector>
#include <sstream>
#include <cstdio>
#include <memory>
using std::ostream;
using std::stringstream;
@ -44,6 +45,28 @@ using std::endl;
#define DFHACK_FUNCTION_SIG __func__
#endif
/*! \namespace dts
* std.reverse() == dts, The namespace that include forward compatible helpers
* which can be used from newer standards. The preprocessor check prefers
* standard version if one is available. The standard version gets imported with
* using.
*/
namespace dts {
// Check if lib supports the feature test macro or version is over c++14.
#if __cpp_lib_make_unique < 201304 && __cplusplus < 201402L
//! Insert c++14 make_unique to be forward compatible. Array versions are
//! missing
template<typename T, typename... Args>
typename std::enable_if<!std::is_array<T>::value, std::unique_ptr<T> >::type
make_unique(Args&&... args)
{
return std::unique_ptr<T>{new T{std::forward<Args>(args)...}};
}
#else /* >= c++14 */
using std::make_unique;
#endif
}
template <typename T>
void print_bits ( T val, ostream& out )
{

@ -31,6 +31,7 @@ distribution.
#include <string>
#include <set>
#include <memory>
#include "DataDefs.h"
#include "df/graphic.h"
@ -210,9 +211,9 @@ namespace DFHack
DFHACK_EXPORT bool findGraphicsTile(const std::string &page, int x, int y, int *ptile, int *pgs = NULL);
// Push and remove viewscreens
DFHACK_EXPORT bool show(df::viewscreen *screen, df::viewscreen *before = NULL, Plugin *p = NULL);
inline bool show(df::viewscreen *screen, Plugin *p)
{ return show(screen, NULL, p); }
DFHACK_EXPORT bool show(std::unique_ptr<df::viewscreen> screen, df::viewscreen *before = NULL, Plugin *p = NULL);
inline bool show(std::unique_ptr<df::viewscreen> screen, Plugin *p)
{ return show(std::move(screen), NULL, p); }
DFHACK_EXPORT void dismiss(df::viewscreen *screen, bool to_first = false);
DFHACK_EXPORT bool isDismissed(df::viewscreen *screen);
DFHACK_EXPORT bool hasActiveScreens(Plugin *p);

@ -299,7 +299,7 @@ bool Screen::findGraphicsTile(const std::string &pagename, int x, int y, int *pt
static std::map<df::viewscreen*, Plugin*> plugin_screens;
bool Screen::show(df::viewscreen *screen, df::viewscreen *before, Plugin *plugin)
bool Screen::show(std::unique_ptr<df::viewscreen> screen, df::viewscreen *before, Plugin *plugin)
{
CHECK_NULL_POINTER(screen);
CHECK_INVALID_ARGUMENT(!screen->parent && !screen->child);
@ -316,15 +316,16 @@ bool Screen::show(df::viewscreen *screen, df::viewscreen *before, Plugin *plugin
screen->child = parent->child;
screen->parent = parent;
parent->child = screen;
if (screen->child)
screen->child->parent = screen;
df::viewscreen* s = screen.release();
parent->child = s;
if (s->child)
s->child->parent = s;
if (dfhack_viewscreen::is_instance(screen))
static_cast<dfhack_viewscreen*>(screen)->onShow();
if (dfhack_viewscreen::is_instance(s))
static_cast<dfhack_viewscreen*>(s)->onShow();
if (plugin)
plugin_screens[screen] = plugin;
plugin_screens[s] = plugin;
return true;
}

@ -809,7 +809,7 @@ struct autochop_hook : public df::viewscreen_dwarfmodest
if (isInDesignationMenu() && input->count(interface_key::CUSTOM_C))
{
sendKey(interface_key::LEAVESCREEN);
Screen::show(new ViewscreenAutochop(), plugin_self);
Screen::show(dts::make_unique<ViewscreenAutochop>(), plugin_self);
}
else
{
@ -852,7 +852,7 @@ command_result df_autochop (color_ostream &out, vector <string> & parameters)
return CR_WRONG_USAGE;
}
if (Maps::IsValid())
Screen::show(new ViewscreenAutochop(), plugin_self);
Screen::show(dts::make_unique<ViewscreenAutochop>(), plugin_self);
return CR_OK;
}

@ -156,7 +156,7 @@ struct buildingplan_hook : public df::viewscreen_dwarfmodest
}
else if (input->count(interface_key::CUSTOM_SHIFT_M))
{
Screen::show(new ViewscreenChooseMaterial(planner.getDefaultItemFilterForType(type)), plugin_self);
Screen::show(dts::make_unique<ViewscreenChooseMaterial>(planner.getDefaultItemFilterForType(type)), plugin_self);
}
else if (input->count(interface_key::CUSTOM_Q))
{

@ -320,7 +320,7 @@ command_result show_prompt(color_ostream &out, std::vector <std::string> & param
std::string params;
for(size_t i=0;i<parameters.size();i++)
params+=parameters[i]+" ";
Screen::show(new viewscreen_commandpromptst(params), plugin_self);
Screen::show(dts::make_unique<viewscreen_commandpromptst>(params), plugin_self);
return CR_OK;
}
bool hotkey_allow_all(df::viewscreen *top)

@ -1062,7 +1062,7 @@ public:
{
df::unit *selected_unit = (selected_column == 1) ? dwarf_activity_column.getFirstSelectedElem() : nullptr;
Screen::dismiss(this);
Screen::show(new ViewscreenDwarfStats(selected_unit), plugin_self);
Screen::show(dts::make_unique<ViewscreenDwarfStats>(selected_unit), plugin_self);
}
else if (input->count(interface_key::CUSTOM_SHIFT_Z))
{
@ -1643,7 +1643,7 @@ public:
{
auto unitscr = df::allocate<df::viewscreen_unitst>();
unitscr->unit = unit;
Screen::show(unitscr);
Screen::show(std::unique_ptr<df::viewscreen>(unitscr));
}
}
else if (input->count(interface_key::CUSTOM_SHIFT_Z))
@ -1737,7 +1737,7 @@ private:
static void open_stats_screen()
{
Screen::show(new ViewscreenFortStats(), plugin_self);
Screen::show(dts::make_unique<ViewscreenFortStats>(), plugin_self);
}
static void add_work_history(df::unit *unit, activity_type type)
@ -1977,12 +1977,12 @@ static command_result dwarfmonitor_cmd(color_ostream &out, vector <string> & par
else if (cmd == 's' || cmd == 'S')
{
if(Maps::IsValid())
Screen::show(new ViewscreenFortStats(), plugin_self);
Screen::show(dts::make_unique<ViewscreenFortStats>(), plugin_self);
}
else if (cmd == 'p' || cmd == 'P')
{
if(Maps::IsValid())
Screen::show(new ViewscreenPreferences(), plugin_self);
Screen::show(dts::make_unique<ViewscreenPreferences>(), plugin_self);
}
else if (cmd == 'r' || cmd == 'R')
{

@ -1380,7 +1380,7 @@ void embark_assist::finder_ui::init(DFHack::Plugin *plugin_self, embark_assist::
if (!embark_assist::finder_ui::state) { // First call. Have to do the setup
embark_assist::finder_ui::ui_setup(find_callback, max_inorganic);
}
Screen::show(new ViewscreenFindUi(), plugin_self);
Screen::show(dts::make_unique<ViewscreenFindUi>(), plugin_self);
}
//===============================================================================

@ -322,5 +322,5 @@ namespace embark_assist{
//===============================================================================
void embark_assist::help_ui::init(DFHack::Plugin *plugin_self) {
Screen::show(new embark_assist::help_ui::ViewscreenHelpUi(), plugin_self);
Screen::show(dts::make_unique<embark_assist::help_ui::ViewscreenHelpUi>(), plugin_self);
}

@ -696,7 +696,7 @@ struct choose_start_site_hook : df::viewscreen_choose_start_sitest
void display_settings()
{
Screen::show(new embark_tools_settings, plugin_self);
Screen::show(dts::make_unique<embark_tools_settings>(), plugin_self);
}
inline bool is_valid_page()

@ -312,7 +312,7 @@ static command_result hotkeys_cmd(color_ostream &out, vector <string> & paramete
if (Gui::getFocusString(top_screen) != "dfhack/viewscreen_hotkeys")
{
find_active_keybindings(top_screen);
Screen::show(new ViewscreenHotkeys(top_screen), plugin_self);
Screen::show(dts::make_unique<ViewscreenHotkeys>(top_screen), plugin_self);
}
}
}

@ -1814,14 +1814,14 @@ void viewscreen_unitlaborsst::feed(set<df::interface_key> *events)
if (events->count(interface_key::CUSTOM_B))
{
Screen::show(new viewscreen_unitbatchopst(units, true, &do_refresh_names), plugin_self);
Screen::show(dts::make_unique<viewscreen_unitbatchopst>(units, true, &do_refresh_names), plugin_self);
}
if (events->count(interface_key::CUSTOM_E))
{
vector<UnitInfo*> tmp;
tmp.push_back(cur);
Screen::show(new viewscreen_unitbatchopst(tmp, false, &do_refresh_names), plugin_self);
Screen::show(dts::make_unique<viewscreen_unitbatchopst>(tmp, false, &do_refresh_names), plugin_self);
}
if (events->count(interface_key::CUSTOM_P))
@ -1832,11 +1832,11 @@ void viewscreen_unitlaborsst::feed(set<df::interface_key> *events)
has_selected = true;
if (has_selected) {
Screen::show(new viewscreen_unitprofessionset(units, true), plugin_self);
Screen::show(dts::make_unique<viewscreen_unitprofessionset>(units, true), plugin_self);
} else {
vector<UnitInfo*> tmp;
tmp.push_back(cur);
Screen::show(new viewscreen_unitprofessionset(tmp, false), plugin_self);
Screen::show(dts::make_unique<viewscreen_unitprofessionset>(tmp, false), plugin_self);
}
}
@ -2187,7 +2187,7 @@ struct unitlist_hook : df::viewscreen_unitlistst
{
if (units[page].size())
{
Screen::show(new viewscreen_unitlaborsst(units[page], cursor_pos[page]), plugin_self);
Screen::show(dts::make_unique<viewscreen_unitlaborsst>(units[page], cursor_pos[page]), plugin_self);
return;
}
}

@ -677,7 +677,7 @@ public:
}
else if (input->count(interface_key::HELP))
{
Screen::show(new search_help, plugin_self);
Screen::show(dts::make_unique<search_help>(), plugin_self);
}
bool key_processed = false;
@ -1342,7 +1342,7 @@ struct stocks_hook : public df::viewscreen_storesst
if (input->count(interface_key::CUSTOM_E))
{
Screen::dismiss(this);
Screen::show(new ViewscreenStocks(), plugin_self);
Screen::show(dts::make_unique<ViewscreenStocks>(), plugin_self);
return;
}
INTERPOSE_NEXT(feed)(input);
@ -1377,7 +1377,7 @@ struct stocks_stockpile_hook : public df::viewscreen_dwarfmodest
if (input->count(interface_key::CUSTOM_I))
{
Screen::show(new ViewscreenStocks(sp), plugin_self);
Screen::show(dts::make_unique<ViewscreenStocks>(sp), plugin_self);
return true;
}
@ -1451,7 +1451,7 @@ static command_result stocks_cmd(color_ostream &out, vector <string> & parameter
}
else if (toLower(parameters[0])[0] == 's')
{
Screen::show(new ViewscreenStocks(), plugin_self);
Screen::show(dts::make_unique<ViewscreenStocks>(), plugin_self);
return CR_OK;
}
}