Detect empty subnamespaces in [clear_global_variable] (bug #21093)

Once they become empty, there is no config left behind to clear, which causes persist_context::get_node() to consistently return NULL. Previously, this caused a straight null pointer dereference; in order to guard against undefined behavior, an assertion check was added to properly terminate execution first. Neither solution is correct. My understanding of this unwiedly mess is that persist_file_context::clear_var() recurses outwards from the original target pruning empty subnamespaces. At some point we are going to run out of things to delete, so just quit the recursion at that point and return to the caller and everything should be fine (or so it was in my test runs with different subnamespace layouts). (In general, the fact that this code completely lacks documentation of any sort and has several non-obvious layers to it is a *really* bad sign for maintainability of this feature in the future. Furthermore, this is not the first time we have had problems with glogal variables nested in subnamespaces -- see bug #20385, which was arguably more disastrous in practice.)
wesnoth · Oct 5, 2014 · 6dfcf27 · 6dfcf27
1 parent 3c8a52e
commit 6dfcf27
Show file tree

Hide file tree

Showing 2 changed files with 5 additions and 3 deletions.
diff --git a/changelog b/changelog
@@ -38,6 +38,8 @@ Version 1.11.16+dev:
    * Fix bug #22306: move_unit moves a unit even when it shouldn't
    * Fixed Gameplay -> Time of Day help topic displaying the Dawn ToD picture
      where the Second Watch picture should be used instead.
+   * Fixed mishandling of nested subnamespaces by the [clear_global_variable]
+     WML action causing an assertion failure (bug #21093).
 
 Version 1.11.16:
  * Add-ons client:

diff --git a/src/persist_context.cpp b/src/persist_context.cpp
@@ -141,9 +141,9 @@ bool persist_file_context::clear_var(const std::string &global, bool immediate)
 	while ((active->empty()) && (!namespace_.lineage_.empty())) {
 		name_space prev = namespace_.prev();
 		active = get_node(cfg_, prev);
-		/// @todo: This assertion replaces a seg fault. Still need to fix the
-		/// real bug (documented as bug #21093).
-		assert(active != NULL);
+		if (active == NULL) {
+			break;
+		}
 		active->clear_children(namespace_.node_);
 		if (active->has_child("variables") && active->child("variables").empty()) {
 			active->clear_children("variables");