From 6495bd470d40b2b489e6fe350e2da08be64d4bec Mon Sep 17 00:00:00 2001 From: Andreas Nordal Date: Fri, 18 Mar 2016 23:14:16 +0100 Subject: [PATCH] Fix memory leaks at exit found in tests This fixes all memory leaks found by compiling with clang++ -g -fsanitize=address and running the tests. Method: Ensure that memory is freed by the destructor of its respective container, either by storing objects directly instead of by pointer, or implementing the required destructor. --- src/complete.cpp | 92 ++++++++++++++++------------------------------ src/fish_tests.cpp | 4 +- src/history.cpp | 54 ++++++++++++++++++++++----- src/history.h | 13 +++---- src/wutil.cpp | 16 ++++---- 5 files changed, 95 insertions(+), 84 deletions(-) diff --git a/src/complete.cpp b/src/complete.cpp index 5e77af256..8af2c822f 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -186,7 +186,6 @@ public: /** Adds or removes an option. */ void add_option(const complete_entry_opt_t &opt); bool remove_option(const wcstring &option, complete_option_type_t type); - void remove_all_options(); completion_entry_t(const wcstring &c, bool type, bool author) : cmd(c), @@ -201,20 +200,20 @@ public: struct completion_entry_set_comparer { /** Comparison for std::set */ - bool operator()(completion_entry_t *p1, completion_entry_t *p2) const + bool operator()(const completion_entry_t &p1, const completion_entry_t &p2) const { /* Paths always come last for no particular reason */ - if (p1->cmd_is_path != p2->cmd_is_path) + if (p1.cmd_is_path != p2.cmd_is_path) { - return p1->cmd_is_path < p2->cmd_is_path; + return p1.cmd_is_path < p2.cmd_is_path; } else { - return p1->cmd < p2->cmd; + return p1.cmd < p2.cmd; } } }; -typedef std::set completion_entry_set_t; +typedef std::set completion_entry_set_t; static completion_entry_set_t completion_set; // Comparison function to sort completions by their order field @@ -520,46 +519,28 @@ bool completer_t::condition_test(const wcstring &condition) } -/** Search for an exactly matching completion entry. Must be called while locked. */ -static completion_entry_t *complete_find_exact_entry(const wcstring &cmd, const bool cmd_is_path) -{ - ASSERT_IS_LOCKED(completion_lock); - completion_entry_t *result = NULL; - completion_entry_t tmp_entry(cmd, cmd_is_path, false); - completion_entry_set_t::iterator iter = completion_set.find(&tmp_entry); - if (iter != completion_set.end()) - { - result = *iter; - } - return result; -} - /** Locate the specified entry. Create it if it doesn't exist. Must be called while locked. */ -static completion_entry_t *complete_get_exact_entry(const wcstring &cmd, bool cmd_is_path) +static completion_entry_t &complete_get_exact_entry(const wcstring &cmd, bool cmd_is_path) { ASSERT_IS_LOCKED(completion_lock); - completion_entry_t *c; - c = complete_find_exact_entry(cmd, cmd_is_path); + std::pair ins = + completion_set.insert(completion_entry_t(cmd, cmd_is_path, false)); - if (c == NULL) - { - c = new completion_entry_t(cmd, cmd_is_path, false); - completion_set.insert(c); - } - - return c; + // NOTE SET_ELEMENTS_ARE_IMMUTABLE: Exposing mutable access here is only + // okay as long as callers do not change any field that matters to ordering + // - affecting order without telling std::set invalidates its internal state + return const_cast(*ins.first); } void complete_set_authoritative(const wchar_t *cmd, bool cmd_is_path, bool authoritative) { - completion_entry_t *c; - CHECK(cmd,); scoped_lock lock(completion_lock); - c = complete_get_exact_entry(cmd, cmd_is_path); - c->authoritative = authoritative; + + completion_entry_t &c = complete_get_exact_entry(cmd, cmd_is_path); + c.authoritative = authoritative; } @@ -580,8 +561,7 @@ void complete_add(const wchar_t *cmd, /* Lock the lock that allows us to edit the completion entry list */ scoped_lock lock(completion_lock); - completion_entry_t *c; - c = complete_get_exact_entry(cmd, cmd_is_path); + completion_entry_t &c = complete_get_exact_entry(cmd, cmd_is_path); /* Create our new option */ complete_entry_opt_t opt; @@ -594,7 +574,7 @@ void complete_add(const wchar_t *cmd, if (desc) opt.desc = desc; opt.flags = flags; - c->add_option(opt); + c.add_option(opt); } /** @@ -621,28 +601,23 @@ bool completion_entry_t::remove_option(const wcstring &option, complete_option_t return this->options.empty(); } -void completion_entry_t::remove_all_options() -{ - ASSERT_IS_LOCKED(completion_lock); - this->options.clear(); -} - void complete_remove(const wcstring &cmd, bool cmd_is_path, const wcstring &option, complete_option_type_t type) { scoped_lock lock(completion_lock); completion_entry_t tmp_entry(cmd, cmd_is_path, false); - completion_entry_set_t::iterator iter = completion_set.find(&tmp_entry); + completion_entry_set_t::iterator iter = completion_set.find(tmp_entry); if (iter != completion_set.end()) { - completion_entry_t *entry = *iter; - bool delete_it = entry->remove_option(option, type); + // const_cast: See SET_ELEMENTS_ARE_IMMUTABLE + completion_entry_t &entry = const_cast(*iter); + + bool delete_it = entry.remove_option(option, type); if (delete_it) { /* Delete this entry */ completion_set.erase(iter); - delete entry; } } } @@ -652,14 +627,7 @@ void complete_remove_all(const wcstring &cmd, bool cmd_is_path) scoped_lock lock(completion_lock); completion_entry_t tmp_entry(cmd, cmd_is_path, false); - completion_entry_set_t::iterator iter = completion_set.find(&tmp_entry); - if (iter != completion_set.end()) - { - completion_entry_t *entry = *iter; - entry->remove_all_options(); - completion_set.erase(iter); - delete entry; - } + completion_set.erase(tmp_entry); } @@ -1187,12 +1155,12 @@ bool completer_t::complete_param(const wcstring &scmd_orig, const wcstring &spop scoped_lock lock(completion_lock); for (completion_entry_set_t::const_iterator iter = completion_set.begin(); iter != completion_set.end(); ++iter) { - const completion_entry_t *i = *iter; - const wcstring &match = i->cmd_is_path ? path : cmd; - if (wildcard_match(match, i->cmd)) + const completion_entry_t &i = *iter; + const wcstring &match = i.cmd_is_path ? path : cmd; + if (wildcard_match(match, i.cmd)) { /* Copy all of their options into our list */ - all_options.push_back(i->get_options()); //Oof, this is a lot of copying + all_options.push_back(i.get_options()); //Oof, this is a lot of copying } } } @@ -1909,7 +1877,11 @@ wcstring complete_print() scoped_lock locker(completion_lock); // Get a list of all completions in a vector, then sort it by order - std::vector all_completions(completion_set.begin(), completion_set.end()); + std::vector all_completions; + for (completion_entry_set_t::const_iterator i = completion_set.begin(); i != completion_set.end(); ++i) + { + all_completions.push_back(&*i); + } sort(all_completions.begin(), all_completions.end(), compare_completions_by_order); for (std::vector::const_iterator iter = all_completions.begin(); iter != all_completions.end(); ++iter) diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index dfc346092..5bacab80e 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2645,9 +2645,11 @@ static void test_universal() if (system("mkdir -p /tmp/fish_uvars_test/")) err(L"mkdir failed"); const int threads = 16; + static int ctx[threads]; for (int i=0; i < threads; i++) { - iothread_perform(test_universal_helper, new int(i)); + ctx[i] = i; + iothread_perform(test_universal_helper, &ctx[i]); } iothread_drain_all(); diff --git a/src/history.cpp b/src/history.cpp index a37d2b776..ff99db9a0 100644 --- a/src/history.cpp +++ b/src/history.cpp @@ -56,6 +56,9 @@ Our history format is intended to be valid YAML. Here it is: /** Default buffer size for flushing to the history file */ #define HISTORY_OUTPUT_BUFFER_SIZE (4096 * 4) +namespace +{ + /* Helper class for certain output. This is basically a string that allows us to ensure we only flush at record boundaries, and avoids the copying of ostringstream. Have you ever tried to implement your own streambuf? Total insanity. */ class history_output_buffer_t { @@ -168,7 +171,7 @@ public: explicit history_lru_node_t(const history_item_t &item) : lru_node_t(item.str()), timestamp(item.timestamp()), - required_paths(item.required_paths) + required_paths(item.get_required_paths()) {} }; @@ -207,9 +210,31 @@ public: } }; -static pthread_mutex_t hist_lock = PTHREAD_MUTEX_INITIALIZER; +class history_collection_t +{ + pthread_mutex_t m_lock; + std::map m_histories; -static std::map histories; +public: + history_collection_t() + { + VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_init(&m_lock, NULL)); + } + ~history_collection_t() + { + for (std::map::const_iterator i = m_histories.begin(); i != m_histories.end(); ++i) + { + delete i->second; + } + pthread_mutex_destroy(&m_lock); + } + history_t& alloc(const wcstring &name); + void save(); +}; + +} //anonymous namespace + +static history_collection_t histories; static wcstring history_filename(const wcstring &name, const wcstring &suffix); @@ -524,16 +549,21 @@ static size_t offset_of_next_item(const char *begin, size_t mmap_length, history return result; } -history_t & history_t::history_with_name(const wcstring &name) +history_t & history_collection_t::alloc(const wcstring &name) { /* Note that histories are currently never deleted, so we can return a reference to them without using something like shared_ptr */ - scoped_lock locker(hist_lock); - history_t *& current = histories[name]; + scoped_lock locker(m_lock); + history_t *& current = m_histories[name]; if (current == NULL) current = new history_t(name); return *current; } +history_t & history_t::history_with_name(const wcstring &name) +{ + return histories.alloc(name); +} + history_t::history_t(const wcstring &pname) : name(pname), first_unwritten_new_item_index(0), @@ -1809,15 +1839,21 @@ void history_init() } -void history_destroy() +void history_collection_t::save() { /* Save all histories */ - for (std::map::iterator iter = histories.begin(); iter != histories.end(); ++iter) + for (std::map::iterator iter = m_histories.begin(); iter != m_histories.end(); ++iter) { - iter->second->save(); + history_t *hist = iter->second; + hist->save(); } } +void history_destroy() +{ + histories.save(); +} + void history_sanity_check() { diff --git a/src/history.h b/src/history.h index f99950aa0..58291f22a 100644 --- a/src/history.h +++ b/src/history.h @@ -43,7 +43,6 @@ typedef uint32_t history_identifier_t; class history_item_t { friend class history_t; - friend class history_lru_node_t; friend class history_tests_t; private: @@ -115,15 +114,9 @@ private: history_t(const history_t&); history_t &operator=(const history_t&); - /** Private creator */ - explicit history_t(const wcstring &pname); - /** Privately add an item. If pending, the item will not be returned by history searches until a call to resolve_pending. */ void add(const history_item_t &item, bool pending = false); - /** Destructor */ - ~history_t(); - /** Lock for thread safety */ pthread_mutex_t lock; @@ -208,6 +201,12 @@ private: static history_item_t decode_item(const char *base, size_t len, history_file_type_t type); public: + /** Constructor */ + explicit history_t(const wcstring &); + + /** Destructor */ + ~history_t(); + /** Returns history with the given name, creating it if necessary */ static history_t & history_with_name(const wcstring &name); diff --git a/src/wutil.cpp b/src/wutil.cpp index 2ff358bc8..62602c2cf 100644 --- a/src/wutil.cpp +++ b/src/wutil.cpp @@ -44,8 +44,8 @@ const file_id_t kInvalidFileID = {(dev_t)-1LL, (ino_t)-1LL, (uint64_t)-1LL, -1, /* Lock to protect wgettext */ static pthread_mutex_t wgettext_lock; -/* Maps string keys to (immortal) pointers to string values. */ -typedef std::map wgettext_map_t; +/* Map used as cache by wgettext. */ +typedef std::map wgettext_map_t; static wgettext_map_t wgettext_map; bool wreaddir_resolving(DIR *dir, const std::wstring &dir_path, std::wstring &out_name, bool *out_is_dir) @@ -488,16 +488,18 @@ const wchar_t *wgettext(const wchar_t *in) wcstring key = in; scoped_lock lock(wgettext_lock); - // Reference to pointer to string - const wchar_t *& val = wgettext_map[key]; - if (val == NULL) + wcstring &val = wgettext_map[key]; + if (val.empty()) { cstring mbs_in = wcs2string(key); char *out = fish_gettext(mbs_in.c_str()); - val = wcsdup(format_string(L"%s", out).c_str()); //note that this writes into the map! + val = format_string(L"%s", out).c_str(); } errno = err; - return val; //looks dangerous but is safe, since the string is stored in the map + + // The returned string is stored in the map + // TODO: If we want to shrink the map, this would be a problem + return val.c_str(); } int wmkdir(const wcstring &name, int mode)