Merge remote-tracking branch 'origin/topic/robin/clone-cycles'

* origin/topic/robin/clone-cycles:
  GH-293: Protect copy() against reference cycles.

CHANGES

@@ -1,4 +1,12 @@
 
+2.6-361 | 2019-06-04 10:30:21 -0700
+
+  * GH-293: Protect copy() against reference cycles. (Robin Sommer, Corelight)
+
+    Reference cycles shouldn't occur but there's nothing really preventing
+    people from creating them, so may just as well be safe and deal with
+    them when cloning values.
+
 2.6-359 | 2019-05-31 13:37:17 -0700
 
   * Remove old documentation reference to rotate_interval (Jon Siwek, Corelight)

VERSION

@@ -1 +1 @@
-2.6-359
+2.6-361

src/OpaqueVal.cc

@@ -97,7 +97,7 @@ Val* MD5Val::DoClone(CloneState* state)
 		EVP_MD_CTX_copy_ex(out->ctx, ctx);
 		}
 
-	return out;
+	return state->NewClone(this, out);
 	}
 
 void MD5Val::digest(val_list& vlist, u_char result[MD5_DIGEST_LENGTH])
@@ -241,7 +241,7 @@ Val* SHA1Val::DoClone(CloneState* state)
 		EVP_MD_CTX_copy_ex(out->ctx, ctx);
 		}
 
-	return out;
+	return state->NewClone(this, out);
 	}
 
 void SHA1Val::digest(val_list& vlist, u_char result[SHA_DIGEST_LENGTH])
@@ -376,7 +376,7 @@ Val* SHA256Val::DoClone(CloneState* state)
 		EVP_MD_CTX_copy_ex(out->ctx, ctx);
 		}
 
-	return out;
+	return state->NewClone(this, out);
 	}
 
 void SHA256Val::digest(val_list& vlist, u_char result[SHA256_DIGEST_LENGTH])
@@ -517,7 +517,7 @@ Val* EntropyVal::DoClone(CloneState* state)
 	uinfo.cache = false;
 	Val* clone = Unserialize(&uinfo, type);
 	free(data);
-	return clone;
+	return state->NewClone(this, clone);
 	}
 
 bool EntropyVal::Feed(const void* data, size_t size)
@@ -639,10 +639,10 @@ Val* BloomFilterVal::DoClone(CloneState* state)
 		{
 		auto bf = new BloomFilterVal(bloom_filter->Clone());
 		bf->Typify(type);
-		return bf;
+		return state->NewClone(this, bf);
 		}
 
-	return new BloomFilterVal();
+	return state->NewClone(this, new BloomFilterVal());
 	}
 
 bool BloomFilterVal::Typify(BroType* arg_type)
@@ -801,7 +801,8 @@ CardinalityVal::~CardinalityVal()
 
 Val* CardinalityVal::DoClone(CloneState* state)
 	{
-	return new CardinalityVal(new probabilistic::CardinalityCounter(*c));
+	return state->NewClone(this,
+			       new CardinalityVal(new probabilistic::CardinalityCounter(*c)));
 	}
 
 IMPLEMENT_SERIAL(CardinalityVal, SER_CARDINALITY_VAL);

src/Val.cc

@@ -86,8 +86,6 @@ Val* Val::Clone(CloneState* state)
 
 	auto c = DoClone(state);
 	assert(c);
-
-	state->clones.insert(std::make_pair(this, c));
 	return c;
 	}
 
@@ -1155,8 +1153,12 @@ unsigned int StringVal::MemoryAllocation() const
 
 Val* StringVal::DoClone(CloneState* state)
 	{
-	return new StringVal(new BroString((u_char*) val.string_val->Bytes(),
-	                                   val.string_val->Len(), 1));
+	// We could likely treat this type as immutable and return a reference
+	// instead of creating a new copy, but we first need to be careful and
+	// audit whether anything internal actually does mutate it.
+	return state->NewClone(this, new StringVal(
+	        new BroString((u_char*) val.string_val->Bytes(),
+	                      val.string_val->Len(), 1)));
 	}
 
 IMPLEMENT_SERIAL(StringVal, SER_STRING_VAL);
