Merge remote-tracking branch 'origin/topic/neverlord/intrusive-ptr'

* origin/topic/neverlord/intrusive-ptr:
  Add API documentation to IntrusivePtr
  Rename make{ Counted => _intrusive }
  Remove braces around single return statements
  Integrate review feedback
  Add and use new IntrusivePt type in Zeek

CHANGES

@@ -1,4 +1,18 @@
 
+3.1.0-dev.253 | 2019-11-05 09:54:01 -0800
+
+  * Add and use new IntrusivePtr type (Dominik Charousset, Corelight)
+
+    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.
+
 3.1.0-dev.247 | 2019-11-04 17:26:32 -0800
 
   * Cluster-ize FTP data channel analysis (Jeff Barber)

VERSION

@@ -1 +1 @@
-3.1.0-dev.247
+3.1.0-dev.253

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.detach();
 		}
 
 	return std::make_pair(true, rf);

src/IntrusivePtr.h

@@ -0,0 +1,301 @@
+// See the file "COPYING" in the main distribution directory for copyright.
+
+#pragma once
+
+#include <type_traits>
+#include <utility>
+
+/**
+ * An intrusive, reference counting smart pointer implementation. Much like
+ * @c std::shared_ptr, this smart pointer models shared ownership of an object
+ * through a pointer. Several @c IntrusivePtr instances may point to the same
+ * object.
+ *
+ * The @c IntrusivePtr requires two free functions associated to @c T that must
+ * be available via argument-dependent lookup: @c Ref and @c Unref. The former
+ * increments the reference by one whenever a new owner participates in the
+ * lifetime of the shared object and the latter decrements the reference count
+ * by one. Once the reference count reaches zero, @c Unref also is responsible
+ * for destroying the shared object.
+ *
+ * The @c IntrusivePtr works with any type that offers the two free functions,
+ * but most notably is designed to work with @c BroObj and its subtypes.
+ *
+ * The same object may get managed via @c IntrusivePtr in one part of the
+ * code base while another part of the program manages it manually by passing
+ * raw pointers and calling @c Ref and @c Unref explicitly. However, new code
+ * should use a smart pointer whenever possible to reduce boilerplate code and
+ * increase robustness of the code (in particular w.r.t. exceptions).
+ */
+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
+		}
+
+	/**
+	 * Constructs a new intrusive pointer for managing the lifetime of the object
+	 * pointed to by @c raw_ptr.
+	 * @param raw_ptr Pointer to the shared object.
+	 * @param add_ref Denotes whether the reference count of the object shall be
+	 *                increased during construction.
+	 */
+	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_);
+		}
+
+	/**
+	 * Detaches an object from the automated lifetime management and sets this
+	 * intrusive pointer to @c nullptr.
+	 * @returns the raw pointer without modifying the reference count.
+	 */
+	pointer detach() noexcept
+		{
+		auto result = ptr_;
+		if ( result )
+			ptr_ = nullptr;
+		return result;
+		}
+
+	/**
+	 * Convenience function for assigning a new raw pointer. Equivalent to calling
+	 * @c operator= with an @c IntrusivePtr constructed from the arguments.
+	 * @param new_value Pointer to the new shared object.
+	 * @param add_ref Denotes whether the reference count of the new shared object
+	 *                shall be increased.
+	 */
+	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 a reference counted object and wrapping it
+ * into an intrusive pointers.
+ * @param args Arguments for constructing the shared object of type @c T.
+ * @returns an @c IntrusivePtr pointing to the new object.
+ * @note This function assumes that any @c T starts with a reference count of 1.
+ * @relates IntrusivePtr
+ */
+template <class T, class... Ts>
+IntrusivePtr<T> make_intrusive(Ts&&... args)
+	{
+	// Assumes that objects start with a reference count of 1!
+	return {new T(std::forward<Ts>(args)...), false};
+	}
+
+// -- comparison to nullptr ----------------------------------------------------
+
+/**
+ * @relates IntrusivePtr
+ */
+template <class T>
+bool operator==(const IntrusivePtr<T>& x, std::nullptr_t) {
+  return !x;
+}
+
+/**
+ * @relates IntrusivePtr
+ */
+template <class T>
+bool operator==(std::nullptr_t, const IntrusivePtr<T>& x) {
+  return !x;
+}
+
+/**
+ * @relates IntrusivePtr
+ */
+template <class T>
+bool operator!=(const IntrusivePtr<T>& x, std::nullptr_t) {
+  return static_cast<bool>(x);
+}
+
+/**
+ * @relates IntrusivePtr
+ */
+template <class T>
+bool operator!=(std::nullptr_t, const IntrusivePtr<T>& x) {
+  return static_cast<bool>(x);
+}
+
+// -- comparison to raw pointer ------------------------------------------------
+
+/**
+ * @relates IntrusivePtr
+ */
+template <class T>
+bool operator==(const IntrusivePtr<T>& x, const T* y) {
+  return x.get() == y;
+}
+
+/**
+ * @relates IntrusivePtr
+ */
+template <class T>
+bool operator==(const T* x, const IntrusivePtr<T>& y) {
+  return x == y.get();
+}
+
+/**
+ * @relates IntrusivePtr
+ */
+template <class T>
+bool operator!=(const IntrusivePtr<T>& x, const T* y) {
+  return x.get() != y;
+}
+
+/**
+ * @relates IntrusivePtr
+ */
+template <class T>
+bool operator!=(const T* x, const IntrusivePtr<T>& y) {
+  return x != y.get();
+}
+
+/**
+ * @relates IntrusivePtr
+ */
+template <class T>
+bool operator<(const IntrusivePtr<T>& x, const T* y)
+	{
+	return x.get() < y;
+	}
+
+/**
+ * @relates IntrusivePtr
+ */
+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).
+
+/**
+ * @relates IntrusivePtr
+ */
+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();
+	}
+
+/**
+ * @relates IntrusivePtr
+ */
+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();
+	}
+
+/**
+ * @relates IntrusivePtr
+ */
+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).detach();
 		}
 
 	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 = make_intrusive<TableVal>(tt);
 
 		for ( auto& item : a )
 			{
@@ -213,12 +213,9 @@ 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 = make_intrusive<ListVal>(TYPE_ANY);
 
 			for ( auto i = 0u; i < indices->size(); ++i )
 				{
@@ -226,21 +223,16 @@ struct val_converter {
 				                                         (*expected_index_types)[i]);
 
 				if ( ! index_val )
-					{
-					Unref(rval);
-					Unref(list_val);
 					return nullptr;
-					}
 
-				list_val->Append(index_val);
+				list_val->Append(index_val.detach());
 				}
 
 
-			rval->Assign(list_val, nullptr);
-			Unref(list_val);
+			rval->Assign(list_val.get(), nullptr);
 			}
 
