From 9c59b7ff4824e7d37d385d4c408ce375fd2bb24a Mon Sep 17 00:00:00 2001 From: Pauli Date: Thu, 14 Jun 2018 17:12:06 +0300 Subject: [PATCH] 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. --- library/LuaApi.cpp | 6 +----- library/include/MiscUtils.h | 23 +++++++++++++++++++++++ library/include/modules/Screen.h | 7 ++++--- library/modules/Screen.cpp | 15 ++++++++------- plugins/autochop.cpp | 4 ++-- plugins/buildingplan.cpp | 2 +- plugins/command-prompt.cpp | 2 +- plugins/dwarfmonitor.cpp | 10 +++++----- plugins/embark-assistant/finder_ui.cpp | 2 +- plugins/embark-assistant/help_ui.cpp | 2 +- plugins/embark-tools.cpp | 2 +- plugins/hotkeys.cpp | 2 +- plugins/manipulator.cpp | 10 +++++----- plugins/stocks.cpp | 8 ++++---- 14 files changed, 58 insertions(+), 37 deletions(-) diff --git a/library/LuaApi.cpp b/library/LuaApi.cpp index e735eeb4b..f2e1d22ac 100644 --- a/library/LuaApi.cpp +++ b/library/LuaApi.cpp @@ -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{screen}, before); lua_pushboolean(L, ok); return 1; diff --git a/library/include/MiscUtils.h b/library/include/MiscUtils.h index 378b7a728..b2c1106a6 100644 --- a/library/include/MiscUtils.h +++ b/library/include/MiscUtils.h @@ -31,6 +31,7 @@ distribution. #include #include #include +#include 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 std::enable_if::value, std::unique_ptr >::type +make_unique(Args&&... args) +{ + return std::unique_ptr{new T{std::forward(args)...}}; +} +#else /* >= c++14 */ +using std::make_unique; +#endif +} + template void print_bits ( T val, ostream& out ) { diff --git a/library/include/modules/Screen.h b/library/include/modules/Screen.h index 673533f3e..1dcc13c06 100644 --- a/library/include/modules/Screen.h +++ b/library/include/modules/Screen.h @@ -31,6 +31,7 @@ distribution. #include #include +#include #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 screen, df::viewscreen *before = NULL, Plugin *p = NULL); + inline bool show(std::unique_ptr 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); diff --git a/library/modules/Screen.cpp b/library/modules/Screen.cpp index 6ac39aa05..0739400a7 100644 --- a/library/modules/Screen.cpp +++ b/library/modules/Screen.cpp @@ -299,7 +299,7 @@ bool Screen::findGraphicsTile(const std::string &pagename, int x, int y, int *pt static std::map plugin_screens; -bool Screen::show(df::viewscreen *screen, df::viewscreen *before, Plugin *plugin) +bool Screen::show(std::unique_ptr 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(screen)->onShow(); + if (dfhack_viewscreen::is_instance(s)) + static_cast(s)->onShow(); if (plugin) - plugin_screens[screen] = plugin; + plugin_screens[s] = plugin; return true; } diff --git a/plugins/autochop.cpp b/plugins/autochop.cpp index eccd16035..e7f3874d3 100644 --- a/plugins/autochop.cpp +++ b/plugins/autochop.cpp @@ -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(), plugin_self); } else { @@ -852,7 +852,7 @@ command_result df_autochop (color_ostream &out, vector & parameters) return CR_WRONG_USAGE; } if (Maps::IsValid()) - Screen::show(new ViewscreenAutochop(), plugin_self); + Screen::show(dts::make_unique(), plugin_self); return CR_OK; } diff --git a/plugins/buildingplan.cpp b/plugins/buildingplan.cpp index a3bb97319..c19eb02e9 100644 --- a/plugins/buildingplan.cpp +++ b/plugins/buildingplan.cpp @@ -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(planner.getDefaultItemFilterForType(type)), plugin_self); } else if (input->count(interface_key::CUSTOM_Q)) { diff --git a/plugins/command-prompt.cpp b/plugins/command-prompt.cpp index 449da0e80..0a45708d7 100644 --- a/plugins/command-prompt.cpp +++ b/plugins/command-prompt.cpp @@ -320,7 +320,7 @@ command_result show_prompt(color_ostream &out, std::vector & param std::string params; for(size_t i=0;i(params), plugin_self); return CR_OK; } bool hotkey_allow_all(df::viewscreen *top) diff --git a/plugins/dwarfmonitor.cpp b/plugins/dwarfmonitor.cpp index e786b21dd..f4a62a118 100644 --- a/plugins/dwarfmonitor.cpp +++ b/plugins/dwarfmonitor.cpp @@ -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(selected_unit), plugin_self); } else if (input->count(interface_key::CUSTOM_SHIFT_Z)) { @@ -1643,7 +1643,7 @@ public: { auto unitscr = df::allocate(); unitscr->unit = unit; - Screen::show(unitscr); + Screen::show(std::unique_ptr(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(), 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 & par else if (cmd == 's' || cmd == 'S') { if(Maps::IsValid()) - Screen::show(new ViewscreenFortStats(), plugin_self); + Screen::show(dts::make_unique(), plugin_self); } else if (cmd == 'p' || cmd == 'P') { if(Maps::IsValid()) - Screen::show(new ViewscreenPreferences(), plugin_self); + Screen::show(dts::make_unique(), plugin_self); } else if (cmd == 'r' || cmd == 'R') { diff --git a/plugins/embark-assistant/finder_ui.cpp b/plugins/embark-assistant/finder_ui.cpp index 3f972635f..fd4f8e708 100644 --- a/plugins/embark-assistant/finder_ui.cpp +++ b/plugins/embark-assistant/finder_ui.cpp @@ -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(), plugin_self); } //=============================================================================== diff --git a/plugins/embark-assistant/help_ui.cpp b/plugins/embark-assistant/help_ui.cpp index 32fb28dbb..19f0ea1b0 100644 --- a/plugins/embark-assistant/help_ui.cpp +++ b/plugins/embark-assistant/help_ui.cpp @@ -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(), plugin_self); } diff --git a/plugins/embark-tools.cpp b/plugins/embark-tools.cpp index 34613cafb..da6a2f3b1 100644 --- a/plugins/embark-tools.cpp +++ b/plugins/embark-tools.cpp @@ -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(), plugin_self); } inline bool is_valid_page() diff --git a/plugins/hotkeys.cpp b/plugins/hotkeys.cpp index d3ee600e3..2728afcf9 100644 --- a/plugins/hotkeys.cpp +++ b/plugins/hotkeys.cpp @@ -312,7 +312,7 @@ static command_result hotkeys_cmd(color_ostream &out, vector & 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(top_screen), plugin_self); } } } diff --git a/plugins/manipulator.cpp b/plugins/manipulator.cpp index 5dab2ab09..a1fd6e07f 100644 --- a/plugins/manipulator.cpp +++ b/plugins/manipulator.cpp @@ -1814,14 +1814,14 @@ void viewscreen_unitlaborsst::feed(set *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(units, true, &do_refresh_names), plugin_self); } if (events->count(interface_key::CUSTOM_E)) { vector tmp; tmp.push_back(cur); - Screen::show(new viewscreen_unitbatchopst(tmp, false, &do_refresh_names), plugin_self); + Screen::show(dts::make_unique(tmp, false, &do_refresh_names), plugin_self); } if (events->count(interface_key::CUSTOM_P)) @@ -1832,11 +1832,11 @@ void viewscreen_unitlaborsst::feed(set *events) has_selected = true; if (has_selected) { - Screen::show(new viewscreen_unitprofessionset(units, true), plugin_self); + Screen::show(dts::make_unique(units, true), plugin_self); } else { vector tmp; tmp.push_back(cur); - Screen::show(new viewscreen_unitprofessionset(tmp, false), plugin_self); + Screen::show(dts::make_unique(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(units[page], cursor_pos[page]), plugin_self); return; } } diff --git a/plugins/stocks.cpp b/plugins/stocks.cpp index a533f4669..b5831d159 100644 --- a/plugins/stocks.cpp +++ b/plugins/stocks.cpp @@ -677,7 +677,7 @@ public: } else if (input->count(interface_key::HELP)) { - Screen::show(new search_help, plugin_self); + Screen::show(dts::make_unique(), 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(), 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(sp), plugin_self); return true; } @@ -1451,7 +1451,7 @@ static command_result stocks_cmd(color_ostream &out, vector & parameter } else if (toLower(parameters[0])[0] == 's') { - Screen::show(new ViewscreenStocks(), plugin_self); + Screen::show(dts::make_unique(), plugin_self); return CR_OK; } }