Merge remote-tracking branch 'origin/topic/jsiwek/gh-857-rebuild-tables-on-record-redef'

* origin/topic/jsiwek/gh-857-rebuild-tables-on-record-redef:
  Initialize RecordVal default fields when redef'd
  GH-857: fix redefining record types used to index tables
  Change RecordVals to get resized at time of RecordType redef

CHANGES

@@ -1,4 +1,25 @@
 
+3.2.0-dev.277 | 2020-03-20 11:29:36 -0700
+
+  * Initialize RecordVal default fields when redef'd (Jon Siwek, Corelight)
+
+    If a RecordVal had been created, but later its RecordType redef'd to
+    contain fields with &default, those fields were incorrectly left
+    uninitialized.
+
+  * GH-857: fix redefining record types used to index tables (Jon Siwek, Corelight)
+
+    This change tracks all TableVals created at parse-time whose index
+    depends on a given RecordType.  Should that RecordType be redef'd, those
+    TableVals are immediately rebuilt such that they are valid to
+    subsequently use in either parse-time initializations or eventually in
+    any arbitrary run-time expression.
+
+  * Change RecordVals to get resized at time of RecordType redef (Jon Siwek, Corelight)
+
+    Opposed to unconditionally checking all RecordVals whether they need to
+    be resized after parsing ends.
+
 3.2.0-dev.273 | 2020-03-20 10:05:21 -0700
 
   * GH-865: fix parsing of SMB NegotiateContextList

VERSION

@@ -1 +1 @@
-3.2.0-dev.273
+3.2.0-dev.277

doc

@@ -1 +1 @@
-Subproject commit 61a7ba44ff98021fcbe5bd5fe0888dbd3d45d2b5
+Subproject commit ab139e9180a3a968fbcd04bae12a4a752d0770a7

src/Type.cc

@@ -819,8 +819,16 @@ const char* RecordType::AddFields(type_decl_list* others, attr_list* attr)
 		{
 		if ( ! td->FindAttr(ATTR_DEFAULT) &&
 		     ! td->FindAttr(ATTR_OPTIONAL) )
+			{
+			delete others;
 			return "extension field must be &optional or have &default";
+			}
+		}
 
+	TableVal::SaveParseTimeTableState(this);
+
+	for ( const auto& td : *others )
+		{
 		if ( log )
 			{
 			if ( ! td->attrs )
@@ -835,6 +843,8 @@ const char* RecordType::AddFields(type_decl_list* others, attr_list* attr)
 	delete others;
 
 	num_fields = types->length();
+	RecordVal::ResizeParseTimeRecords(this);
+	TableVal::RebuildParseTimeTables();
 	return nullptr;
 	}
 

src/Val.cc

@@ -14,6 +14,7 @@
 #include <stdlib.h>
 
 #include <cmath>
+#include <set>
 
 #include "Attr.h"
 #include "BroString.h"
@@ -1325,10 +1326,64 @@ static void table_entry_val_delete_func(void* val)
 	delete tv;
 	}
 
+static void find_nested_record_types(BroType* t, std::set<RecordType*>* found)
+	{
+	if ( ! t )
+		return;
+
+	switch ( t->Tag() ) {
+	case TYPE_RECORD:
+		{
+		auto rt = t->AsRecordType();
+		found->emplace(rt);
+
+		for ( auto i = 0; i < rt->NumFields(); ++i )
+			find_nested_record_types(rt->FieldDecl(i)->type.get(), found);
+		}
+		return;
+	case TYPE_TABLE:
+		find_nested_record_types(t->AsTableType()->Indices(), found);
+		find_nested_record_types(t->AsTableType()->YieldType(), found);
+		return;
+	case TYPE_LIST:
+		{
+		for ( auto& t : *t->AsTypeList()->Types() )
+			find_nested_record_types(t, found);
+		}
+		return;
+	case TYPE_FUNC:
+		find_nested_record_types(t->AsFuncType()->Args(), found);
+		find_nested_record_types(t->AsFuncType()->YieldType(), found);
+		return;
+	case TYPE_VECTOR:
+		find_nested_record_types(t->AsVectorType()->YieldType(), found);
+		return;
+	case TYPE_TYPE:
+		find_nested_record_types(t->AsTypeType()->Type(), found);
+		return;
+	default:
+		return;
+	}
+	}
+
 TableVal::TableVal(IntrusivePtr<TableType> t, IntrusivePtr<Attributes> a) : Val(t.get())
 	{
 	Init(std::move(t));
 	SetAttrs(std::move(a));
+
+	if ( ! is_parsing )
+		return;
+
+	for ( const auto& t : *table_type->IndexTypes() )
+		{
+		std::set<RecordType*> found;
+		// TODO: this likely doesn't have to be repeated for each new TableVal,
+		//       can remember the resulting dependencies per TableType
+		find_nested_record_types(t, &found);
+
+		for ( auto rt : found )
+			parse_time_table_record_dependencies[rt].emplace_back(NewRef{}, this);
+		}
 	}
 
 void TableVal::Init(IntrusivePtr<TableType> t)
