OpaqueVal: use class IntrusivePtr

NEWS

@@ -12,6 +12,11 @@ New Functionality
 Changed Functionality
 ---------------------
 
+- Several C++ functions have been changed to pass smart pointers
+  (`class IntrusivePtr<>`) instead of raw pointers.  This makes the
+  code more robust.  External plugins may need to be updated to this
+  API change.
+
 Removed Functionality
 ---------------------
 

src/OpaqueVal.cc

@@ -56,7 +56,7 @@ const std::string& OpaqueMgr::TypeID(const OpaqueVal* v) const
 	return x->first;
 	}
 
-OpaqueVal* OpaqueMgr::Instantiate(const std::string& id) const
+IntrusivePtr<OpaqueVal> OpaqueMgr::Instantiate(const std::string& id) const
 	{
 	auto x = _types.find(id);
 	return x != _types.end() ? (*x->second)() : nullptr;
@@ -73,7 +73,7 @@ broker::expected<broker::data> OpaqueVal::Serialize() const
 	return {broker::vector{std::move(type), std::move(*d)}};
 	}
 
-OpaqueVal* OpaqueVal::Unserialize(const broker::data& data)
+IntrusivePtr<OpaqueVal> OpaqueVal::Unserialize(const broker::data& data)
 	{
 	auto v = caf::get_if<broker::vector>(&data);
 
@@ -90,7 +90,6 @@ OpaqueVal* OpaqueVal::Unserialize(const broker::data& data)
 
 	if ( ! val->DoUnserialize((*v)[1]) )
 		{
-		Unref(val);
 		return nullptr;
 		}
 
@@ -154,7 +153,7 @@ Val* OpaqueVal::DoClone(CloneState* state)
 		return nullptr;
 
 	auto rval = OpaqueVal::Unserialize(std::move(*d));
-	return state->NewClone(this, rval);
+	return state->NewClone(this, rval.release());
 	}
 
 bool HashVal::IsValid() const
@@ -171,12 +170,13 @@ bool HashVal::Init()
 	return valid;
 	}
 
-StringVal* HashVal::Get()
+IntrusivePtr<StringVal> HashVal::Get()
 	{
 	if ( ! valid )
-		return val_mgr->GetEmptyString();
+		return IntrusivePtr<StringVal>(AdoptRef{},
+					       val_mgr->GetEmptyString());
 
-	StringVal* result = DoGet();
+	auto result = DoGet();
 	valid = false;
 	return result;
 	}
@@ -202,10 +202,10 @@ bool HashVal::DoFeed(const void*, size_t)
 	return false;
 	}
 
-StringVal* HashVal::DoGet()
+IntrusivePtr<StringVal> HashVal::DoGet()
 	{
 	assert(! "missing implementation of DoGet()");
-	return val_mgr->GetEmptyString();
+	return IntrusivePtr<StringVal>(AdoptRef{}, val_mgr->GetEmptyString());
 	}
 
 HashVal::HashVal(OpaqueType* t) : OpaqueVal(t)
@@ -285,14 +285,14 @@ bool MD5Val::DoFeed(const void* data, size_t size)
 	return true;
 	}
 
-StringVal* MD5Val::DoGet()
+IntrusivePtr<StringVal> MD5Val::DoGet()
 	{
 	if ( ! IsValid() )
-		return val_mgr->GetEmptyString();
+		return IntrusivePtr<StringVal>(AdoptRef{}, val_mgr->GetEmptyString());
 
 	u_char digest[MD5_DIGEST_LENGTH];
 	hash_final(ctx, digest);
-	return new StringVal(md5_digest_print(digest));
+	return make_intrusive<StringVal>(md5_digest_print(digest));
 	}
 
 IMPLEMENT_OPAQUE_VALUE(MD5Val)
@@ -425,14 +425,15 @@ bool SHA1Val::DoFeed(const void* data, size_t size)
 	return true;
 	}
 
-StringVal* SHA1Val::DoGet()
+IntrusivePtr<StringVal> SHA1Val::DoGet()
 	{
 	if ( ! IsValid() )
-		return val_mgr->GetEmptyString();
+		return IntrusivePtr<StringVal>(AdoptRef{},
+					       val_mgr->GetEmptyString());
 
 	u_char digest[SHA_DIGEST_LENGTH];
 	hash_final(ctx, digest);
-	return new StringVal(sha1_digest_print(digest));
+	return make_intrusive<StringVal>(sha1_digest_print(digest));
 	}
 
 IMPLEMENT_OPAQUE_VALUE(SHA1Val)
@@ -568,14 +569,15 @@ bool SHA256Val::DoFeed(const void* data, size_t size)
 	return true;
 	}
 
-StringVal* SHA256Val::DoGet()
+IntrusivePtr<StringVal> SHA256Val::DoGet()
 	{
 	if ( ! IsValid() )
-		return val_mgr->GetEmptyString();
+		return IntrusivePtr<StringVal>(AdoptRef{},
+					       val_mgr->GetEmptyString());
 
 	u_char digest[SHA256_DIGEST_LENGTH];
 	hash_final(ctx, digest);
-	return new StringVal(sha256_digest_print(digest));
+	return make_intrusive<StringVal>(sha256_digest_print(digest));
 	}
 
 IMPLEMENT_OPAQUE_VALUE(SHA256Val)
