diff --git a/CHANGES b/CHANGES index 2780c1d353..2bf9485d30 100644 --- a/CHANGES +++ b/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) diff --git a/VERSION b/VERSION index 6ff97a7bfc..368e86895d 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.6-359 +2.6-361 diff --git a/src/OpaqueVal.cc b/src/OpaqueVal.cc index 5b6c9aa483..bf91c3f40d 100644 --- a/src/OpaqueVal.cc +++ b/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); diff --git a/src/Val.cc b/src/Val.cc index 50c36d7239..49eafc7929 100644 --- a/src/Val.cc +++ b/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 ) { diff --git a/src/Val.h b/src/Val.h index c253c265db..d17dc24cb2 100644 --- a/src/Val.h +++ b/src/Val.h @@ -424,7 +424,16 @@ protected: // For internal use by the Val::Clone() methods. struct CloneState { - std::unordered_map 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 clones; }; Val* Clone(CloneState* state); diff --git a/src/file_analysis/analyzer/x509/X509.cc b/src/file_analysis/analyzer/x509/X509.cc index 2db537bfc2..dfbea70686 100644 --- a/src/file_analysis/analyzer/x509/X509.cc +++ b/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 diff --git a/src/probabilistic/Topk.cc b/src/probabilistic/Topk.cc index d143c5834e..c64357c17f 100644 --- a/src/probabilistic/Topk.cc +++ b/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 diff --git a/testing/btest/Baseline/language.copy-cycle/out b/testing/btest/Baseline/language.copy-cycle/out new file mode 100644 index 0000000000..57562e818e --- /dev/null +++ b/testing/btest/Baseline/language.copy-cycle/out @@ -0,0 +1,3 @@ +F (expected: F) +T (expected: T) +T (expected: T) diff --git a/testing/btest/language/copy-cycle.zeek b/testing/btest/language/copy-cycle.zeek new file mode 100644 index 0000000000..347affeb40 --- /dev/null +++ b/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)); + }