From e2bf12d54ab4720fe2ac48ecc06ad879d67d6a02 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 22 Feb 2020 11:01:59 +0100 Subject: [PATCH] Val: pass IntrusivePtr<> to TableVal::ExpandAndInit() Clarifies ownership and fixes memory leaks. Closes https://github.com/zeek/zeek/issues/811 --- src/Expr.cc | 12 ++++-------- src/Val.cc | 41 +++++++++++++++-------------------------- src/Val.h | 7 ++++--- 3 files changed, 23 insertions(+), 37 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index 2b4839c79a..80b4e18685 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -2477,19 +2477,17 @@ Val* AssignExpr::InitVal(const BroType* t, Val* aggr) const TableVal* tv = aggr->AsTableVal(); const TableType* tt = tv->Type()->AsTableType(); const BroType* yt = tv->Type()->YieldType(); - Val* index = op1->InitVal(tt->Indices(), 0); - Val* v = op2->InitVal(yt, 0); + IntrusivePtr index{AdoptRef{}, op1->InitVal(tt->Indices(), 0)}; + IntrusivePtr v{AdoptRef{}, op2->InitVal(yt, 0)}; if ( ! index || ! v ) return 0; - if ( ! tv->ExpandAndInit(index, v) ) + if ( ! tv->ExpandAndInit(std::move(index), std::move(v)) ) { - Unref(index); Unref(tv); return 0; } - Unref(index); return tv; } @@ -4899,14 +4897,12 @@ Val* ListExpr::AddSetInit(const BroType* t, Val* aggr) const if ( ! element ) return 0; - if ( ! tv->ExpandAndInit(element, 0) ) + if ( ! tv->ExpandAndInit({AdoptRef{}, element}, 0) ) { - Unref(element); Unref(tv); return 0; } - Unref(element); } return tv; diff --git a/src/Val.cc b/src/Val.cc index 4f7fee8333..9d840a8bc5 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -1676,20 +1676,19 @@ bool TableVal::IsSubsetOf(const TableVal* tv) const return true; } -int TableVal::ExpandAndInit(Val* index, Val* new_val) +int TableVal::ExpandAndInit(IntrusivePtr index, IntrusivePtr new_val) { BroType* index_type = index->Type(); if ( index_type->IsSet() ) { - Val* new_index = index->AsTableVal()->ConvertToList(); - Unref(index); - return ExpandAndInit(new_index, new_val); + index = {AdoptRef{}, index->AsTableVal()->ConvertToList()}; + return ExpandAndInit(std::move(index), std::move(new_val)); } if ( index_type->Tag() != TYPE_LIST ) // Nothing to expand. - return CheckAndAssign(index, new_val); + return CheckAndAssign(index.get(), std::move(new_val)); ListVal* iv = index->AsListVal(); if ( iv->BaseTag() != TYPE_ANY ) @@ -1698,10 +1697,9 @@ int TableVal::ExpandAndInit(Val* index, Val* new_val) reporter->InternalError("bad singleton list index"); for ( int i = 0; i < iv->Length(); ++i ) - if ( ! ExpandAndInit(iv->Index(i), new_val ? new_val->Ref() : 0) ) + if ( ! ExpandAndInit({NewRef{}, iv->Index(i)}, new_val) ) return 0; - Unref(new_val); return 1; } @@ -1720,13 +1718,9 @@ int TableVal::ExpandAndInit(Val* index, Val* new_val) if ( i >= vl->length() ) // Nothing to expand. - return CheckAndAssign(index, new_val); + return CheckAndAssign(index.get(), std::move(new_val)); else - { - int result = ExpandCompoundAndInit(vl, i, new_val); - Unref(new_val); - return result; - } + return ExpandCompoundAndInit(vl, i, std::move(new_val)); } } @@ -2191,17 +2185,17 @@ void TableVal::Describe(ODesc* d) const } } -int TableVal::ExpandCompoundAndInit(val_list* vl, int k, Val* new_val) +int TableVal::ExpandCompoundAndInit(val_list* vl, int k, IntrusivePtr new_val) { Val* ind_k_v = (*vl)[k]; - ListVal* ind_k = ind_k_v->Type()->IsSet() ? - ind_k_v->AsTableVal()->ConvertToList() : - ind_k_v->Ref()->AsListVal(); + auto ind_k = ind_k_v->Type()->IsSet() ? + IntrusivePtr{AdoptRef{}, ind_k_v->AsTableVal()->ConvertToList()} : + IntrusivePtr{NewRef{}, ind_k_v->AsListVal()}; for ( int i = 0; i < ind_k->Length(); ++i ) { Val* ind_k_i = ind_k->Index(i); - ListVal* expd = new ListVal(TYPE_ANY); + auto expd = make_intrusive(TYPE_ANY); loop_over_list(*vl, j) { if ( j == k ) @@ -2210,21 +2204,16 @@ int TableVal::ExpandCompoundAndInit(val_list* vl, int k, Val* new_val) expd->Append((*vl)[j]->Ref()); } - int success = ExpandAndInit(expd, new_val ? new_val->Ref() : 0); - Unref(expd); + int success = ExpandAndInit(std::move(expd), new_val); if ( ! success ) - { - Unref(ind_k); return 0; - } } - Unref(ind_k); return 1; } -int TableVal::CheckAndAssign(Val* index, Val* new_val) +int TableVal::CheckAndAssign(Val* index, IntrusivePtr new_val) { Val* v = 0; if ( subnets ) @@ -2236,7 +2225,7 @@ int TableVal::CheckAndAssign(Val* index, Val* new_val) if ( v ) index->Warn("multiple initializations for index"); - return Assign(index, new_val); + return Assign(index, new_val.release()); } void TableVal::InitDefaultFunc(Frame* f) diff --git a/src/Val.h b/src/Val.h index 5bd3e2e49a..0777d79779 100644 --- a/src/Val.h +++ b/src/Val.h @@ -27,6 +27,7 @@ using std::string; #define UDP_PORT_MASK 0x20000 #define ICMP_PORT_MASK 0x30000 +template class IntrusivePtr; template class PDict; class IterCookie; @@ -754,7 +755,7 @@ public: // Expands any lists in the index into multiple initializations. // Returns true if the initializations typecheck, false if not. - int ExpandAndInit(Val* index, Val* new_val); + int ExpandAndInit(IntrusivePtr index, IntrusivePtr new_val); // Returns the element's value if it exists in the table, // nil otherwise. Note, "index" is not const because we @@ -825,8 +826,8 @@ protected: void Init(TableType* t); void CheckExpireAttr(attr_tag at); - int ExpandCompoundAndInit(val_list* vl, int k, Val* new_val); - int CheckAndAssign(Val* index, Val* new_val); + int ExpandCompoundAndInit(val_list* vl, int k, IntrusivePtr new_val); + int CheckAndAssign(Val* index, IntrusivePtr new_val); // Calculates default value for index. Returns 0 if none. Val* Default(Val* index);