-		return rval;
+		return rval.detach();
 		}
 
 	result_type operator()(broker::table& a)
@@ -249,7 +241,7 @@ struct val_converter {
 			return nullptr;
 
 		auto tt = type->AsTableType();
-		auto rval = new TableVal(tt);
+		auto rval = make_intrusive<TableVal>(tt);
 
 		for ( auto& item : a )
 			{
@@ -281,12 +273,9 @@ 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 = make_intrusive<ListVal>(TYPE_ANY);
 
 			for ( auto i = 0u; i < indices->size(); ++i )
 				{
@@ -294,30 +283,21 @@ struct val_converter {
 				                                         (*expected_index_types)[i]);
 
 				if ( ! index_val )
-					{
-					Unref(rval);
-					Unref(list_val);
 					return nullptr;
-					}
 
-				list_val->Append(index_val);
+				list_val->Append(index_val.detach());
 				}
 
 			auto value_val = bro_broker::data_to_val(move(item.second),
 			                                         tt->YieldType());
 
 			if ( ! value_val )
-				{
-				Unref(rval);
-				Unref(list_val);
 				return nullptr;
+
+			rval->Assign(list_val.get(), value_val.detach());
 			}
 
-			rval->Assign(list_val, value_val);
-			Unref(list_val);
-			}
-
-		return rval;
+		return rval.detach();
 		}
 
 	result_type operator()(broker::vector& a)
@@ -325,22 +305,19 @@ struct val_converter {
 		if ( type->Tag() == TYPE_VECTOR )
 			{
 			auto vt = type->AsVectorType();
-			auto rval = new VectorVal(vt);
+			auto rval = make_intrusive<VectorVal>(vt);
 
 			for ( auto& item : a )
 				{
 				auto item_val = bro_broker::data_to_val(move(item), vt->YieldType());
 
 				if ( ! item_val )
-					{
-					Unref(rval);
 					return nullptr;
+
+				rval->Assign(rval->Size(), item_val.detach());
 				}
 
-				rval->Assign(rval->Size(), item_val);
-				}
-
-			return rval;
+			return rval.detach();
 			}
 		else if ( type->Tag() == TYPE_FUNC )
 			{
@@ -385,16 +362,13 @@ struct val_converter {
 		else if ( type->Tag() == TYPE_RECORD )
 			{
 			auto rt = type->AsRecordType();
-			auto rval = new RecordVal(rt);
+			auto rval = make_intrusive<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;
-					}
 
 				if ( caf::get_if<broker::none>(&a[idx]) != nullptr )
 					{
@@ -407,16 +381,13 @@ struct val_converter {
 				                                        rt->FieldType(i));
 
 				if ( ! item_val )
-					{
-					Unref(rval);
 					return nullptr;
-					}
 
-				rval->Assign(i, item_val);
+				rval->Assign(i, item_val.detach());
 				++idx;
 				}
 
-			return rval;
+			return rval.detach();
 			}
 		else if ( type->Tag() == TYPE_PATTERN )
 			{
@@ -791,12 +762,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 +1132,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

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.detach());
 		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.detach());
 	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);
-		Unref(val_from_data);
+		auto rval = call_option_handlers_and_set_value(ID, i, val_from_data.get(), location);
 		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.detach();
 			e->parent = b;
 
 			b->elements.insert(b->elements.end(), e);