From 59a3968fd26d5ca742e54eb15715bef3dd696e68 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Thu, 26 Jan 2017 12:40:55 -0800 Subject: [PATCH] Switch to using unique_ptr in env_node_t Makes our memory management of the variable stack more explicit --- src/env.cpp | 89 ++++++++++++++++++++++++++--------------------------- 1 file changed, 43 insertions(+), 46 deletions(-) diff --git a/src/env.cpp b/src/env.cpp index 849724cff..6edd50b3f 100644 --- a/src/env.cpp +++ b/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 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 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() { - this->top = new env_node_t(false); - this->global_env = this->top; - } - - ~var_stack_t() { - while (this->top) { - env_node_t *next = this->top->next; - delete this->top; - this->top = next; - } + var_stack_t() : top(new env_node_t(false)) { + this->global_env = this->top.get(); } // 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); - node->next = this->top; - this->top = node; - if (new_scope && local_scope_exports(this->top)) { + std::unique_ptr node(new env_node_t(new_scope)); + node->next = std::move(this->top); + this->top = std::move(node); + 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]); - if (result != killme->env.end()) { + var_table_t::iterator result = top->env.find(locale_variable[i]); + if (result != top->env.end()) { locale_changed = locale_variable[i]; break; } } - if (killme->new_scope) { //!OCLINT(collapsible if statements) - if (killme->exportv || local_scope_exports(killme->next)) { + if (top->new_scope) { //!OCLINT(collapsible if statements) + 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 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) { - const var_entry_t &entry = iter->second; + for (const auto &entry_pair : old_top->env) { + 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 *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);