@@ -833,7 +835,7 @@ string BloomFilterVal::InternalState() const
 	return bloom_filter->InternalState();
 	}
 
-BloomFilterVal* BloomFilterVal::Merge(const BloomFilterVal* x,
+IntrusivePtr<BloomFilterVal> BloomFilterVal::Merge(const BloomFilterVal* x,
 					     const BloomFilterVal* y)
 	{
 	if ( x->Type() && // any one 0 is ok here
@@ -859,11 +861,10 @@ BloomFilterVal* BloomFilterVal::Merge(const BloomFilterVal* x,
 		return 0;
 		}
 
-	BloomFilterVal* merged = new BloomFilterVal(copy);
+	auto merged = make_intrusive<BloomFilterVal>(copy);
 
 	if ( x->Type() && ! merged->Typify(x->Type()) )
 		{
-		Unref(merged);
 		reporter->Error("failed to set type on merged Bloom filter");
 		return 0;
 		}
@@ -1035,9 +1036,9 @@ ParaglobVal::ParaglobVal(std::unique_ptr<paraglob::Paraglob> p)
 	this->internal_paraglob = std::move(p);
 	}
 
-VectorVal* ParaglobVal::Get(StringVal* &pattern)
+IntrusivePtr<VectorVal> ParaglobVal::Get(StringVal* &pattern)
 	{
-	VectorVal* rval = new VectorVal(internal_type("string_vec")->AsVectorType());
+	auto rval = make_intrusive<VectorVal>(internal_type("string_vec")->AsVectorType());
 	std::string string_pattern (reinterpret_cast<const char*>(pattern->Bytes()), pattern->Len());
 
 	std::vector<std::string> matches = this->internal_paraglob->get(string_pattern);

src/OpaqueVal.h

@@ -2,6 +2,7 @@
 
 #pragma once
 
+#include "IntrusivePtr.h"
 #include "RandTest.h"
 #include "Val.h"
 #include "digest.h"
@@ -20,7 +21,7 @@ class OpaqueVal;
   */
 class OpaqueMgr {
 public:
-	using Factory = OpaqueVal* ();
+	using Factory = IntrusivePtr<OpaqueVal> ();
 
 	/**
 	 * Return's a unique ID for the type of an opaque value.
@@ -44,7 +45,7 @@ public:
 	 * is unknown, this will return null.
 	 *
 	 */
-	OpaqueVal* Instantiate(const std::string& id) const;
+	IntrusivePtr<OpaqueVal> Instantiate(const std::string& id) const;
 
 	/** Returns the global manager singleton object. */
 	static OpaqueMgr* mgr();
@@ -67,10 +68,11 @@ private:
 /** Macro to insert into an OpaqueVal-derived class's declaration. */
 #define DECLARE_OPAQUE_VALUE(T)                            \
     friend class OpaqueMgr::Register<T>;                   \
+    friend IntrusivePtr<T> make_intrusive<T>();            \
     broker::expected<broker::data> DoSerialize() const override;             \
     bool DoUnserialize(const broker::data& data) override; \
     const char* OpaqueName() const override { return #T; } \
-    static OpaqueVal* OpaqueInstantiate() { return new T(); }
+    static IntrusivePtr<OpaqueVal> OpaqueInstantiate() { return make_intrusive<T>(); }
 
 #define __OPAQUE_MERGE(a, b) a ## b
 #define __OPAQUE_ID(x) __OPAQUE_MERGE(_opaque, x)
@@ -102,7 +104,7 @@ public:
 	 * @param data Broker representation as returned by *Serialize()*.
 	 * @return unserialized instances with reference count at +1
 	 */
-	static OpaqueVal* Unserialize(const broker::data& data);
+	static IntrusivePtr<OpaqueVal> Unserialize(const broker::data& data);
 
 protected:
 	friend class Val;
@@ -163,7 +165,7 @@ public:
 	bool IsValid() const;
 	bool Init();
 	bool Feed(const void* data, size_t size);
-	StringVal* Get();
+	IntrusivePtr<StringVal> Get();
 
 protected:
 	HashVal()	{ valid = false; }
@@ -171,7 +173,7 @@ protected:
 
 	virtual bool DoInit();
 	virtual bool DoFeed(const void* data, size_t size);
-	virtual StringVal* DoGet();
+	virtual IntrusivePtr<StringVal> DoGet();
 
 private:
 	// This flag exists because Get() can only be called once.
@@ -196,7 +198,7 @@ protected:
 
 	bool DoInit() override;
 	bool DoFeed(const void* data, size_t size) override;
-	StringVal* DoGet() override;
+	IntrusivePtr<StringVal> DoGet() override;
 
 	DECLARE_OPAQUE_VALUE(MD5Val)
 private:
@@ -217,7 +219,7 @@ protected:
 
 	bool DoInit() override;
 	bool DoFeed(const void* data, size_t size) override;
-	StringVal* DoGet() override;
+	IntrusivePtr<StringVal> DoGet() override;
 
 	DECLARE_OPAQUE_VALUE(SHA1Val)
 private:
@@ -238,7 +240,7 @@ protected:
 
 	bool DoInit() override;
 	bool DoFeed(const void* data, size_t size) override;
-	StringVal* DoGet() override;
+	IntrusivePtr<StringVal> DoGet() override;
 
 	DECLARE_OPAQUE_VALUE(SHA256Val)
 private:
@@ -277,7 +279,7 @@ public:
 	bool Empty() const;
 	string InternalState() const;
 
-	static BloomFilterVal* Merge(const BloomFilterVal* x,
+	static IntrusivePtr<BloomFilterVal> Merge(const BloomFilterVal* x,
 						  const BloomFilterVal* y);
 
 protected:
@@ -324,7 +326,7 @@ private:
 class ParaglobVal : public OpaqueVal {
 public:
 	explicit ParaglobVal(std::unique_ptr<paraglob::Paraglob> p);
-	VectorVal* Get(StringVal* &pattern);
+	IntrusivePtr<VectorVal> Get(StringVal* &pattern);
 	Val* DoClone(CloneState* state) override;
 	bool operator==(const ParaglobVal& other) const;
 

src/broker/Data.cc

@@ -442,7 +442,7 @@ struct val_converter {
 			return rval;
 			}
 		else if ( type->Tag() == TYPE_OPAQUE )
-			return OpaqueVal::Unserialize(a);
+			return OpaqueVal::Unserialize(a).release();
 
 		return nullptr;
 		}
@@ -772,9 +772,7 @@ struct type_checker {
 			// TODO: Could avoid doing the full unserialization here
 			// and just check if the type is a correct match.
 			auto ov = OpaqueVal::Unserialize(a);
-			auto rval = ov != nullptr;
-			Unref(ov);
-			return rval;
+			return ov != nullptr;
 			}
 
 		return false;

src/broker/data.bif

@@ -52,7 +52,7 @@ function Broker::__opaque_clone_through_serialization%(d: any%): any
         	return val_mgr->GetFalse();
 		}
 	
-	return OpaqueVal::Unserialize(std::move(*x));
+	return OpaqueVal::Unserialize(std::move(*x)).release();
 	%}
 
 function Broker::__set_create%(%): Broker::Data

src/file_analysis/analyzer/hash/Hash.cc

@@ -54,6 +54,6 @@ void Hash::Finalize()
 	mgr.QueueEventFast(file_hash, {
 		GetFile()->GetVal()->Ref(),
 		new StringVal(kind),
-		hash->Get(),
+		hash->Get().release(),
 	});
 	}

src/probabilistic/bloom-filter.bif

@@ -233,7 +233,7 @@ function bloomfilter_merge%(bf1: opaque of bloomfilter,
 		return 0;
 		}
 
-	return BloomFilterVal::Merge(bfv1, bfv2);
+	return BloomFilterVal::Merge(bfv1, bfv2).release();
 	%}
 
 ## Returns a string with a representation of a Bloom filter's internal

src/zeek.bif

@@ -762,7 +762,7 @@ function sha256_hash_update%(handle: opaque of sha256, data: string%): bool
 ##    sha256_hash sha256_hash_init sha256_hash_update sha256_hash_finish
 function md5_hash_finish%(handle: opaque of md5%): string
 	%{
-	return static_cast<HashVal*>(handle)->Get();
+	return static_cast<HashVal*>(handle)->Get().release();
 	%}
 
 ## Returns the final SHA1 digest of an incremental hash computation.
@@ -776,7 +776,7 @@ function md5_hash_finish%(handle: opaque of md5%): string
 ##    sha256_hash sha256_hash_init sha256_hash_update sha256_hash_finish
 function sha1_hash_finish%(handle: opaque of sha1%): string
 	%{
-	return static_cast<HashVal*>(handle)->Get();
+	return static_cast<HashVal*>(handle)->Get().release();
 	%}
 
 ## Returns the final SHA256 digest of an incremental hash computation.
@@ -790,7 +790,7 @@ function sha1_hash_finish%(handle: opaque of sha1%): string
 ##    sha256_hash sha256_hash_init sha256_hash_update
 function sha256_hash_finish%(handle: opaque of sha256%): string
 	%{
-	return static_cast<HashVal*>(handle)->Get();
+	return static_cast<HashVal*>(handle)->Get().release();
 	%}
 
 ## Initializes and returns a new paraglob.
@@ -842,7 +842,7 @@ function paraglob_init%(v: any%) : opaque of paraglob
 ## ## .. zeek:see::paraglob_add paraglob_equals paraglob_init
 function paraglob_match%(handle: opaque of paraglob, match: string%): string_vec
 	%{
-	return static_cast<ParaglobVal*>(handle)->Get(match);
+	return static_cast<ParaglobVal*>(handle)->Get(match).release();
 	%}
 
 ## Compares two paraglobs for equality.