From 615f8cd443b2b31424b3a07cf499bfcc1bd0f934 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 30 Apr 2020 13:04:38 -0700 Subject: [PATCH 1/9] Fix bug with List where replace() doesn't work with non-pointer types --- src/List.h | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/List.h b/src/List.h index d4b9798e4d..57d26c462b 100644 --- a/src/List.h +++ b/src/List.h @@ -240,16 +240,16 @@ public: T replace(int ent_index, const T& new_ent) // replace entry #i with a new value { if ( ent_index < 0 ) - return 0; + return get_default_value(); - T old_ent = nullptr; + T old_ent = get_default_value(); if ( ent_index > num_entries - 1 ) { // replacement beyond the end of the list resize(ent_index + 1); for ( int i = num_entries; i < max_entries; ++i ) - entries[i] = nullptr; + entries[i] = get_default_value(); num_entries = max_entries; } else @@ -287,6 +287,20 @@ public: protected: + T get_default_value() + { + if constexpr ( std::is_pointer_v ) + return nullptr; + else if constexpr ( std::is_integral_v ) + return 0; + else if constexpr ( std::is_floating_point_v ) + return 0.0; + else + // TODO: Should this return an error at compile time that we don't + // know what this type is? + return T(); + } + // This could essentially be an std::vector if we wanted. Some // reasons to maybe not refactor to use std::vector ? // From 0558a7bfed976bfeef3a5d7cea6b40e9b650197b Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 30 Apr 2020 13:12:03 -0700 Subject: [PATCH 2/9] Add unit testing for List --- src/CMakeLists.txt | 1 + src/List.cc | 116 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+) create mode 100644 src/List.cc diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 8e533f5733..facc913a59 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -246,6 +246,7 @@ set(MAIN_SRCS IntSet.cc IP.cc IPAddr.cc + List.cc Reporter.cc NFA.cc Net.cc diff --git a/src/List.cc b/src/List.cc new file mode 100644 index 0000000000..8d70cc95c2 --- /dev/null +++ b/src/List.cc @@ -0,0 +1,116 @@ +#include +#include <3rdparty/doctest.h> + +TEST_CASE("list construction") + { + List list; + CHECK(list.empty()); + + List list2(10); + CHECK(list2.empty()); + CHECK(list2.max() == 10); + } + +TEST_CASE("list operation") + { + List list({ 1, 2, 3 }); + CHECK(list.size() == 3); + CHECK(list.max() == 3); + CHECK(list[0] == 1); + CHECK(list[1] == 2); + CHECK(list[2] == 3); + + // push_back forces a resize of the list here, which grows the list + // by a growth factor. That makes the max elements equal to 6. + list.push_back(4); + CHECK(list.size() == 4); + CHECK(list.max() == 6); + CHECK(list[3] == 4); + + CHECK(list.front() == 1); + CHECK(list.back() == 4); + + list.pop_front(); + CHECK(list.size() == 3); + CHECK(list.front() == 2); + + list.pop_back(); + CHECK(list.size() == 2); + CHECK(list.back() == 3); + + list.push_back(4); + CHECK(list.is_member(2)); + CHECK(list.member_pos(2) == 0); + + list.remove(2); + CHECK(list.size() == 2); + CHECK(list[0] == 3); + CHECK(list[1] == 4); + + // Squash the list down to the existing elements. + list.resize(); + CHECK(list.size() == 2); + CHECK(list.max() == 2); + + // Attempt replacing a known position. + int old = list.replace(0, 10); + CHECK(list.size() == 2); + CHECK(list.max() == 2); + CHECK(old == 3); + CHECK(list[0] == 10); + CHECK(list[1] == 4); + + // Attempt replacing an element off the end of the list, which + // causes a resize. + old = list.replace(3, 5); + CHECK(list.size() == 4); + CHECK(list.max() == 4); + CHECK(old == 0); + CHECK(list[0] == 10); + CHECK(list[1] == 4); + CHECK(list[2] == 0); + CHECK(list[3] == 5); + + // Attempt replacing an element with a negative index, which returns the + // default value for the list type. + old = list.replace(-1, 50); + CHECK(list.size() == 4); + CHECK(list.max() == 4); + CHECK(old == 0); + + list.clear(); + CHECK(list.size() == 0); + CHECK(list.max() == 0); + } + +TEST_CASE("list iteration") + { + List list({ 1, 2, 3, 4}); + + int index = 1; + for ( int v : list ) + CHECK(v == index++); + + index = 1; + for ( auto it = list.begin(); it != list.end(); index++, ++it) + CHECK(*it == index); + } + +TEST_CASE("plists") + { + PList list; + list.push_back(new int(1)); + list.push_back(new int(2)); + list.push_back(new int(3)); + + CHECK(*list[0] == 1); + + int* new_val = new int(5); + auto old = list.replace(-1, new_val); + delete new_val; + CHECK(old == nullptr); + + for ( auto v : list ) + delete v; + list.clear(); + } From 28e510084264f09304610ff6d1ac8a839ffe243e Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 30 Apr 2020 13:26:00 -0700 Subject: [PATCH 3/9] Add ability for List to be ordered/unordered This fixes a "bug" with List where remove_nth() can be an O(n) operation when it doesn't need to be. remove_nth for lists that don't necessarily need to keep an order can be an O(1) operation instead. --- src/List.cc | 14 ++++++++++++++ src/List.h | 36 ++++++++++++++++++++++++------------ 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/src/List.cc b/src/List.cc index 8d70cc95c2..1f2e4ec410 100644 --- a/src/List.cc +++ b/src/List.cc @@ -114,3 +114,17 @@ TEST_CASE("plists") delete v; list.clear(); } + +TEST_CASE("unordered list operation") + { + List list({1, 2, 3, 4}); + CHECK(list.size() == 4); + + // An unordered list doesn't maintain the ordering of the elements when + // one is removed. It just swaps the last element into the hole. + list.remove(2); + CHECK(list.size() == 3); + CHECK(list[0] == 1); + CHECK(list[1] == 4); + CHECK(list[2] == 3); + } diff --git a/src/List.h b/src/List.h index 57d26c462b..55256b35fb 100644 --- a/src/List.h +++ b/src/List.h @@ -29,7 +29,9 @@ // TODO: this can be removed in v3.1 when List::sort() is removed typedef int (*list_cmp_func)(const void* v1, const void* v2); -template +enum list_order { LIST_ORDERED, LIST_UNORDERED }; + +template class List { public: @@ -195,13 +197,11 @@ public: bool remove(const T& a) // delete entry from list { - for ( int i = 0; i < num_entries; ++i ) + int pos = member_pos(a); + if ( pos != -1 ) { - if ( a == entries[i] ) - { - remove_nth(i); - return true; - } + remove_nth(pos); + return true; } return false; @@ -212,10 +212,22 @@ public: assert(n >=0 && n < num_entries); T old_ent = entries[n]; - --num_entries; - for ( ; n < num_entries; ++n ) - entries[n] = entries[n+1]; + // For data where we don't care about ordering, we don't care about keeping + // the list in the same order when removing an element. Just swap the last + // element with the element being removed. + if ( Order == LIST_ORDERED ) + { + --num_entries; + + for ( ; n < num_entries; ++n ) + entries[n] = entries[n+1]; + } + else + { + entries[n] = entries[num_entries - 1]; + --num_entries; + } return old_ent; } @@ -332,8 +344,8 @@ protected: // Specialization of the List class to store pointers of a type. -template -using PList = List; +template +using PList = List; // Popular type of list: list of strings. typedef PList name_list; From 499a3353b53a1e3828ee3da9075c8176b37c4b7f Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 30 Apr 2020 13:27:29 -0700 Subject: [PATCH 4/9] Change timer_list in BroList to be an unordered list. This type is used by Conn and Analyzer to hold onto timers being added and removed. We don't expect the elements in those lists to maintain an order as the list is being modified. --- src/BroList.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/BroList.h b/src/BroList.h index 48e9ebee04..c7637b6bd0 100644 --- a/src/BroList.h +++ b/src/BroList.h @@ -23,4 +23,4 @@ class Attr; typedef PList attr_list; class Timer; -typedef PList timer_list; +typedef PList timer_list; From 9c44403c62616548e6172e4e7228506ff1020c29 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Thu, 7 May 2020 14:57:03 -0700 Subject: [PATCH 5/9] Fix crash when trying to redef non-existing enum Fixes GH-958 --- src/parse.y | 5 +++-- testing/btest/Baseline/language.enum-nonexisting/output | 1 + testing/btest/language/enum-nonexisting.zeek | 6 ++++++ 3 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 testing/btest/Baseline/language.enum-nonexisting/output create mode 100644 testing/btest/language/enum-nonexisting.zeek diff --git a/src/parse.y b/src/parse.y index 54761ca9df..d2f122a92b 100644 --- a/src/parse.y +++ b/src/parse.y @@ -155,13 +155,14 @@ static void parser_redef_enum (ID *id) /* Redef an enum. id points to the enum to be redefined. Let cur_enum_type point to it. */ assert(cur_enum_type == NULL); + // abort on errors; enums need to be accessible to continue parsing if ( ! id->Type() ) - id->Error("unknown identifier"); + reporter->FatalError("unknown enum identifier \"%s\"", id->Name()); else { cur_enum_type = id->Type()->AsEnumType(); if ( ! cur_enum_type ) - id->Error("not an enum"); + reporter->FatalError("identifier \"%s\" is not an enum", id->Name()); } } diff --git a/testing/btest/Baseline/language.enum-nonexisting/output b/testing/btest/Baseline/language.enum-nonexisting/output new file mode 100644 index 0000000000..fa32be3d46 --- /dev/null +++ b/testing/btest/Baseline/language.enum-nonexisting/output @@ -0,0 +1 @@ +fatal error in /path/to/zeek/testing/infrastructure/.tmp/language.enum-nonexisting/enum-nonexisting.zeek, line 4: unknown enum identifier "notexisting" diff --git a/testing/btest/language/enum-nonexisting.zeek b/testing/btest/language/enum-nonexisting.zeek new file mode 100644 index 0000000000..5dda65e664 --- /dev/null +++ b/testing/btest/language/enum-nonexisting.zeek @@ -0,0 +1,6 @@ +# @TEST-EXEC-FAIL: zeek -b %INPUT >output 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER="$SCRIPTS/diff-remove-abspath" btest-diff output + +redef enum notexisting += { + This_Causes_a_Segfault +}; From 695457fe44c4adfbf2edab955fee0074ef365980 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Thu, 7 May 2020 22:17:25 -0700 Subject: [PATCH 6/9] Unbreak build on Fedora 32 (gcc 10.0.1) It requires cstdint in a few more headers. --- src/Anon.h | 1 + src/Notifier.h | 1 + src/Reassem.h | 1 + 3 files changed, 3 insertions(+) diff --git a/src/Anon.h b/src/Anon.h index 3b2e704083..ecda53f023 100644 --- a/src/Anon.h +++ b/src/Anon.h @@ -12,6 +12,7 @@ #include #include +#include // TODO: Anon.h may not be the right place to put these functions ... diff --git a/src/Notifier.h b/src/Notifier.h index a3ae5188b7..d6618a5e5f 100644 --- a/src/Notifier.h +++ b/src/Notifier.h @@ -8,6 +8,7 @@ #pragma once #include +#include namespace notifier { diff --git a/src/Reassem.h b/src/Reassem.h index 4166f4dccb..079de77584 100644 --- a/src/Reassem.h +++ b/src/Reassem.h @@ -9,6 +9,7 @@ #include #include #include // for u_char +#include // Whenever subclassing the Reassembler class // you should add to this for known subclasses. From 2c04a562362d3046247417aa6695f747a94178e4 Mon Sep 17 00:00:00 2001 From: James Lagermann Date: Fri, 8 May 2020 12:31:56 -0500 Subject: [PATCH 7/9] added examples to set_to_regex comments Signed-ff-by: James Lagermann --- scripts/base/utils/patterns.zeek | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/scripts/base/utils/patterns.zeek b/scripts/base/utils/patterns.zeek index fc2ee6ba61..5f51aedd91 100644 --- a/scripts/base/utils/patterns.zeek +++ b/scripts/base/utils/patterns.zeek @@ -13,6 +13,13 @@ module GLOBAL; ## string parsing reducing it to a single backslash upon rendering. ## ## Returns: the input pattern with "~~" replaced by OR'd elements of input set. +## +## Examples: +## global r1 = set_to_regex(set("a", "b", "c"), "~~"); +## results: r1 = /^?(a|b|c)$?/ +## +## global r2 = set_to_regex(set("a.com", "b.com", "c.com"), "\\.(~~)"); +## results: r2 = /^?(\.(a\.com|b\.com|c\.com))$?/ function set_to_regex(ss: set[string], pat: string): pattern { local i: count = 0; From 690061b01c6d9de90a1552370819d4a309ee3293 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 7 May 2020 17:25:26 -0700 Subject: [PATCH 8/9] Review fixups --- src/BroList.h | 2 +- src/List.cc | 2 +- src/List.h | 28 +++++++--------------------- 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/src/BroList.h b/src/BroList.h index c7637b6bd0..a37f40d84a 100644 --- a/src/BroList.h +++ b/src/BroList.h @@ -23,4 +23,4 @@ class Attr; typedef PList attr_list; class Timer; -typedef PList timer_list; +typedef PList timer_list; diff --git a/src/List.cc b/src/List.cc index 1f2e4ec410..c19353b341 100644 --- a/src/List.cc +++ b/src/List.cc @@ -117,7 +117,7 @@ TEST_CASE("plists") TEST_CASE("unordered list operation") { - List list({1, 2, 3, 4}); + List list({1, 2, 3, 4}); CHECK(list.size() == 4); // An unordered list doesn't maintain the ordering of the elements when diff --git a/src/List.h b/src/List.h index 55256b35fb..c6d0902013 100644 --- a/src/List.h +++ b/src/List.h @@ -29,9 +29,9 @@ // TODO: this can be removed in v3.1 when List::sort() is removed typedef int (*list_cmp_func)(const void* v1, const void* v2); -enum list_order { LIST_ORDERED, LIST_UNORDERED }; +enum class ListOrder : int { ORDERED, UNORDERED }; -template +template class List { public: @@ -216,7 +216,7 @@ public: // For data where we don't care about ordering, we don't care about keeping // the list in the same order when removing an element. Just swap the last // element with the element being removed. - if ( Order == LIST_ORDERED ) + if constexpr ( Order == ListOrder::ORDERED ) { --num_entries; @@ -252,16 +252,16 @@ public: T replace(int ent_index, const T& new_ent) // replace entry #i with a new value { if ( ent_index < 0 ) - return get_default_value(); + return T{}; - T old_ent = get_default_value(); + T old_ent{}; if ( ent_index > num_entries - 1 ) { // replacement beyond the end of the list resize(ent_index + 1); for ( int i = num_entries; i < max_entries; ++i ) - entries[i] = get_default_value(); + entries[i] = T{}; num_entries = max_entries; } else @@ -299,20 +299,6 @@ public: protected: - T get_default_value() - { - if constexpr ( std::is_pointer_v ) - return nullptr; - else if constexpr ( std::is_integral_v ) - return 0; - else if constexpr ( std::is_floating_point_v ) - return 0.0; - else - // TODO: Should this return an error at compile time that we don't - // know what this type is? - return T(); - } - // This could essentially be an std::vector if we wanted. Some // reasons to maybe not refactor to use std::vector ? // @@ -344,7 +330,7 @@ protected: // Specialization of the List class to store pointers of a type. -template +template using PList = List; // Popular type of list: list of strings. From 61ce1b18fba7b093b0d3e2c6e5a4b6dc5a1bbcaf Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 6 Sep 2019 14:35:03 -0700 Subject: [PATCH 9/9] Limit rate at which MMDB error/status messages are emitted If there's some bad state we can be in where MMDB lookup/open operations consistently fail, then the volume of associated reporter messages can get overwhelmingly large especially if a lookup operation is being done for each network connection. This adds a limit of an arbitrary 20 messages every 5 minutes, which should be enough information to understand the overall open/close/lookup-failure pattern. --- src/zeek.bif | 43 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/src/zeek.bif b/src/zeek.bif index a4562508f6..9d0a4f673d 100644 --- a/src/zeek.bif +++ b/src/zeek.bif @@ -3639,6 +3639,32 @@ extern "C" { #include } +static int mmdb_msg_count = 0; +static constexpr int mmdb_msg_limit = 20; +static double mmdb_msg_suppression_time = 0; +static constexpr double mmdb_msg_suppression_duration = 300; + +static void report_mmdb_msg(const char* format, ...) + { + if ( network_time > mmdb_msg_suppression_time + mmdb_msg_suppression_duration ) + { + mmdb_msg_count = 0; + mmdb_msg_suppression_time = network_time; + } + + if ( mmdb_msg_count >= mmdb_msg_limit ) + return; + + ++mmdb_msg_count; + + va_list al; + va_start(al, format); + std::string msg = fmt(format, al); + va_end(al); + + reporter->Info("%s", msg.data()); + } + class MMDB { public: MMDB(const char* filename, struct stat info); @@ -3711,8 +3737,8 @@ bool MMDB::StaleDB() if ( buf.st_ino != file_info.st_ino || buf.st_mtime != file_info.st_mtime ) { - reporter->Info("Inode change detected for MaxMind DB [%s]", - mmdb.filename); + report_mmdb_msg("Inode change detected for MaxMind DB [%s]", + mmdb.filename); return true; } @@ -3757,8 +3783,8 @@ static bool mmdb_open(const char* filename, bool asn) else did_mmdb_loc_db_error = false; - reporter->Info("Failed to open MaxMind DB: %s [%s]", filename, - e.what()); + report_mmdb_msg("Failed to open MaxMind DB: %s [%s]", filename, + e.what()); return false; } @@ -3779,7 +3805,7 @@ static void mmdb_check_loc() { if ( mmdb_loc && mmdb_loc->StaleDB() ) { - reporter->Info("Closing stale MaxMind DB [%s]", mmdb_loc->Filename()); + report_mmdb_msg("Closing stale MaxMind DB [%s]", mmdb_loc->Filename()); did_mmdb_loc_db_error = false; mmdb_loc.release(); } @@ -3789,7 +3815,7 @@ static void mmdb_check_asn() { if ( mmdb_asn && mmdb_asn->StaleDB() ) { - reporter->Info("Closing stale MaxMind DB [%s]", mmdb_asn->Filename()); + report_mmdb_msg("Closing stale MaxMind DB [%s]", mmdb_asn->Filename()); did_mmdb_asn_db_error = false; mmdb_asn.release(); } @@ -3822,8 +3848,7 @@ static bool mmdb_lookup(const IPAddr& addr, MMDB_lookup_result_s& result, catch ( const std::exception& e ) { - reporter->Info("MaxMind DB lookup location error [%s]", - e.what()); + report_mmdb_msg("MaxMind DB lookup location error [%s]", e.what()); return false; } @@ -3873,7 +3898,7 @@ static Val* mmdb_getvalue(MMDB_entry_data_s* entry_data, int status, break; default: - reporter->Info("MaxMind DB error [%s]", MMDB_strerror(status)); + report_mmdb_msg("MaxMind DB error [%s]", MMDB_strerror(status)); break; }