From f28a648057cfeb0914100b12a00713547875675f Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Mon, 26 Apr 2021 22:07:13 -0700 Subject: [PATCH 1/4] Avoid allocating a HashKey for no-op table expiry iterations --- src/Val.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Val.cc b/src/Val.cc index f4842003c1..06499f9c6e 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -2542,7 +2542,6 @@ 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; @@ -2550,7 +2549,6 @@ void TableVal::DoExpire(double t) 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(); 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 ) From 76483a9efa0459c51985b72473095e294767aafe Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Mon, 26 Apr 2021 22:31:24 -0700 Subject: [PATCH 2/4] Remove saving/restoring of value pointer after calling expire_func 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. --- src/Val.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/Val.cc b/src/Val.cc index 06499f9c6e..bbb86bb3d5 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -2542,14 +2542,12 @@ void TableVal::DoExpire(double t) expire_iterator = new RobustDictIterator(std::move(it)); } - 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) ) { - v = (*expire_iterator)->GetValue(); + auto v = (*expire_iterator)->GetValue(); if ( v->ExpireAccessTime() == 0 ) { @@ -2573,12 +2571,10 @@ 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; continue; } From d51bd4bc4675d4d7234da0d11e31dbede5efe17c Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Mon, 26 Apr 2021 22:45:14 -0700 Subject: [PATCH 3/4] Fix using clear_table() within an &expire_func 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(). --- src/Val.cc | 8 +++++- .../Baseline/bifs.clear_table_expire_func/out | 2 ++ .../btest/bifs/clear_table_expire_func.zeek | 26 +++++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 testing/btest/Baseline/bifs.clear_table_expire_func/out create mode 100644 testing/btest/bifs/clear_table_expire_func.zeek diff --git a/src/Val.cc b/src/Val.cc index bbb86bb3d5..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; @@ -2575,6 +2577,10 @@ void TableVal::DoExpire(double t) if ( ! v ) { // user-provided function deleted it + if ( ! expire_iterator ) + // Entire table got dropped (e.g. clear_table() / RemoveAll()) + break; + continue; } @@ -2613,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; + } + From 292e3e18a3691d835b2d52ab8462a90ae47b0ae7 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Mon, 26 Apr 2021 23:03:32 -0700 Subject: [PATCH 4/4] Add a fatal error condition for invalid Dictionary insertion distances 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. --- src/Dict.cc | 6 ++++++ 1 file changed, 6 insertions(+) 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; }