@@ -2529,7 +2584,69 @@ HashKey* TableVal::ComputeHash(const Val* index) const
 	return table_hash->ComputeHash(index, 1);
 	}
 
-vector<RecordVal*> RecordVal::parse_time_records;
+void TableVal::SaveParseTimeTableState(RecordType* rt)
+	{
+	auto it = parse_time_table_record_dependencies.find(rt);
+
+	if ( it == parse_time_table_record_dependencies.end() )
+		return;
+
+	auto& table_vals = it->second;
+
+	for ( auto& tv : table_vals )
+		parse_time_table_states[tv.get()] = tv->DumpTableState();
+	}
+
+void TableVal::RebuildParseTimeTables()
+	{
+	for ( auto& [tv, ptts] : parse_time_table_states )
+		tv->RebuildTable(std::move(ptts));
+
+	parse_time_table_states.clear();
+	}
+
+void TableVal::DoneParsing()
+	{
+	parse_time_table_record_dependencies.clear();
+	}
+
+TableVal::ParseTimeTableState TableVal::DumpTableState()
+	{
+	const PDict<TableEntryVal>* tbl = AsTable();
+	IterCookie* cookie = tbl->InitForIteration();
+
+	HashKey* key;
+	TableEntryVal* val;
+
+	ParseTimeTableState rval;
+
+	while ( (val = tbl->NextEntry(key, cookie)) )
+		{
+		rval.emplace_back(IntrusivePtr<Val>{AdoptRef{}, RecoverIndex(key)},
+		                  IntrusivePtr<Val>{NewRef{}, val->Value()});
+
+		delete key;
+		}
+
+	RemoveAll();
+	return rval;
+	}
+
+void TableVal::RebuildTable(ParseTimeTableState ptts)
+	{
+	delete table_hash;
+	table_hash = new CompositeHash(IntrusivePtr<TypeList>(NewRef{},
+	                               table_type->Indices()));
+
+	for ( auto& [key, val] : ptts )
+		Assign(key.get(), val.release());
+	}
+
+TableVal::ParseTimeTableStates TableVal::parse_time_table_states;
+
+TableVal::TableRecordDependencies TableVal::parse_time_table_record_dependencies;
+
+RecordVal::RecordTypeValMap RecordVal::parse_time_records;
 
 RecordVal::RecordVal(RecordType* t, bool init_fields) : Val(t)
 	{
@@ -2538,10 +2655,7 @@ RecordVal::RecordVal(RecordType* t, bool init_fields) : Val(t)
 	val_list* vl = val.val_list_val = new val_list(n);
 
 	if ( is_parsing )
-		{
+		parse_time_records[t].emplace_back(NewRef{}, this);
-		parse_time_records.emplace_back(this);
-		Ref();
-		}
 
 	if ( ! init_fields )
 		return;
@@ -2621,12 +2735,18 @@ IntrusivePtr<Val> RecordVal::LookupWithDefault(int field) const
 	return Type()->AsRecordType()->FieldDefault(field);
 	}
 
-void RecordVal::ResizeParseTimeRecords()
+void RecordVal::ResizeParseTimeRecords(RecordType* rt)
 	{
-	for ( auto& rv : parse_time_records )
+	auto it = parse_time_records.find(rt);
+
+	if ( it == parse_time_records.end() )
+		return;
+
+	auto& rvs = it->second;
+
+	for ( auto& rv : rvs )
 		{
 		auto vs = rv->val.val_list_val;
-		auto rt = rv->Type()->AsRecordType();
 		auto current_length = vs->length();
 		auto required_length = rt->NumFields();
 
@@ -2635,12 +2755,13 @@ void RecordVal::ResizeParseTimeRecords()
 			vs->resize(required_length);
 
 			for ( auto i = current_length; i < required_length; ++i )
-				vs->replace(i, nullptr);
+				vs->replace(i, rt->FieldDefault(i).release());
 			}
-
-		Unref(rv);
 		}