@@ -1223,10 +1225,13 @@ unsigned int PatternVal::MemoryAllocation() const
 
 Val* PatternVal::DoClone(CloneState* state)
 	{
+	// We could likely treat this type as immutable and return a reference
+	// instead of creating a new copy, but we first need to be careful and
+	// audit whether anything internal actually does mutate it.
 	auto re = new RE_Matcher(val.re_val->PatternText(),
 	                         val.re_val->AnywherePatternText());
 	re->Compile();
-	return new PatternVal(re);
+	return state->NewClone(this, new PatternVal(re));
 	}
 
 IMPLEMENT_SERIAL(PatternVal, SER_PATTERN_VAL);
@@ -1331,6 +1336,7 @@ Val* ListVal::DoClone(CloneState* state)
 	{
 	auto lv = new ListVal(tag);
 	lv->vals.resize(vals.length());
+	state->NewClone(this, lv);
 
 	loop_over_list(vals, i)
 		lv->Append(vals[i]->Clone(state));
@@ -2541,6 +2547,7 @@ void TableVal::ReadOperation(Val* index, TableEntryVal* v)
 Val* TableVal::DoClone(CloneState* state)
 	{
 	auto tv = new TableVal(table_type);
+	state->NewClone(this, tv);
 
 	const PDict(TableEntryVal)* tbl = AsTable();
 	IterCookie* cookie = tbl->InitForIteration();
@@ -3158,6 +3165,7 @@ Val* RecordVal::DoClone(CloneState* state)
 	// we don't touch it.
 	auto rv = new RecordVal(Type()->AsRecordType(), false);
 	rv->origin = nullptr;
+	state->NewClone(this, rv);
 
 	loop_over_list(*val.val_list_val, i)
 		{
@@ -3454,6 +3462,7 @@ Val* VectorVal::DoClone(CloneState* state)
 	{
 	auto vv = new VectorVal(vector_type);
 	vv->val.vector_val->reserve(val.vector_val->size());
+	state->NewClone(this, vv);
 
 	for ( unsigned int i = 0; i < val.vector_val->size(); ++i )
 		{

src/Val.h

@@ -424,7 +424,16 @@ protected:
 
 	// For internal use by the Val::Clone() methods.
 	struct CloneState {
-	    std::unordered_map<const Val*, Val*> clones;
+		// Caches a cloned value for later reuse during the same
+		// cloning operation. For recursive types, call this *before*
+		// descending down.
+		Val* NewClone(Val *src, Val* dst)
+			{
+			clones.insert(std::make_pair(src, dst));
+			return dst;
+			}
+
+		std::unordered_map<Val*, Val*> clones;
 	};
 
 	Val* Clone(CloneState* state);

src/file_analysis/analyzer/x509/X509.cc

@@ -483,7 +483,7 @@ Val* X509Val::DoClone(CloneState* state)
 	if ( certificate )
 		copy->certificate = X509_dup(certificate);
 
-	return copy;
+	return state->NewClone(this, copy);
 	}
 
 ::X509* X509Val::GetCertificate() const

src/probabilistic/Topk.cc

@@ -187,7 +187,7 @@ Val* TopkVal::DoClone(CloneState* state)
 	{
 	auto clone = new TopkVal(size);
 	clone->Merge(this);
-	return clone;
+	return state->NewClone(this, clone);
 	}
 
 bool TopkVal::DoSerialize(SerialInfo* info) const

testing/btest/Baseline/language.copy-cycle/out

@@ -0,0 +1,3 @@
+F (expected: F)
+T (expected: T)
+T (expected: T)

testing/btest/language/copy-cycle.zeek

@@ -0,0 +1,23 @@
+# @TEST-EXEC: zeek -b %INPUT >out
+# @TEST-EXEC: btest-diff out
+
+type B: record {
+	x: any &optional;
+	};
+
+type A: record  {
+        x: any &optional;
+        y: B;
+	};
+
+event zeek_init()
+	{
+	local x: A;
+	x$x = x;
+	x$y$x = x;
+	local y = copy(x);
+
+	print fmt("%s (expected: F)", same_object(x, y));
+	print fmt("%s (expected: T)", same_object(y, y$x));
+	print fmt("%s (expected: T)", same_object(y, y$y$x));
+	}