Add and use new IntrusivePt type in Zeek

Manual memory management via Ref/Unref is verbose and prone to error. An
intrusive smart pointer automates the reference counting, makes code
more robust (in particular w.r.t. to exceptions) and reduces boilerplate
code. A big benefit of the intrusive smart pointers for Zeek is that
they can co-exist with the manual memory management. Rather than having
to port the entire code base at once, we can migrate components
one-by-one. In this first step, we add the new template
`IntrusivePtr<T>` and start using it in the Broker Manager. This makes
the previous `unref_guard` obsolete.

src/Frame.cc

@@ -403,14 +403,14 @@ std::pair<bool, Frame*> Frame::Unserialize(const broker::vector& data)
 		broker::integer g = *has_type;
 		BroType t( static_cast<TypeTag>(g) );
 
-		Val* val = bro_broker::data_to_val(std::move(val_tuple[0]), &t);
+		auto val = bro_broker::data_to_val(std::move(val_tuple[0]), &t);
 		if ( ! val )
 			{
 			Unref(rf);
 			return std::make_pair(false, nullptr);
 			}
 
-		rf->frame[i] = val;
+		rf->frame[i] = val.release();
 		}
 
 	return std::make_pair(true, rf);

src/IntrusivePtr.h

@@ -0,0 +1,225 @@
+// See the file "COPYING" in the main distribution directory for copyright.
+
+#pragma once
+
+#include <type_traits>
+#include <utility>
+
+// These forward declarations only exist to enable ADL for the Ref and Unref
+// functions.
+
+struct IntrusivePtrDummy;
+
+void Ref(IntrusivePtrDummy*);
+
+void Unref(IntrusivePtrDummy*);
+
+// An intrusive, reference counting smart pointer implementation.
+template <class T>
+class IntrusivePtr {
+public:
+	// -- member types
+
+	using pointer = T*;
+
+	using const_pointer = const T*;
+
+	using element_type = T;
+
+	using reference = T&;
+
+	using const_reference = const T&;
+
+	// -- constructors, destructors, and assignment operators
+
+	constexpr IntrusivePtr() noexcept : ptr_(nullptr)
+		{
+		// nop
+		}
+
+	constexpr IntrusivePtr(std::nullptr_t) noexcept : IntrusivePtr()
+		{
+		// nop
+		}
+
+	IntrusivePtr(pointer raw_ptr, bool add_ref) noexcept
+		{
+		setPtr(raw_ptr, add_ref);
+		}
+
+	IntrusivePtr(IntrusivePtr&& other) noexcept : ptr_(other.detach())
+		{
+		// nop
+		}
+
+	IntrusivePtr(const IntrusivePtr& other) noexcept
+		{
+		setPtr(other.get(), true);
+		}
+
+	template <class U, class = std::enable_if_t<std::is_convertible_v<U*, T*>>>
+	IntrusivePtr(IntrusivePtr<U> other) noexcept : ptr_(other.detach())
+		{
+		// nop
+		}
+
+	~IntrusivePtr()
+		{
+		if (ptr_)
+			Unref(ptr_);
+		}
+
+	void swap(IntrusivePtr& other) noexcept
+		{
+		std::swap(ptr_, other.ptr_);
+		}
+
+	// Returns the raw pointer without modifying the reference count and sets
+	// this to `nullptr`.
+	pointer release() noexcept
+		{
+		auto result = ptr_;
+		if (result)
+			ptr_ = nullptr;
+		return result;
+		}
+
+	void reset(pointer new_value = nullptr, bool add_ref = true) noexcept
+		{
+		auto old = ptr_;
+		setPtr(new_value, add_ref);
+		if (old)
+			Unref(old);
+		}
+
+	IntrusivePtr& operator=(IntrusivePtr other) noexcept
+		{
+		swap(other);
+		return *this;
+		}
+
+	pointer get() const noexcept
+		{
+		return ptr_;
+		}
+
+	pointer operator->() const noexcept
+		{
+		return ptr_;
+		}
+
+	reference operator*() const noexcept
+		{
+		return *ptr_;
+		}
+
+	bool operator!() const noexcept
+		{
+		return !ptr_;
+		}
+
+	explicit operator bool() const noexcept
+		{
+		return ptr_ != nullptr;
+		}
+
+private:
+	void setPtr(pointer raw_ptr, bool add_ref) noexcept
+		{
+		ptr_ = raw_ptr;
+		if ( raw_ptr && add_ref )
+			Ref(raw_ptr);
+		}
+
+	pointer ptr_;
+};
+
+// Convenience function for creating intrusive pointers.
+template <class T, class... Ts>
+IntrusivePtr<T> makeCounted(Ts&&... args)
+	{
+	// Assumes that objects start with a reference count of 1!
+	return {new T(std::forward<Ts>(args)...), false};
+	}
+
+// -- comparison to nullptr ----------------------------------------------------
+
+template <class T>
+bool operator==(const IntrusivePtr<T>& x, std::nullptr_t) {
+  return !x;
+}
+
+template <class T>
+bool operator==(std::nullptr_t, const IntrusivePtr<T>& x) {
+  return !x;
+}
+
+template <class T>
+bool operator!=(const IntrusivePtr<T>& x, std::nullptr_t) {
+  return static_cast<bool>(x);
+}
+
+template <class T>
+bool operator!=(std::nullptr_t, const IntrusivePtr<T>& x) {
+  return static_cast<bool>(x);
+}
+
+// -- comparison to raw pointer ------------------------------------------------
+
+template <class T>
+bool operator==(const IntrusivePtr<T>& x, const T* y) {
+  return x.get() == y;
+}
+
+template <class T>
+bool operator==(const T* x, const IntrusivePtr<T>& y) {
+  return x == y.get();
+}
+
+template <class T>
+bool operator!=(const IntrusivePtr<T>& x, const T* y) {
+  return x.get() != y;
+}
+
+template <class T>
+bool operator!=(const T* x, const IntrusivePtr<T>& y) {
+  return x != y.get();
+}
+
+template <class T>
+bool operator<(const IntrusivePtr<T>& x, const T* y)
+	{
+	return x.get() < y;
+	}
+
+template <class T>
+bool operator<(const T* x, const IntrusivePtr<T>& y)
+	{
+	return x < y.get();
+	}
+
+// -- comparison to intrusive pointer ------------------------------------------
+
+// Using trailing return type and decltype() here removes this function from
+// overload resolution if the two pointers types are not comparable (SFINAE).
+template <class T, class U>
+auto operator==(const IntrusivePtr<T>& x, const IntrusivePtr<U>& y)
+-> decltype(x.get() == y.get())
+	{
+	return x.get() == y.get();
+	}
+
+template <class T, class U>
+auto operator!=(const IntrusivePtr<T>& x, const IntrusivePtr<U>& y)
+-> decltype(x.get() != y.get())
+	{
+	return x.get() != y.get();
+	}
+
+template <class T>
+auto operator<(const IntrusivePtr<T>& x, const IntrusivePtr<T>& y)
+-> decltype(x.get() < y.get())
+	{
+	return x.get() < y.get();
+	}
+

