From 076759877197ad11c426ee2b23d2c79983ffdef3 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Mon, 3 Jun 2019 15:20:30 +0000 Subject: [PATCH] GH-293: Protect copy() against reference cycles. 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. While the code is a bit more cumbersome this way, it could actually be bit faster as well as it no longer caches non-mutable values. (I measured it with the test suite: That's about the same in execution time, maybe tiny little bit faster now; definitly not slower). --- src/OpaqueVal.cc | 15 ++++++------ src/Val.cc | 10 +++++--- src/Val.h | 11 ++++++++- src/probabilistic/Topk.cc | 2 +- .../btest/Baseline/language.copy-cycle/out | 3 +++ testing/btest/language/copy-cycle.zeek | 23 +++++++++++++++++++ 6 files changed, 52 insertions(+), 12 deletions(-) create mode 100644 testing/btest/Baseline/language.copy-cycle/out create mode 100644 testing/btest/language/copy-cycle.zeek 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..a02aaac98c 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -1155,8 +1155,8 @@ 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)); + 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); @@ -1226,7 +1226,7 @@ Val* PatternVal::DoClone(CloneState* state) 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 +1331,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 +2542,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 +3160,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 +3457,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/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)); + }