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

@@ -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_t *, completion_entry_set_comparer> completion_entry_set_t;
+typedef std::set<completion_entry_t, completion_entry_set_comparer> 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<completion_entry_set_t::iterator, bool> 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<completion_entry_t&>(*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<completion_entry_t&>(*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<const completion_entry_t *> all_completions(completion_set.begin(), completion_set.end());
+    std::vector<const completion_entry_t *> 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 completion_entry_t *>::const_iterator iter = all_completions.begin(); iter != all_completions.end(); ++iter)

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();
 

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<wcstring, history_t *> m_histories;
 
-static std::map<wcstring, history_t *> histories;
+public:
+    history_collection_t()
+    {
+        VOMIT_ON_FAILURE_NO_ERRNO(pthread_mutex_init(&m_lock, NULL));
+    }
+    ~history_collection_t()
+    {
+        for (std::map<wcstring, history_t*>::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<wcstring, history_t *>::iterator iter = histories.begin(); iter != histories.end(); ++iter)
+    for (std::map<wcstring, history_t *>::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()
 {

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);
 

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<wcstring, const wchar_t *> wgettext_map_t;
+/* Map used as cache by wgettext. */
+typedef std::map<wcstring, wcstring> 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)