+	}
 
+void RecordVal::DoneParsing()
+	{
 	parse_time_records.clear();
 	}
 

src/Val.h

@@ -811,9 +811,30 @@ public:
 
 	notifier::Modifiable* Modifiable() override	{ return this; }
 
+	// Retrieves and saves all table state (key-value pairs) for
+	// tables whose index type depends on the given RecordType.
+	static void SaveParseTimeTableState(RecordType* rt);
+
+	// Rebuilds all TableVals whose state was previously saved by
+	// SaveParseTimeTableState().  This is used to re-recreate the tables
+	// in the event that a record type gets redefined while parsing.
+	static void RebuildParseTimeTables();
+
+	// Clears all state that was used to track TableVals that depending
+	// on RecordTypes.
+	static void DoneParsing();
+
 protected:
 	void Init(IntrusivePtr<TableType> t);
 
+	using TableRecordDependencies = std::unordered_map<RecordType*, std::vector<IntrusivePtr<TableVal>>>;
+
+	using ParseTimeTableState = std::vector<std::pair<IntrusivePtr<Val>, IntrusivePtr<Val>>>;
+	using ParseTimeTableStates = std::unordered_map<TableVal*, ParseTimeTableState>;
+
+	ParseTimeTableState DumpTableState();
+	void RebuildTable(ParseTimeTableState ptts);
+
 	void CheckExpireAttr(attr_tag at);
 	int ExpandCompoundAndInit(val_list* vl, int k, IntrusivePtr<Val> new_val);
 	int CheckAndAssign(Val* index, IntrusivePtr<Val> new_val);
@@ -853,6 +874,9 @@ protected:
 	IntrusivePtr<Expr> change_func;
 	// prevent recursion of change functions
 	bool in_change_func = false;
