Switch to using unique_ptr in env_node_t

Makes our memory management of the variable stack more explicit

src/env.cpp

@@ -75,7 +75,6 @@ static const wchar_t *const curses_variable[] = {L"TERM", L"TERMINFO", L"TERMINF
 class env_node_t {
     friend struct var_stack_t;
     env_node_t(bool is_new_scope) : new_scope(is_new_scope) {}
-    ~env_node_t() {}
 
 public:
 
@@ -88,7 +87,7 @@ public:
     /// Does this node contain any variables which are exported to subshells.
     bool exportv = false;
     /// Pointer to next level.
-    env_node_t *next = NULL;
+    std::unique_ptr<env_node_t> next;
 
     /// Returns a pointer to the given entry if present, or NULL.
     const var_entry_t *find_entry(const wcstring &key);
@@ -100,7 +99,7 @@ class variable_entry_t {
 
 static pthread_mutex_t env_lock = PTHREAD_MUTEX_INITIALIZER;
 
-static int local_scope_exports(env_node_t *n);
+static bool local_scope_exports(const env_node_t *n);
 static void handle_locale(const wchar_t *env_var_name);
 
 // A class wrapping up a variable stack
@@ -109,9 +108,10 @@ static void handle_locale(const wchar_t *env_var_name);
 // if we introduce multiple threads of execution
 struct var_stack_t {
     // Top node on the function stack.
-    env_node_t *top = NULL;
+    std::unique_ptr<env_node_t> top = NULL;
 
-    // Bottom node on the function stack.
+    // Bottom node on the function stack
+    // This is an observer pointer
     env_node_t *global_env = NULL;
 
     // Exported variable array used by execv.
@@ -122,17 +122,8 @@ struct var_stack_t {
     void mark_changed_exported() { has_changed_exported = true; }
     void update_export_array_if_necessary();
 
-    var_stack_t() {
+    var_stack_t() : top(new env_node_t(false)) {
-        this->top = new env_node_t(false);
+        this->global_env = this->top.get();
-        this->global_env = this->top;
-    }
-
-    ~var_stack_t() {
-        while (this->top) {
-            env_node_t *next = this->top->next;
-            delete this->top;
-            this->top = next;
-        }
     }
 
     // Pushes a new node onto our stack
@@ -154,50 +145,55 @@ struct var_stack_t {
 };
 
 void var_stack_t::push(bool new_scope) {
-    env_node_t *node = new env_node_t(new_scope);
+    std::unique_ptr<env_node_t> node(new env_node_t(new_scope));
-    node->next = this->top;
+    node->next = std::move(this->top);
-    this->top = node;
+    this->top = std::move(node);
-    if (new_scope && local_scope_exports(this->top)) {
+    if (new_scope && local_scope_exports(this->top.get())) {
         this->mark_changed_exported();
     }
 }
 
 void var_stack_t::pop() {
     // Don't pop the global
-    if (this->top == this->global_env) {
+    if (this->top.get() == this->global_env) {
         debug(0, _(L"Tried to pop empty environment stack."));
         sanity_lose();
         return;
     }
 
     const wchar_t *locale_changed = NULL;
-    env_node_t *killme = this->top;
 
     for (int i = 0; locale_variable[i]; i++) {
-        var_table_t::iterator result = killme->env.find(locale_variable[i]);
+        var_table_t::iterator result = top->env.find(locale_variable[i]);
-        if (result != killme->env.end()) {
+        if (result != top->env.end()) {
             locale_changed = locale_variable[i];
             break;
         }
     }
 
-    if (killme->new_scope) {  //!OCLINT(collapsible if statements)
+    if (top->new_scope) {  //!OCLINT(collapsible if statements)
-        if (killme->exportv || local_scope_exports(killme->next)) {
+        if (top->exportv || local_scope_exports(top->next.get())) {
             this->mark_changed_exported();
         }
     }
-    this->top = this->top->next;
+
+    // Actually do the pop!
+    // Move the top pointer into a local variable, then replace the top pointer with the next pointer
+    // afterwards we should have a node with no next pointer, and our top should be non-null
+    std::unique_ptr<env_node_t> old_top = std::move(this->top);
+    this->top = std::move(old_top->next);
+    old_top->next.reset();
+    assert(this->top && old_top && !old_top->next);
     assert(this->top != NULL);
 
     var_table_t::iterator iter;
-    for (iter = killme->env.begin(); iter != killme->env.end(); ++iter) {
+    for (const auto &entry_pair : old_top->env) {
-        const var_entry_t &entry = iter->second;
+        const var_entry_t &entry = entry_pair.second;
         if (entry.exportv) {
             this->mark_changed_exported();
             break;
         }
     }
-    delete killme;
     // TODO: move this to something general
     if (locale_changed) handle_locale(locale_changed);
 }
@@ -207,7 +203,7 @@ const env_node_t *var_stack_t::next_scope_to_search(const env_node_t *node) cons
     if (node == this->global_env) {
         return NULL;
     }
-    return node->new_scope ? this->global_env : node->next;
+    return node->new_scope ? this->global_env : node->next.get();
 }
 
 env_node_t *var_stack_t::next_scope_to_search(env_node_t *node) {
@@ -215,13 +211,13 @@ env_node_t *var_stack_t::next_scope_to_search(env_node_t *node) {
     if (node == this->global_env) {
         return NULL;
     }
-    return node->new_scope ? this->global_env : node->next;
+    return node->new_scope ? this->global_env : node->next.get();
 }
 
 env_node_t *var_stack_t::resolve_unspecified_scope() {
-    env_node_t *node = this->top;
+    env_node_t *node = this->top.get();
     while (node && !node->new_scope) {
-        node = node->next;
+        node = node->next.get();
     }
     return node ? node : this->global_env;
 }
@@ -637,7 +633,7 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
 /// Search all visible scopes in order for the specified key. Return the first scope in which it was
 /// found.
 static env_node_t *env_get_node(const wcstring &key) {
-    env_node_t *env = vars_stack().top;
+    env_node_t *env = vars_stack().top.get();
     while (env != NULL) {
         if (env->find_entry(key) != NULL) {
             break;
@@ -750,7 +746,7 @@ int env_set(const wcstring &key, const wchar_t *val, env_mode_flags_t var_mode)
         if (var_mode & ENV_GLOBAL) {
             node = vars_stack().global_env;
         } else if (var_mode & ENV_LOCAL) {
-            node = vars_stack().top;
+            node = vars_stack().top.get();
         } else if (preexisting_node != NULL) {
             node = preexisting_node;
             if ((var_mode & (ENV_EXPORT | ENV_UNEXPORT)) == 0) {
@@ -845,7 +841,7 @@ static bool try_remove(env_node_t *n, const wchar_t *key, int var_mode) {
     if (n->new_scope) {
         return try_remove(vars_stack().global_env, key, var_mode);
     }
-    return try_remove(n->next, key, var_mode);
+    return try_remove(n->next.get(), key, var_mode);
 }
 
 int env_remove(const wcstring &key, int var_mode) {
@@ -857,7 +853,7 @@ int env_remove(const wcstring &key, int var_mode) {
         return 2;
     }
 
-    first_node = vars_stack().top;
+    first_node = vars_stack().top.get();
 
     if (!(var_mode & ENV_UNIVERSAL)) {
         if (var_mode & ENV_GLOBAL) {
@@ -940,7 +936,7 @@ env_var_t env_get_string(const wcstring &key, env_mode_flags_t mode) {
         /* Lock around a local region */
         scoped_lock locker(env_lock);
 
-        env_node_t *env = search_local ? vars_stack().top : vars_stack().global_env;
+        env_node_t *env = search_local ? vars_stack().top.get() : vars_stack().global_env;
 
         while (env != NULL) {
             const var_entry_t *entry = env->find_entry(key);
@@ -1001,7 +997,7 @@ bool env_exist(const wchar_t *key, env_mode_flags_t mode) {
     }
 
     if (test_local || test_global) {
-        const env_node_t *env = test_local ? vars_stack().top : vars_stack().global_env;
+        const env_node_t *env = test_local ? vars_stack().top.get() : vars_stack().global_env;
         while (env != NULL) {
             if (env == vars_stack().global_env && !test_global) {
                 break;
@@ -1032,14 +1028,15 @@ bool env_exist(const wchar_t *key, env_mode_flags_t mode) {
 
 /// Returns true if the specified scope or any non-shadowed non-global subscopes contain an exported
 /// variable.
-static int local_scope_exports(env_node_t *n) {
+static bool local_scope_exports(const env_node_t *n) {
+    assert(n != NULL);
     if (n == vars_stack().global_env) return 0;
 
     if (n->exportv) return 1;
 
     if (n->new_scope) return 0;
 
-    return local_scope_exports(n->next);
+    return local_scope_exports(n->next.get());
 }
 
 void env_push(bool new_scope) {
@@ -1073,7 +1070,7 @@ wcstring_list_t env_get_names(int flags) {
     int show_global = flags & ENV_GLOBAL;
     int show_universal = flags & ENV_UNIVERSAL;
 
-    env_node_t *n = vars_stack().top;
+    const env_node_t *n = vars_stack().top.get();
     const bool show_exported = (flags & ENV_EXPORT) || !(flags & ENV_UNEXPORT);
     const bool show_unexported = (flags & ENV_UNEXPORT) || !(flags & ENV_EXPORT);
 
@@ -1089,7 +1086,7 @@ wcstring_list_t env_get_names(int flags) {
             if (n->new_scope)
                 break;
             else
-                n = n->next;
+                n = n->next.get();
         }
     }
 
@@ -1116,7 +1113,7 @@ static void get_exported(const env_node_t *n, std::map<wcstring, wcstring> *h) {
     if (n->new_scope)
         get_exported(vars_stack().global_env, h);
     else
-        get_exported(n->next, h);
+        get_exported(n->next.get(), h);
 
     var_table_t::const_iterator iter;
     for (iter = n->env.begin(); iter != n->env.end(); ++iter) {
@@ -1171,7 +1168,7 @@ void var_stack_t::update_export_array_if_necessary() {
 
     debug(4, L"env_export_arr() recalc");
 
-    get_exported(this->top, &vals);
+    get_exported(this->top.get(), &vals);
 
     if (uvars()) {
         const wcstring_list_t uni = uvars()->get_names(true, false);