From b4f52ff311f4b58078e06b6a94faad9c53f1a15c Mon Sep 17 00:00:00 2001
From: Robin Sommer <robin@corelight.com>
Date: Mon, 11 Apr 2022 13:40:19 +0200
Subject: [PATCH] Fix robust iterators when modifying dictionary during
 iteration.

When inserting/deleting elements, we now remove their `DictEntries`
from any robust iterators' bookkeeping. First, we don't need that
information anymore, and second the `DictEntries` contain pointers
that may become invalid.

I don't know how to write a unit test for this unfortunately because
it depends on where exactly things land in the hash table.

Btw, memory mgmt for DictEntries is pretty fragile: They contain
pointers to both memory they own (`key`) and memory they don't own
(`value`). The former type of pointers is shallow-copied on
assignment/copy-construction, meaning that there can be multiple
instances seemingly owning the same memory. That only works because
deletion is manual, and not part of standard destruction. The second
type of pointer has a similar problem, except that it's managed
externally. It's important to not end up with multiple `DictEntries`
pointing to the same value (which is actually what that iterator
bookkeeping did).

Addresses #2032.
---
 src/Dict.cc | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/Dict.cc b/src/Dict.cc
index 62e4dd0d87..bd90014d9b 100644
--- a/src/Dict.cc
+++ b/src/Dict.cc
@@ -1299,6 +1299,15 @@ void Dictionary::AdjustOnInsert(IterCookie* c, const detail::DictEntry& entry, i
 	{
 	ASSERT(c);
 	ASSERT_VALID(c);
+
+	// Remove any previous instances of this value that we may have recorded as
+	// their pointers will get invalid. We won't need that knowledge anymore
+	// anyways, will update with new information below as needed.
+	c->inserted->erase(std::remove(c->inserted->begin(), c->inserted->end(), entry),
+	                   c->inserted->end());
+	c->visited->erase(std::remove(c->visited->begin(), c->visited->end(), entry),
+	                  c->visited->end());
+
 	if ( insert_position < c->next )
 		c->inserted->push_back(entry);
 	if ( insert_position < c->next && c->next <= last_affected_position )
@@ -1314,6 +1323,12 @@ void Dictionary::AdjustOnInsert(IterCookie* c, const detail::DictEntry& entry, i
 void Dictionary::AdjustOnInsert(RobustDictIterator* c, const detail::DictEntry& entry,
                                 int insert_position, int last_affected_position)
 	{
+	// See note in Dictionary::AdjustOnInsert() above.
+	c->inserted->erase(std::remove(c->inserted->begin(), c->inserted->end(), entry),
+	                   c->inserted->end());
+	c->visited->erase(std::remove(c->visited->begin(), c->visited->end(), entry),
+	                  c->visited->end());
+
 	if ( insert_position < c->next )
 		c->inserted->push_back(entry);
 	if ( insert_position < c->next && c->next <= last_affected_position )
@@ -1442,8 +1457,13 @@ void Dictionary::AdjustOnRemove(IterCookie* c, const detail::DictEntry& entry, i
                                 int last_affected_position)
 	{
 	ASSERT_VALID(c);
+
+	// See note in Dictionary::AdjustOnInsert() above.
 	c->inserted->erase(std::remove(c->inserted->begin(), c->inserted->end(), entry),
 	                   c->inserted->end());
+	c->visited->erase(std::remove(c->visited->begin(), c->visited->end(), entry),
+	                  c->visited->end());
+
 	if ( position < c->next && c->next <= last_affected_position )
 		{
 		int moved = HeadOfClusterByPosition(c->next - 1);
@@ -1462,8 +1482,12 @@ void Dictionary::AdjustOnRemove(IterCookie* c, const detail::DictEntry& entry, i
 void Dictionary::AdjustOnRemove(RobustDictIterator* c, const detail::DictEntry& entry, int position,
                                 int last_affected_position)
 	{
+	// See note in Dictionary::AdjustOnInsert() above.
 	c->inserted->erase(std::remove(c->inserted->begin(), c->inserted->end(), entry),
 	                   c->inserted->end());
+	c->visited->erase(std::remove(c->visited->begin(), c->visited->end(), entry),
+	                  c->visited->end());
+
 	if ( position < c->next && c->next <= last_affected_position )
 		{
 		int moved = HeadOfClusterByPosition(c->next - 1);