+
+	static TableRecordDependencies parse_time_table_record_dependencies;
+	static ParseTimeTableStates parse_time_table_states;
 };
 
 class RecordVal : public Val, public notifier::Modifiable {
@@ -911,14 +935,17 @@ public:
 	// Extend the underlying arrays of record instances created during
 	// parsing to match the number of fields in the record type (they may
 	// mismatch as a result of parse-time record type redefinitions.
-	static void ResizeParseTimeRecords();
+	static void ResizeParseTimeRecords(RecordType* rt);
+
+	static void DoneParsing();
 
 protected:
 	IntrusivePtr<Val> DoClone(CloneState* state) override;
 
 	BroObj* origin;
 
-	static vector<RecordVal*> parse_time_records;
+	using RecordTypeValMap = std::unordered_map<RecordType*, std::vector<IntrusivePtr<RecordVal>>>;
+	static RecordTypeValMap parse_time_records;
 };
 
 class EnumVal : public Val {

src/main.cc

@@ -649,7 +649,8 @@ int main(int argc, char** argv)
 	yyparse();
 	is_parsing = false;
 
-	RecordVal::ResizeParseTimeRecords();
+	RecordVal::DoneParsing();
+	TableVal::DoneParsing();
 
 	init_general_global_var();
 	init_net_var();

testing/btest/Baseline/language.record-redef-after-init/output

@@ -1,10 +1,11 @@
-[a=redef, d=<uninitialized>, e=<uninitialized>, f=<uninitialized>, g=<uninitialized>, h=<uninitialized>, i=<uninitialized>, j=<uninitialized>, k=<uninitialized>, l=<uninitialized>, m=<uninitialized>, n=<uninitialized>, o=<uninitialized>, p=<uninitialized>, q=<uninitialized>]
+[a=redef, d=<uninitialized>, e=<uninitialized>, f=<uninitialized>, g=<uninitialized>, h=<uninitialized>, i=<uninitialized>, j=<uninitialized>, k=<uninitialized>, l=<uninitialized>, m=<uninitialized>, n=<uninitialized>, o=<uninitialized>, p=<uninitialized>, q=OPTQ]
 [a=runtime, d=<uninitialized>, e=<uninitialized>, f=<uninitialized>, g=<uninitialized>, h=<uninitialized>, i=<uninitialized>, j=<uninitialized>, k=<uninitialized>, l=<uninitialized>, m=<uninitialized>, n=<uninitialized>, o=<uninitialized>, p=<uninitialized>, q=OPTQ]
 [a=local, d=<uninitialized>, e=<uninitialized>, f=<uninitialized>, g=<uninitialized>, h=<uninitialized>, i=<uninitialized>, j=<uninitialized>, k=<uninitialized>, l=<uninitialized>, m=<uninitialized>, n=<uninitialized>, o=<uninitialized>, p=<uninitialized>, q=OPTQ]
-[a=redef, d=<uninitialized>, e=<uninitialized>, f=<uninitialized>, g=<uninitialized>, h=<uninitialized>, i=<uninitialized>, j=<uninitialized>, k=<uninitialized>, l=<uninitialized>, m=<uninitialized>, n=<uninitialized>, o=<uninitialized>, p=<uninitialized>, q=<uninitialized>]
+[a=redef, d=<uninitialized>, e=<uninitialized>, f=<uninitialized>, g=<uninitialized>, h=<uninitialized>, i=<uninitialized>, j=<uninitialized>, k=<uninitialized>, l=<uninitialized>, m=<uninitialized>, n=<uninitialized>, o=<uninitialized>, p=<uninitialized>, q=OPTQ]
-[a=redef, d=<uninitialized>, e=<uninitialized>, f=<uninitialized>, g=<uninitialized>, h=<uninitialized>, i=<uninitialized>, j=<uninitialized>, k=<uninitialized>, l=<uninitialized>, m=<uninitialized>, n=<uninitialized>, o=<uninitialized>, p=<uninitialized>, q=<uninitialized>]
+[a=redef, d=<uninitialized>, e=<uninitialized>, f=<uninitialized>, g=<uninitialized>, h=<uninitialized>, i=<uninitialized>, j=<uninitialized>, k=<uninitialized>, l=<uninitialized>, m=<uninitialized>, n=<uninitialized>, o=<uninitialized>, p=<uninitialized>, q=OPTQ]
+OPTQ
 newp
-[a=redef, d=<uninitialized>, e=<uninitialized>, f=<uninitialized>, g=<uninitialized>, h=<uninitialized>, i=<uninitialized>, j=<uninitialized>, k=<uninitialized>, l=<uninitialized>, m=<uninitialized>, n=<uninitialized>, o=<uninitialized>, p=newp, q=<uninitialized>]
+[a=redef, d=<uninitialized>, e=<uninitialized>, f=<uninitialized>, g=<uninitialized>, h=<uninitialized>, i=<uninitialized>, j=<uninitialized>, k=<uninitialized>, l=<uninitialized>, m=<uninitialized>, n=<uninitialized>, o=<uninitialized>, p=newp, q=OPTQ]
 OPTQ
 our value
 [a=redef, d=<uninitialized>, e=<uninitialized>, f=<uninitialized>, g=<uninitialized>, h=<uninitialized>, i=<uninitialized>, j=<uninitialized>, k=<uninitialized>, l=<uninitialized>, m=<uninitialized>, n=<uninitialized>, o=<uninitialized>, p=newp, q=our value]

testing/btest/Baseline/language.table-record-idx-redef/out

@@ -0,0 +1,5 @@
+F, T, F
+{
+[[r=[rr=101, rrr=blue pill], a=13, b=28]] = 42,
+[[r=[rr=101, rrr=blue pill], a=37, b=28]] = 1
+}

testing/btest/language/table-record-idx-redef.zeek

@@ -0,0 +1,33 @@
+# @TEST-EXEC: zeek -b %INPUT >out
+# @TEST-EXEC: btest-diff out
+
+type recrec: record {
+	rr: count &default = 101;
+};
+
+type myrec: record {
+	r: recrec &default=recrec();
+	a: count &default=13;
+};
+
+global mr = myrec($a = 37);
+global active: table[myrec] of count = table([mr] = 1);
+
+redef record myrec += {
+	b: count &default=28;
+};
+
+redef record recrec += {
+	rrr: string &default="blue pill";
+};
+
+global check1: bool = myrec() in active;
+global check2: bool = mr in active;
+global check3: bool = myrec($a=37, $b=0) in active;
+
+event zeek_init()
+	{
+	print check1, check2, check3;
+	active[myrec()] = 42;
+	print active;
+	}