Merge remote-tracking branch 'origin/topic/jsiwek/misc-table-stuff'

* origin/topic/jsiwek/misc-table-stuff:
  Add a fatal error condition for invalid Dictionary insertion distances
  Fix using clear_table() within an &expire_func
  Remove saving/restoring of value pointer after calling expire_func
  Avoid allocating a HashKey for no-op table expiry iterations

CHANGES

@@ -1,4 +1,39 @@
 
+4.1.0-dev.554 | 2021-04-28 13:45:53 -0700
+
+  * Add a fatal error condition for invalid Dictionary insertion distances (Jon Siwek, Corelight)
+
+    When choosing poor/aggressive values for `table_expire_interval`,
+    `table_expire_delay`, and/or `table_incremental_step` that tend to
+    leave tables in state of constant table-expiry-iteration, the underlying
+    Dictionary is never allowed the chance to complete remapping operations
+    which re-position entries to more ideal locations (e.g. after
+    reallocating the table to be able to store more entries).
+
+    That situation not only leads to the Dictionary generally having a less
+    efficient structure, but eventually, the lack of re-positioning may
+    cause an insertion to calculate the new entry's
+    distance-from-ideal-position to be a value requiring a full 16-bits or
+    more (>=65535), but an entry only allows storing 16-bit distance values,
+    with 65535 being a sentinel value that is supposed to indicate an empty
+    entry.  Dictionary operations may start misbehaving if that's allowed to
+    happen.
+
+  * Fix using clear_table() within an &expire_func (Jon Siwek, Corelight)
+
+    This previously crashed since clear_table()/TableVal::RemoveAll() left
+    behind a stale iterator to the old table causing a heap-use-after-free
+    when resuming table expiry iteration in TableVal::DoExpire().
+
+  * Remove saving/restoring of value pointer after calling expire_func (Jon Siwek, Corelight)
+
+    It's no longer used for anything.  Previously, it was used to detect
+    whether the expiry batch finished iterating the entire table or not, but
+    that's now determined by directly checking if the iterator itself
+    signifies the end of the table.
+
+  * Avoid allocating a HashKey for no-op table expiry iterations (Jon Siwek, Corelight)
+
 4.1.0-dev.549 | 2021-04-28 13:09:30 -0700
 
   * Fix -Wsign-compare warnings in Debug{Cmds}.cc (Jon Siwek, Corelight)

VERSION

@@ -1 +1 @@
-4.1.0-dev.549
+4.1.0-dev.554

src/Dict.cc

@@ -1027,8 +1027,14 @@ int Dictionary::LookupIndex(const void* key, int key_size, detail::hash_t hash,
 		*insert_position = i;
 
 	if ( insert_distance )
+		{
 		*insert_distance = i - bucket;
 
+		if ( *insert_distance >= detail::TOO_FAR_TO_REACH )
+			reporter->FatalErrorWithCore("Dictionary (size %d) insertion distance too far: %d",
+			                             Length(), *insert_distance);
+		}
+
 	return -1;
 	}
 

src/Val.cc

@@ -1459,6 +1459,8 @@ TableVal::~TableVal()
 
 void TableVal::RemoveAll()
 	{
+	delete expire_iterator;
+	expire_iterator = nullptr;
 	// Here we take the brute force approach.
 	delete table_val;
 	table_val = new PDict<TableEntryVal>;
@@ -2542,16 +2544,12 @@ void TableVal::DoExpire(double t)
 		expire_iterator = new RobustDictIterator(std::move(it));
 		}
 
-	std::unique_ptr<detail::HashKey> k = nullptr;
-	TableEntryVal* v = nullptr;
-	TableEntryVal* v_saved = nullptr;
 	bool modified = false;
 
 	for ( int i = 0; i < zeek::detail::table_incremental_step &&
 		      *expire_iterator != table_val->end_robust(); ++i, ++(*expire_iterator) )
 		{
-		k = (*expire_iterator)->GetHashKey();
-		v = (*expire_iterator)->GetValue<TableEntryVal*>();
+		auto v = (*expire_iterator)->GetValue<TableEntryVal*>();
 
 		if ( v->ExpireAccessTime() == 0 )
 			{
@@ -2564,6 +2562,7 @@ void TableVal::DoExpire(double t)
 
 		else if ( v->ExpireAccessTime() + timeout < t )
 			{
+			auto k = (*expire_iterator)->GetHashKey();
 			ListValPtr idx = nullptr;
 
 			if ( expire_func )
@@ -2574,12 +2573,14 @@ void TableVal::DoExpire(double t)
 				// It's possible that the user-provided
 				// function modified or deleted the table
 				// value, so look it up again.
-				v_saved = v;
 				v = table_val->Lookup(k.get());
 
 				if ( ! v )
 					{ // user-provided function deleted it
-					v = v_saved;
+					if ( ! expire_iterator )
+						// Entire table got dropped (e.g. clear_table() / RemoveAll())
+						break;
+
 					continue;
 					}
 
@@ -2618,7 +2619,7 @@ void TableVal::DoExpire(double t)
 	if ( modified )
 		Modified();
 
-	if ( (*expire_iterator) == table_val->end_robust() )
+	if ( ! expire_iterator || (*expire_iterator) == table_val->end_robust() )
 		{
 		delete expire_iterator;
 		expire_iterator = nullptr;

testing/btest/Baseline/bifs.clear_table_expire_func/out

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+expire

testing/btest/bifs/clear_table_expire_func.zeek

@@ -0,0 +1,26 @@
+# @TEST-EXEC: zeek -b %INPUT > out
+# @TEST-EXEC: btest-diff out
+# @TEST-DOC: Checks use of clear_table() within an &expire_func works.
+
+redef exit_only_after_terminate=T;
+redef table_expire_interval = 1msec;
+
+global myexpire: function(t: table[count] of count, i: count): interval;
+
+global mt: table[count] of count &create_expire=1msec &expire_func=myexpire;
+
+function myexpire(t: table[count] of count, i: count): interval
+	{
+	print "expire";
+	clear_table(mt);
+	terminate();
+	return 0secs;
+	}
+
+event zeek_init()
+	{
+	mt[0] = 0;
+	mt[1] = 1;
+	mt[2] = 2;
+	}
+