src/Val.cc

@@ -3159,7 +3159,7 @@ Val* cast_value_to_type(Val* v, BroType* t)
 		if ( ! dv )
 			return 0;
 
-		return static_cast<bro_broker::DataVal *>(dv)->castTo(t);
+		return static_cast<bro_broker::DataVal *>(dv)->castTo(t).release();
 		}
 
 	return 0;

src/broker/Data.cc

@@ -181,7 +181,7 @@ struct val_converter {
 			return nullptr;
 
 		auto tt = type->AsTableType();
-		auto rval = new TableVal(tt);
+		auto rval = makeCounted<TableVal>(tt);
 
 		for ( auto& item : a )
 			{
@@ -214,11 +214,10 @@ struct val_converter {
 			if ( static_cast<size_t>(expected_index_types->length()) !=
 			     indices->size() )
 				{
-				Unref(rval);
 				return nullptr;
 				}
 
-			auto list_val = new ListVal(TYPE_ANY);
+			auto list_val = makeCounted<ListVal>(TYPE_ANY);
 
 			for ( auto i = 0u; i < indices->size(); ++i )
 				{
@@ -227,20 +226,17 @@ struct val_converter {
 
 				if ( ! index_val )
 					{
-					Unref(rval);
-					Unref(list_val);
 					return nullptr;
 					}
 
-				list_val->Append(index_val);
+				list_val->Append(index_val.release());
 				}
 
 
-			rval->Assign(list_val, nullptr);
+			rval->Assign(list_val.get(), nullptr);
-			Unref(list_val);
 			}
 
-		return rval;
+		return rval.release();
 		}
 
 	result_type operator()(broker::table& a)
@@ -249,7 +245,7 @@ struct val_converter {
 			return nullptr;
 
 		auto tt = type->AsTableType();
-		auto rval = new TableVal(tt);
+		auto rval = makeCounted<TableVal>(tt);
 
 		for ( auto& item : a )
 			{
@@ -282,11 +278,10 @@ struct val_converter {
 			if ( static_cast<size_t>(expected_index_types->length()) !=
 			     indices->size() )
 				{
-				Unref(rval);
 				return nullptr;
 				}
 
-			auto list_val = new ListVal(TYPE_ANY);
+			auto list_val = makeCounted<ListVal>(TYPE_ANY);
 
 			for ( auto i = 0u; i < indices->size(); ++i )
 				{
@@ -295,12 +290,10 @@ struct val_converter {
 
 				if ( ! index_val )
 					{
-					Unref(rval);
-					Unref(list_val);
 					return nullptr;
 					}
 
-				list_val->Append(index_val);
+				list_val->Append(index_val.release());
 				}
 
 			auto value_val = bro_broker::data_to_val(move(item.second),
@@ -308,16 +301,13 @@ struct val_converter {
 
 			if ( ! value_val )
 				{
-				Unref(rval);
-				Unref(list_val);
 				return nullptr;
 				}
 
-			rval->Assign(list_val, value_val);
+			rval->Assign(list_val.get(), value_val.release());
-			Unref(list_val);
 			}
 
-		return rval;
+		return rval.release();
 		}
 
 	result_type operator()(broker::vector& a)
@@ -325,7 +315,7 @@ struct val_converter {
 		if ( type->Tag() == TYPE_VECTOR )
 			{
 			auto vt = type->AsVectorType();
-			auto rval = new VectorVal(vt);
+			auto rval = makeCounted<VectorVal>(vt);
 
 			for ( auto& item : a )
 				{
@@ -333,14 +323,13 @@ struct val_converter {
 
 				if ( ! item_val )
 					{
-					Unref(rval);
 					return nullptr;
 					}
 
-				rval->Assign(rval->Size(), item_val);
+				rval->Assign(rval->Size(), item_val.release());
 				}
 
-			return rval;
+			return rval.release();
 			}
 		else if ( type->Tag() == TYPE_FUNC )
 			{
@@ -385,14 +374,13 @@ struct val_converter {
 		else if ( type->Tag() == TYPE_RECORD )
 			{
 			auto rt = type->AsRecordType();
-			auto rval = new RecordVal(rt);
+			auto rval = makeCounted<RecordVal>(rt);
 			auto idx = 0u;
 
 			for ( auto i = 0u; i < static_cast<size_t>(rt->NumFields()); ++i )
 				{
 				if ( idx >= a.size() )
 					{
-					Unref(rval);
 					return nullptr;
 					}
 
@@ -404,19 +392,18 @@ struct val_converter {
 					}
 
 				auto item_val = bro_broker::data_to_val(move(a[idx]),
-																								rt->FieldType(i));
+				                                        rt->FieldType(i));
 
 				if ( ! item_val )
 					{
-					Unref(rval);
 					return nullptr;
 					}
 
-				rval->Assign(i, item_val);
+				rval->Assign(i, item_val.release());
 				++idx;
 				}
 
-			return rval;
+			return rval.release();
 			}
 		else if ( type->Tag() == TYPE_PATTERN )
 			{
@@ -791,12 +778,12 @@ static bool data_type_check(const broker::data& d, BroType* t)
 	return caf::visit(type_checker{t}, d);
 	}
 
-Val* bro_broker::data_to_val(broker::data d, BroType* type)
+IntrusivePtr<Val> bro_broker::data_to_val(broker::data d, BroType* type)
 	{
 	if ( type->Tag() == TYPE_ANY )
-		return bro_broker::make_data_val(move(d));
+		return {bro_broker::make_data_val(move(d)), false};
 
-	return caf::visit(val_converter{type}, std::move(d));
+	return {caf::visit(val_converter{type}, std::move(d)), false};
 	}
 
 broker::expected<broker::data> bro_broker::val_to_data(Val* v)
@@ -1161,7 +1148,7 @@ bool bro_broker::DataVal::canCastTo(BroType* t) const
 	return data_type_check(data, t);
 	}
 
-Val* bro_broker::DataVal::castTo(BroType* t)
+IntrusivePtr<Val> bro_broker::DataVal::castTo(BroType* t)
 	{
 	return data_to_val(data, t);
 	}

src/broker/Data.h

@@ -7,6 +7,7 @@
 #include "Reporter.h"
 #include "Frame.h"
 #include "Expr.h"
+#include "IntrusivePtr.h"
 
 namespace bro_broker {
 
@@ -58,7 +59,7 @@ broker::expected<broker::data> val_to_data(Val* v);
  * @return a pointer to a new Bro value or a nullptr if the conversion was not
  * possible.
  */
-Val* data_to_val(broker::data d, BroType* type);
+IntrusivePtr<Val> data_to_val(broker::data d, BroType* type);
 
 /**
  * Convert a Bro threading::Value to a Broker data value.
@@ -107,7 +108,7 @@ public:
 		d->Add("}");
 		}
 
-	Val* castTo(BroType* t);
+	IntrusivePtr<Val> castTo(BroType* t);
 	bool canCastTo(BroType* t) const;
 
 	// Returns the Bro type that scripts use to represent a Broker data
@@ -181,9 +182,9 @@ struct type_name_getter {
 		{ return "table"; }
 
 	result_type operator()(const broker::vector&)
-		{ 
+		{
 		assert(tag == TYPE_VECTOR || tag == TYPE_RECORD);
-	 	return tag == TYPE_VECTOR ? "vector" : "record";
+		return tag == TYPE_VECTOR ? "vector" : "record";
 		}
 
 	TypeTag tag;

src/broker/Manager.cc

@@ -65,12 +65,6 @@ const broker::endpoint_info Manager::NoPeer{{}, {}};
 
 int Manager::script_scope = 0;
 
-struct unref_guard {
-	unref_guard(Val* v) : val(v) {}
-	~unref_guard() { Unref(val); }
-	Val* val;
-};
-
 struct scoped_reporter_location {
 	scoped_reporter_location(Frame* frame)
 		{
@@ -1039,7 +1033,7 @@ void Manager::ProcessEvent(const broker::topic& topic, broker::zeek::Event ev)
 		auto val = data_to_val(std::move(args[i]), expected_type);
 
 		if ( val )
-			vl.push_back(val);
+			vl.push_back(val.release());
 		else
 			{
 			auto expected_name = type_name(expected_type->Tag());
@@ -1086,8 +1080,6 @@ bool bro_broker::Manager::ProcessLogCreate(broker::zeek::LogCreate lc)
 		return false;
 		}
 
-	unref_guard stream_id_unreffer{stream_id};
-
 	auto writer_id = data_to_val(std::move(lc.writer_id()), writer_id_type);
 	if ( ! writer_id )
 		{
@@ -1095,8 +1087,6 @@ bool bro_broker::Manager::ProcessLogCreate(broker::zeek::LogCreate lc)
 		return false;
 		}
 
-	unref_guard writer_id_unreffer{writer_id};
-
 	auto writer_info = std::unique_ptr<logging::WriterBackend::WriterInfo>(new logging::WriterBackend::WriterInfo);
 	if ( ! writer_info->FromBroker(std::move(lc.writer_info())) )
 		{
@@ -1163,8 +1153,6 @@ bool bro_broker::Manager::ProcessLogWrite(broker::zeek::LogWrite lw)
 		return false;
 		}
 
-	unref_guard stream_id_unreffer{stream_id};
-
 	// Get writer ID.
 	auto writer_id = data_to_val(std::move(lw.writer_id()), writer_id_type);
 	if ( ! writer_id )
@@ -1173,7 +1161,6 @@ bool bro_broker::Manager::ProcessLogWrite(broker::zeek::LogWrite lw)
 		return false;
 		}
 
-	unref_guard writer_id_unreffer{writer_id};
 	auto path = caf::get_if<std::string>(&lw.path());
 
 	if ( ! path )
@@ -1258,7 +1245,7 @@ bool Manager::ProcessIdentifierUpdate(broker::zeek::IdentifierUpdate iu)
 		return false;
 		}
 
-	id->SetVal(val);
+	id->SetVal(val.release());
 	return true;
 	}
 

src/option.bif

@@ -90,8 +90,7 @@ function Option::set%(ID: string, val: any, location: string &default=""%): bool
 			return val_mgr->GetBool(0);
 			}
 
-		auto rval = call_option_handlers_and_set_value(ID, i, val_from_data, location);
+		auto rval = call_option_handlers_and_set_value(ID, i, val_from_data.get(), location);
-		Unref(val_from_data);
 		return val_mgr->GetBool(rval);
 		}
 

src/probabilistic/Topk.cc

@@ -505,14 +505,14 @@ bool TopkVal::DoUnserialize(const broker::data& data)
 		for ( uint64_t j = 0; j < *elements_count; j++ )
 			{
 			auto epsilon = caf::get_if<uint64_t>(&(*v)[idx++]);
-			Val* val = bro_broker::data_to_val((*v)[idx++], type);
+			auto val = bro_broker::data_to_val((*v)[idx++], type);
 
 			if ( ! (epsilon && val) )
 				return false;
 
 			Element* e = new Element();
 			e->epsilon = *epsilon;
-			e->value = val;
+			e->value = val.release();
 			e->parent = b;
 
 			b->elements.insert(b->elements.end(), e);