Undo: free and search the undo tree iteratively
Recursion depth equalled history length, so destroying or clearing a buffer with a few hundred thousand undo steps overflowed the stack. UndoSystem::clear() also freed only the first root branch, leaking the root's siblings; it now frees the whole forest. Audit finding M4. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB
This commit is contained in:
+26
-27
@@ -2,6 +2,7 @@
|
|||||||
#include "Buffer.h"
|
#include "Buffer.h"
|
||||||
#include <cassert>
|
#include <cassert>
|
||||||
#include <cstdio>
|
#include <cstdio>
|
||||||
|
#include <vector>
|
||||||
|
|
||||||
|
|
||||||
UndoSystem::UndoSystem(Buffer &owner, UndoTree &tree)
|
UndoSystem::UndoSystem(Buffer &owner, UndoTree &tree)
|
||||||
@@ -260,7 +261,8 @@ void
|
|||||||
UndoSystem::clear()
|
UndoSystem::clear()
|
||||||
{
|
{
|
||||||
discard_pending();
|
discard_pending();
|
||||||
free_node(tree_.root);
|
// The root may have siblings (edits made after undoing to the start).
|
||||||
|
FreeUndoForest(tree_.root);
|
||||||
tree_.root = nullptr;
|
tree_.root = nullptr;
|
||||||
tree_.current = nullptr;
|
tree_.current = nullptr;
|
||||||
tree_.saved = nullptr;
|
tree_.saved = nullptr;
|
||||||
@@ -343,20 +345,13 @@ UndoSystem::apply(const UndoNode *node, int direction)
|
|||||||
void
|
void
|
||||||
UndoSystem::free_node(UndoNode *node)
|
UndoSystem::free_node(UndoNode *node)
|
||||||
{
|
{
|
||||||
|
// Free this node and its child subtree, but not its siblings.
|
||||||
if (!node)
|
if (!node)
|
||||||
return;
|
return;
|
||||||
// Free child subtree(s) and sibling branches
|
UndoNode *children = node->child;
|
||||||
if (node->child) {
|
node->child = nullptr;
|
||||||
// Free entire redo list starting at child, including each subtree
|
|
||||||
UndoNode *branch = node->child;
|
|
||||||
while (branch) {
|
|
||||||
UndoNode *next = branch->next;
|
|
||||||
free_node(branch);
|
|
||||||
branch = next;
|
|
||||||
}
|
|
||||||
node->child = nullptr;
|
|
||||||
}
|
|
||||||
delete node;
|
delete node;
|
||||||
|
FreeUndoForest(children);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
@@ -364,26 +359,27 @@ void
|
|||||||
UndoSystem::free_branch(UndoNode *node)
|
UndoSystem::free_branch(UndoNode *node)
|
||||||
{
|
{
|
||||||
// Free a branch list (node and its next siblings) including their subtrees
|
// Free a branch list (node and its next siblings) including their subtrees
|
||||||
while (node) {
|
FreeUndoForest(node);
|
||||||
UndoNode *next = node->next;
|
|
||||||
free_node(node);
|
|
||||||
node = next;
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
// Iterative search for `target` below `cur`; sets out_parent to its parent.
|
||||||
static bool
|
static bool
|
||||||
dfs_find_parent(UndoNode *cur, UndoNode *target, UndoNode *&out_parent)
|
dfs_find_parent(UndoNode *cur, UndoNode *target, UndoNode *&out_parent)
|
||||||
{
|
{
|
||||||
if (!cur)
|
if (!cur)
|
||||||
return false;
|
return false;
|
||||||
for (UndoNode *child = cur->child; child != nullptr; child = child->next) {
|
std::vector<UndoNode *> stack{cur};
|
||||||
if (child == target) {
|
while (!stack.empty()) {
|
||||||
out_parent = cur;
|
UndoNode *node = stack.back();
|
||||||
return true;
|
stack.pop_back();
|
||||||
|
for (UndoNode *child = node->child; child != nullptr; child = child->next) {
|
||||||
|
if (child == target) {
|
||||||
|
out_parent = node;
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
stack.push_back(child);
|
||||||
}
|
}
|
||||||
if (dfs_find_parent(child, target, out_parent))
|
|
||||||
return true;
|
|
||||||
}
|
}
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
@@ -447,11 +443,14 @@ UndoSystem::is_descendant(UndoNode *root, const UndoNode *target)
|
|||||||
{
|
{
|
||||||
if (!root || !target)
|
if (!root || !target)
|
||||||
return false;
|
return false;
|
||||||
if (root == target)
|
std::vector<UndoNode *> stack{root};
|
||||||
return true;
|
while (!stack.empty()) {
|
||||||
for (UndoNode *child = root->child; child != nullptr; child = child->next) {
|
UndoNode *node = stack.back();
|
||||||
if (is_descendant(child, target))
|
stack.pop_back();
|
||||||
|
if (node == target)
|
||||||
return true;
|
return true;
|
||||||
|
for (UndoNode *child = node->child; child != nullptr; child = child->next)
|
||||||
|
stack.push_back(child);
|
||||||
}
|
}
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|||||||
+17
-10
@@ -1,24 +1,31 @@
|
|||||||
// Undo logic is implemented in UndoSystem; this file only owns node lifetime.
|
// Undo logic is implemented in UndoSystem; this file only owns node lifetime.
|
||||||
#include "UndoTree.h"
|
#include "UndoTree.h"
|
||||||
|
|
||||||
namespace {
|
#include <vector>
|
||||||
|
|
||||||
|
|
||||||
void
|
void
|
||||||
free_node_graph(UndoNode *node)
|
FreeUndoForest(UndoNode *first)
|
||||||
{
|
{
|
||||||
// Walk the sibling (redo-branch) list; for each node, recursively free its
|
// Nodes form a first-child/next-sibling tree whose depth equals the
|
||||||
// child subtree first, then the node itself.
|
// length of the edit history, so walk it with an explicit stack.
|
||||||
while (node) {
|
std::vector<UndoNode *> stack;
|
||||||
UndoNode *next = node->next;
|
if (first)
|
||||||
free_node_graph(node->child);
|
stack.push_back(first);
|
||||||
|
while (!stack.empty()) {
|
||||||
|
UndoNode *node = stack.back();
|
||||||
|
stack.pop_back();
|
||||||
|
if (node->child)
|
||||||
|
stack.push_back(node->child);
|
||||||
|
if (node->next)
|
||||||
|
stack.push_back(node->next);
|
||||||
delete node;
|
delete node;
|
||||||
node = next;
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
} // namespace
|
|
||||||
|
|
||||||
|
|
||||||
UndoTree::~UndoTree()
|
UndoTree::~UndoTree()
|
||||||
{
|
{
|
||||||
free_node_graph(root);
|
FreeUndoForest(root);
|
||||||
delete pending;
|
delete pending;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -15,3 +15,8 @@ struct UndoTree {
|
|||||||
// created for it.
|
// created for it.
|
||||||
~UndoTree();
|
~UndoTree();
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|
||||||
|
// Frees `first`, its whole child subtree, and every node on its `next`
|
||||||
|
// (sibling) chain. Iterative, so history depth cannot overflow the stack.
|
||||||
|
void FreeUndoForest(UndoNode *first);
|
||||||
|
|||||||
Reference in New Issue
Block a user