diff --git a/CHANGES b/CHANGES index 288ad28024..21a7f2b3bf 100644 --- a/CHANGES +++ b/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) diff --git a/VERSION b/VERSION index 6942f1d443..889569c015 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -4.1.0-dev.549 +4.1.0-dev.554 diff --git a/src/Dict.cc b/src/Dict.cc index 47e350c98d..b438387375 100644 --- a/src/Dict.cc +++ b/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; } diff --git a/src/Val.cc b/src/Val.cc index f4842003c1..ca2670185d 100644 --- a/src/Val.cc +++ b/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; @@ -2542,16 +2544,12 @@ void TableVal::DoExpire(double t) expire_iterator = new RobustDictIterator(std::move(it)); } - std::unique_ptr 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(); + auto v = (*expire_iterator)->GetValue(); 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; diff --git a/testing/btest/Baseline/bifs.clear_table_expire_func/out b/testing/btest/Baseline/bifs.clear_table_expire_func/out new file mode 100644 index 0000000000..d31d49d5e0 --- /dev/null +++ b/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 diff --git a/testing/btest/bifs/clear_table_expire_func.zeek b/testing/btest/bifs/clear_table_expire_func.zeek new file mode 100644 index 0000000000..5f3eaee191 --- /dev/null +++ b/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; + } +