Merge remote-tracking branch 'origin/master' into topic/johanna/hash-unification

CHANGES

@@ -1,4 +1,47 @@
 
+3.2.0-dev.486 | 2020-05-11 11:11:51 -0700
+
+  * 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. (Jon Siwek, Corelight)
+
+3.2.0-dev.484 | 2020-05-08 11:50:54 -0700
+
+  * Change timer_list in BroList to be an unordered list. (Tim Wojtulewicz, Corelight)
+
+    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.
+
+  * Add ability for List to be ordered/unordered (Tim Wojtulewicz, Corelight)
+
+    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.
+
+  * Add unit testing for List (Tim Wojtulewicz, Corelight)
+
+  * Fix bug with List where replace() doesn't work with non-pointer types (Tim Wojtulewicz, Corelight)
+
+3.2.0-dev.478 | 2020-05-08 11:47:38 -0700
+
+  * Added examples to set_to_regex comments (James Lagermann, Corelight)
+
+  * Unbreak build on Fedora 32 (gcc 10.0.1) (Johanna Amann, Corelight)
+
+    It requires cstdint in a few more headers.
+
+3.2.0-dev.475 | 2020-05-07 17:15:23 -0700
+
+  * GH-958: Fix crash when trying to redef non-existing enum (Johanna Amann, Corelight)
+
 3.2.0-dev.473 | 2020-05-06 10:40:09 -0700
 
   * Revert addition of final modifier to JSON formatter (Tim Wojtulewicz, Corelight)

VERSION

@@ -1 +1 @@
-3.2.0-dev.473
+3.2.0-dev.486

doc

@@ -1 +1 @@
-Subproject commit 850c5bea8787c315cddc9079a29a17d89db055ec
+Subproject commit 73cce33f7ab3a08ee8c92bf7815a092eae7ad031

scripts/base/utils/patterns.zeek

@@ -4,7 +4,14 @@ module GLOBAL;
 
 ## Given a pattern as a string with two tildes (~~) contained in it, it will
 ## return a pattern with string set's elements OR'd together where the
-## double-tilde was given.
+## double-tilde was given.  Examples:
+##
+##   .. sourcecode:: zeek
+##
+##     global r1 = set_to_regex(set("a", "b", "c"), "~~");
+##     # r1 = /^?(a|b|c)$?/
+##     global r2 = set_to_regex(set("a.com", "b.com", "c.com"), "\\.(~~)");
+##     # r2 = /^?(\.(a\.com|b\.com|c\.com))$?/
 ##
 ## ss: a set of strings to OR together.
 ##

src/Anon.h

@@ -12,6 +12,7 @@
 
 #include <vector>
 #include <map>
+#include <cstdint>
 
 // TODO: Anon.h may not be the right place to put these functions ...
 

src/BroList.h

@@ -23,4 +23,4 @@ class Attr;
 typedef PList<Attr> attr_list;
 
 class Timer;
-typedef PList<Timer> timer_list;
+typedef PList<Timer, ListOrder::UNORDERED> timer_list;

src/CMakeLists.txt

@@ -246,6 +246,7 @@ set(MAIN_SRCS
     IntSet.cc
     IP.cc
     IPAddr.cc
+    List.cc
     Reporter.cc
     NFA.cc
     Net.cc

src/List.cc

@@ -0,0 +1,130 @@
+#include <List.h>
+#include <3rdparty/doctest.h>
+
+TEST_CASE("list construction")
+	{
+	List<int> list;
+	CHECK(list.empty());
+
+	List<int> list2(10);
+	CHECK(list2.empty());
+	CHECK(list2.max() == 10);
+	}
+
+TEST_CASE("list operation")
+	{
+	List<int> 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<int> 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<int> 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();
+	}
+
+TEST_CASE("unordered list operation")
+	{
+	List<int, ListOrder::UNORDERED> 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);
+	}

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<typename T>
+enum class ListOrder : int { ORDERED, UNORDERED };
+
+template<typename T, ListOrder Order = ListOrder::ORDERED>
 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(pos);
-				{
+			return true;
-				remove_nth(i);
-				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 )
+		// For data where we don't care about ordering, we don't care about keeping
-			entries[n] = entries[n+1];
+		// the list in the same order when removing an element. Just swap the last
+		// element with the element being removed.
+		if constexpr ( Order == ListOrder::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;
 		}
@@ -240,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 0;
+			return T{};
 
-		T old_ent = nullptr;
+		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] = nullptr;
+				entries[i] = T{};
 			num_entries = max_entries;
 			}
 		else
@@ -318,8 +330,8 @@ protected:
 
 
 // Specialization of the List class to store pointers of a type.
-template<typename T>
+template<typename T, ListOrder Order = ListOrder::ORDERED>
-using PList = List<T*>;
+using PList = List<T*, Order>;
 
 // Popular type of list: list of strings.
 typedef PList<char> name_list;

src/Notifier.h

@@ -8,6 +8,7 @@
 #pragma once
 
 #include <unordered_map>
+#include <cstdint>
 
 namespace notifier  {
 

src/Reassem.h

@@ -9,6 +9,7 @@
 #include <assert.h>
 #include <string.h>
 #include <sys/types.h> // for u_char
+#include <cstdint>
 
 // Whenever subclassing the Reassembler class
 // you should add to this for known subclasses.

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
 		{
+		if ( ! id->Type() || id->Type()->Tag() != TYPE_ENUM )
+			reporter->FatalError("identifier \"%s\" is not an enum", id->Name());
 		cur_enum_type = id->Type()->AsEnumType();
-		if ( ! cur_enum_type )
-			id->Error("not an enum");
 		}
 	}
 

src/zeek.bif

@@ -3640,6 +3640,32 @@ extern "C" {
 #include <netinet/ip.h>
 }
 
+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);
@@ -3712,8 +3738,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]",
+		report_mmdb_msg("Inode change detected for MaxMind DB [%s]",
-		               mmdb.filename);
+		                mmdb.filename);
 		return true;
 		}
 
@@ -3758,8 +3784,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,
+		report_mmdb_msg("Failed to open MaxMind DB: %s [%s]", filename,
-		               e.what());
+		                e.what());
 		return false;
 		}
 
@@ -3780,7 +3806,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();
 		}
@@ -3790,7 +3816,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();
 		}
@@ -3823,8 +3849,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]",
+		report_mmdb_msg("MaxMind DB lookup location error [%s]", e.what());
-		               e.what());
 		return false;
 		}
 
@@ -3874,7 +3899,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;
 		}
 

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"

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
+};