Scope: lookup_ID() and install_ID() return IntrusivePtr<ID>

This fixes several memory leaks and double free bugs.

src/Debug.cc

@@ -196,13 +196,12 @@ void get_first_statement(Stmt* list, Stmt*& first, Location& loc)
 static void parse_function_name(vector<ParseLocationRec>& result,
 				ParseLocationRec& plr, const string& s)
 	{ // function name
-	ID* id = lookup_ID(s.c_str(), current_module.c_str());
+	auto id = lookup_ID(s.c_str(), current_module.c_str());
 	if ( ! id )
 		{
 		string fullname = make_full_var_name(current_module.c_str(), s.c_str());
 		debug_msg("Function %s not defined.\n", fullname.c_str());
 		plr.type = plrUnknown;
-		Unref(id);
 		return;
 		}
 
@@ -210,7 +209,6 @@ static void parse_function_name(vector<ParseLocationRec>& result,
 		{
 		debug_msg("Function %s not declared.\n", id->Name());
 		plr.type = plrUnknown;
-		Unref(id);
 		return;
 		}
 
@@ -218,7 +216,6 @@ static void parse_function_name(vector<ParseLocationRec>& result,
 		{
 		debug_msg("Function %s declared but not defined.\n", id->Name());
 		plr.type = plrUnknown;
-		Unref(id);
 		return;
 		}
 
@@ -229,12 +226,9 @@ static void parse_function_name(vector<ParseLocationRec>& result,
 		{
 		debug_msg("Function %s is a built-in function\n", id->Name());
 		plr.type = plrUnknown;
-		Unref(id);
 		return;
 		}
 
-	Unref(id);
-
 	Stmt* body = 0;	// the particular body we care about; 0 = all
 
 	if ( bodies.size() == 1 )

src/EventHandler.cc

@@ -38,8 +38,8 @@ FuncType* EventHandler::FType(bool check_export)
 	if ( type )
 		return type;
 
-	ID* id = lookup_ID(name, current_module.c_str(), false, false,
+	auto id = lookup_ID(name, current_module.c_str(), false, false,
-	                   check_export);
+	                    check_export);
 
 	if ( ! id )
 		return 0;
@@ -48,8 +48,6 @@ FuncType* EventHandler::FType(bool check_export)
 		return 0;
 
 	type = id->Type()->AsFuncType();
-	Unref(id);
-
 	return type;
 	}
 

src/Expr.cc

@@ -4455,7 +4455,7 @@ LambdaExpr::LambdaExpr(std::unique_ptr<function_ingredients> arg_ing,
 		}
 
 	// Install that in the global_scope
-	ID* id = install_ID(my_name.c_str(), current_module.c_str(), true, false);
+	auto id = install_ID(my_name.c_str(), current_module.c_str(), true, false);
 
 	// Update lamb's name
 	dummy_func->SetName(my_name.c_str());

src/Func.cc

@@ -606,7 +606,7 @@ BuiltinFunc::BuiltinFunc(built_in_func arg_func, const char* arg_name,
 	name = make_full_var_name(GLOBAL_MODULE_NAME, arg_name);
 	is_pure = arg_is_pure;
 
-	ID* id = lookup_ID(Name(), GLOBAL_MODULE_NAME, false);
+	auto id = lookup_ID(Name(), GLOBAL_MODULE_NAME, false);
 	if ( ! id )
 		reporter->InternalError("built-in function %s missing", Name());
 	if ( id->HasVal() )
@@ -614,7 +614,6 @@ BuiltinFunc::BuiltinFunc(built_in_func arg_func, const char* arg_name,
 
 	type = id->Type()->Ref();
 	id->SetVal(new Val(this));
-	Unref(id);
 	}
 
 BuiltinFunc::~BuiltinFunc()

src/RuleMatcher.cc

@@ -9,6 +9,7 @@
 #include "RuleCondition.h"
 #include "BroString.h"
 #include "ID.h"
+#include "IntrusivePtr.h"
 #include "IntSet.h"
 #include "IP.h"
 #include "analyzer/Analyzer.h"
@@ -1268,17 +1269,14 @@ void RuleMatcher::DumpStateStats(BroFile* f, RuleHdrTest* hdr_test)
 
 static Val* get_bro_val(const char* label)
 	{
-	ID* id = lookup_ID(label, GLOBAL_MODULE_NAME, false);
+	auto id = lookup_ID(label, GLOBAL_MODULE_NAME, false);
 	if ( ! id )
 		{
 		rules_error("unknown script-level identifier", label);
 		return 0;
 		}
 
-	Val* rval = id->ID_Val();
+	return id->ID_Val();
-	Unref(id);
-
-	return rval;
 	}
 
 

src/Scope.cc

@@ -126,7 +126,7 @@ TraversalCode Scope::Traverse(TraversalCallback* cb) const
 	}
 
 
-ID* lookup_ID(const char* name, const char* curr_module, bool no_global,
+IntrusivePtr<ID> lookup_ID(const char* name, const char* curr_module, bool no_global,
 	      bool same_module_only, bool check_export)
 	{
 	string fullname = make_full_var_name(curr_module, name);
@@ -144,8 +144,7 @@ ID* lookup_ID(const char* name, const char* curr_module, bool no_global,
 				reporter->Error("identifier is not exported: %s",
 				      fullname.c_str());
 
-			Ref(id);
+			return {NewRef{}, id};
-			return id;
 			}
 		}
 
@@ -155,16 +154,13 @@ ID* lookup_ID(const char* name, const char* curr_module, bool no_global,
 		string globalname = make_full_var_name(GLOBAL_MODULE_NAME, name);
 		ID* id = global_scope()->Lookup(globalname);
 		if ( id )
-			{
+			return {NewRef{}, id};
-			Ref(id);
-			return id;
-			}
 		}
 
 	return 0;
 	}
 
-ID* install_ID(const char* name, const char* module_name,
+IntrusivePtr<ID> install_ID(const char* name, const char* module_name,
 		bool is_global, bool is_export)
 	{
 	if ( scopes.empty() && ! is_global )
@@ -182,13 +178,13 @@ ID* install_ID(const char* name, const char* module_name,
 
 	string full_name = make_full_var_name(module_name, name);
 
-	ID* id = new ID(full_name.data(), scope, is_export);
+	auto id = make_intrusive<ID>(full_name.data(), scope, is_export);
 	if ( SCOPE_FUNCTION != scope )
-		global_scope()->Insert(std::move(full_name), id);
+		global_scope()->Insert(std::move(full_name), IntrusivePtr{id}.release());
 	else
 		{
 		id->SetOffset(top_scope->Length());
-		top_scope->Insert(std::move(full_name), id);
+		top_scope->Insert(std::move(full_name), IntrusivePtr{id}.release());
 		}
 
 	return id;

src/Scope.h

@@ -88,10 +88,10 @@ extern bool in_debug;
 
 // If no_global is true, don't search in the default "global" namespace.
 // This passed ownership of a ref'ed ID to the caller.
-extern ID* lookup_ID(const char* name, const char* module,
+extern IntrusivePtr<ID> lookup_ID(const char* name, const char* module,
 		     bool no_global = false, bool same_module_only = false,
 		     bool check_export = true);
-extern ID* install_ID(const char* name, const char* module_name,
+extern IntrusivePtr<ID> install_ID(const char* name, const char* module_name,
 			bool is_global, bool is_export);
 
 extern void push_scope(ID* id, attr_list* attrs);

src/Stmt.cc

@@ -199,7 +199,7 @@ static BroFile* print_stdout = 0;
 
 static IntrusivePtr<EnumVal> lookup_enum_val(const char* module_name, const char* name)
 	{
-	ID* id = lookup_ID(name, module_name);
+	auto id = lookup_ID(name, module_name);
 	assert(id);
 	assert(id->IsEnumConst());
 

src/Type.cc

@@ -1172,7 +1172,7 @@ void EnumType::CheckAndAddName(const string& module_name, const char* name,
 		return;
 		}
 
-	ID* id = lookup_ID(name, module_name.c_str());
+	auto id = lookup_ID(name, module_name.c_str());
 
 	if ( ! id )
 		{
@@ -1183,7 +1183,7 @@ void EnumType::CheckAndAddName(const string& module_name, const char* name,
 		if ( deprecation )
 			id->MakeDeprecated(deprecation);
 
-		zeekygen_mgr->Identifier(id);
+		zeekygen_mgr->Identifier(id.get());
 		}
 	else
 		{
@@ -1195,13 +1195,10 @@ void EnumType::CheckAndAddName(const string& module_name, const char* name,
 		     || (id->HasVal() && val != id->ID_Val()->AsEnum())
 		     || (names.find(fullname) != names.end() && names[fullname] != val) )
 			{
-			Unref(id);
 			reporter->Error("identifier or enumerator value in enumerated type definition already exists");
 			SetError();
 			return;
 			}
-
-		Unref(id);
 		}
 
 	AddNameInternal(module_name, name, val, is_export);

src/Var.cc

@@ -396,13 +396,11 @@ void begin_func(ID* id, const char* module_name, function_flavor flavor,
 	for ( int i = 0; i < num_args; ++i )
 		{
 		TypeDecl* arg_i = args->FieldDecl(i);
-		ID* arg_id = lookup_ID(arg_i->id, module_name);
+		auto arg_id = lookup_ID(arg_i->id, module_name);
 
 		if ( arg_id && ! arg_id->IsGlobal() )
 			arg_id->Error("argument name used twice");
 
-		Unref(arg_id);
-
 		arg_id = install_ID(arg_i->id, module_name, false, false);
 		arg_id->SetType(arg_i->type->Ref());
 		}
@@ -509,14 +507,12 @@ void end_func(IntrusivePtr<Stmt> body)
 
 Val* internal_val(const char* name)
 	{
-	ID* id = lookup_ID(name, GLOBAL_MODULE_NAME);
+	auto id = lookup_ID(name, GLOBAL_MODULE_NAME);
 
 	if ( ! id )
 		reporter->InternalError("internal variable %s missing", name);
 
-	Val* rval = id->ID_Val();
+	return id->ID_Val();
-	Unref(id);
-	return rval;
 	}
 
 id_list gather_outer_ids(Scope* scope, Stmt* body)
@@ -541,24 +537,20 @@ id_list gather_outer_ids(Scope* scope, Stmt* body)
 
 Val* internal_const_val(const char* name)
 	{
-	ID* id = lookup_ID(name, GLOBAL_MODULE_NAME);
+	auto id = lookup_ID(name, GLOBAL_MODULE_NAME);
 	if ( ! id )
 		reporter->InternalError("internal variable %s missing", name);
 
 	if ( ! id->IsConst() )
 		reporter->InternalError("internal variable %s is not constant", name);
 
-	Val* rval = id->ID_Val();
+	return id->ID_Val();
-	Unref(id);
-	return rval;
 	}
 
 Val* opt_internal_val(const char* name)
 	{
-	ID* id = lookup_ID(name, GLOBAL_MODULE_NAME);
+	auto id = lookup_ID(name, GLOBAL_MODULE_NAME);
-	Val* rval = id ? id->ID_Val() : 0;
+	return id ? id->ID_Val() : 0;
-	Unref(id);
-	return rval;
 	}
 
 double opt_internal_double(const char* name)
@@ -593,12 +585,11 @@ TableVal* opt_internal_table(const char* name)
 
 ListVal* internal_list_val(const char* name)
 	{
-	ID* id = lookup_ID(name, GLOBAL_MODULE_NAME);
+	auto id = lookup_ID(name, GLOBAL_MODULE_NAME);
 	if ( ! id )
 		return 0;
 
 	Val* v = id->ID_Val();
-	Unref(id);
 
 	if ( v )
 		{
@@ -621,13 +612,11 @@ ListVal* internal_list_val(const char* name)
 
 BroType* internal_type(const char* name)
 	{
-	ID* id = lookup_ID(name, GLOBAL_MODULE_NAME);
+	auto id = lookup_ID(name, GLOBAL_MODULE_NAME);
 	if ( ! id )
 		reporter->InternalError("internal type %s missing", name);
 
-	BroType* rval = id->Type();
+	return id->Type();
-	Unref(id);
-	return rval;
 	}
 
 Func* internal_func(const char* name)

src/input/Manager.cc

@@ -2622,7 +2622,7 @@ Val* Manager::ValueToVal(const Stream* i, const Value* val, bool& have_error) co
 		string enum_string(val->val.string_val.data, val->val.string_val.length);
 
 		// let's try looking it up by global ID.
-		ID* id = lookup_ID(enum_string.c_str(), GLOBAL_MODULE_NAME);
+		auto id = lookup_ID(enum_string.c_str(), GLOBAL_MODULE_NAME);
 		if ( ! id || ! id->IsEnumConst() )
 			{
 			Warning(i, "Value '%s' for stream '%s' is not a valid enum.",

src/parse.y

@@ -666,7 +666,7 @@ expr:
 			{
 			set_location(@1);
 
-			ID* id = lookup_ID($1, current_module.c_str());
+			auto id = lookup_ID($1, current_module.c_str());
 			if ( ! id )
 				{
 				if ( ! in_debug )
@@ -687,12 +687,14 @@ expr:
 				}
 			else
 				{
+				if ( id->IsDeprecated() )
+					reporter->Warning("%s", id->GetDeprecationWarning().c_str());
+
 				if ( ! id->Type() )
 					{
 					id->Error("undeclared variable");
 					id->SetType(error_type());
-					Ref(id);
+					$$ = new NameExpr(id.release());
-					$$ = new NameExpr(id);
 					}
 
 				else if ( id->IsEnumConst() )
@@ -706,14 +708,8 @@ expr:
 					}
 				else
 					{
-					Ref(id);
+					$$ = new NameExpr(id.release());
-					$$ = new NameExpr(id);
 					}
-
-				if ( id->IsDeprecated() )
-					reporter->Warning("%s", id->GetDeprecationWarning().c_str());
-
-				Unref(id);
 				}
 			}
 
@@ -1578,7 +1574,7 @@ event:
 			{
 			set_location(@1, @4);
 
-			ID* id = lookup_ID($1, current_module.c_str());
+			auto id = lookup_ID($1, current_module.c_str());
 			if ( id )
 				{
 				if ( ! id->IsGlobal() )
@@ -1588,8 +1584,6 @@ event:
 					}
 				if ( id->IsDeprecated() )
 					reporter->Warning("%s", id->GetDeprecationWarning().c_str());
-
-				Unref(id);
 				}
 
 			$$ = new EventExpr($1, $3);
@@ -1636,13 +1630,13 @@ case_type:
 			{
 			const char* name = $4;
 			IntrusivePtr<BroType> type{AdoptRef{}, $2};
-			IntrusivePtr<ID> case_var{AdoptRef{}, lookup_ID(name, current_module.c_str())};
+			auto case_var = lookup_ID(name, current_module.c_str());
 
 			if ( case_var && case_var->IsGlobal() )
 				case_var->Error("already a global identifier");
 			else
 				{
-				case_var = {NewRef{}, install_ID(name, current_module.c_str(), false, false)};
+				case_var = install_ID(name, current_module.c_str(), false, false);
 				}
 
 			add_local(case_var, std::move(type), INIT_NONE, 0, 0, VAR_REGULAR);
@@ -1658,7 +1652,7 @@ for_head:
 			// body so that we execute these actions - defining
 			// the local variable - prior to parsing the body,
 			// which might refer to the variable.
-			ID* loop_var = lookup_ID($3, current_module.c_str());
+			auto loop_var = lookup_ID($3, current_module.c_str());
 
 			if ( loop_var )
 				{
@@ -1668,13 +1662,12 @@ for_head:
 
 			else
 				{
-				Unref(loop_var);
 				loop_var = install_ID($3, current_module.c_str(),
 						      false, false);
 				}
 
 			id_list* loop_vars = new id_list;
-			loop_vars->push_back(loop_var);
+			loop_vars->push_back(loop_var.release());
 
 			$$ = new ForStmt(loop_vars, $5);
 			}
@@ -1691,8 +1684,8 @@ for_head:
 
 			// Check for previous definitions of key and
 			// value variables.
-			ID* key_var = lookup_ID($3, module);
+			auto key_var = lookup_ID($3, module);
-			ID* val_var = lookup_ID($5, module);
+			auto val_var = lookup_ID($5, module);
 
 			// Validate previous definitions as needed.
 			if ( key_var )
@@ -1712,9 +1705,9 @@ for_head:
 				val_var = install_ID($5, module, false, false);
 
 			id_list* loop_vars = new id_list;
-			loop_vars->push_back(key_var);
+			loop_vars->push_back(key_var.release());
 
-			$$ = new ForStmt(loop_vars, $7, val_var);
+			$$ = new ForStmt(loop_vars, $7, val_var.release());
 			}
 	|
 		TOK_FOR '(' '[' local_id_list ']' ',' TOK_ID TOK_IN expr ')'
@@ -1723,7 +1716,7 @@ for_head:
 			const char* module = current_module.c_str();
 
 			// Validate value variable
-			ID* val_var = lookup_ID($7, module);
+			auto val_var = lookup_ID($7, module);
 
 			if ( val_var )
 				{
@@ -1733,7 +1726,7 @@ for_head:
 			else
 				val_var = install_ID($7, module, false, false);
 
-			$$ = new ForStmt($4, $9, val_var);
+			$$ = new ForStmt($4, $9, val_var.release());
 			}
 	;
 
@@ -1752,7 +1745,7 @@ local_id:
 			{
 			set_location(@1);
 
-			$$ = lookup_ID($1, current_module.c_str());
+			$$ = lookup_ID($1, current_module.c_str()).release();
 			if ( $$ )
 				{
 				if ( $$->IsGlobal() )
@@ -1763,7 +1756,7 @@ local_id:
 			else
 				{
 				$$ = install_ID($1, current_module.c_str(),
-						false, is_export);
+						false, is_export).release();
 				}
 			}
 	;
@@ -1788,7 +1781,7 @@ global_or_event_id:
 			{
 			set_location(@1);
 
-			$$ = lookup_ID($1, current_module.c_str(), false, defining_global_ID);
+			$$ = lookup_ID($1, current_module.c_str(), false, defining_global_ID).release();
 			if ( $$ )
 				{
 				if ( ! $$->IsGlobal() )
@@ -1813,7 +1806,7 @@ global_or_event_id:
 						current_module.c_str() : 0;
 
 				$$ = install_ID($1, module_name,
-						true, is_export);
+						true, is_export).release();
 				}
 			}
 	;
@@ -1823,7 +1816,7 @@ resolve_id:
 		TOK_ID
 			{
 			set_location(@1);
-			$$ = lookup_ID($1, current_module.c_str());
+			$$ = lookup_ID($1, current_module.c_str()).release();
 
 			if ( ! $$ )
 				reporter->Error("identifier not defined: %s", $1);

src/plugin/ComponentManager.h

@@ -133,9 +133,9 @@ ComponentManager<T, C>::ComponentManager(const string& arg_module, const string&
 	: module(arg_module),
 	  tag_enum_type(make_intrusive<EnumType>(module + "::" + local_id))
 	{
-	::ID* id = install_ID(local_id.c_str(), module.c_str(), true, true);
+	auto id = install_ID(local_id.c_str(), module.c_str(), true, true);
-	add_type(id, tag_enum_type, 0);
+	add_type(id.get(), tag_enum_type, 0);
-	zeekygen_mgr->Identifier(id);
+	zeekygen_mgr->Identifier(id.get());
 	}
 
 template <class T, class C>

src/scan.l

@@ -775,30 +775,26 @@ void do_atifdef(const char* id)
 	{
 	++current_depth;
 
-	ID* i;
+	auto i = lookup_ID(id, current_module.c_str());
 
-	if ( ! (i = lookup_ID(id, current_module.c_str())) )
+	if ( ! i )
 		{
 		if_stack.push_back(current_depth);
 		BEGIN(IGNORE);
 		}
-
-	Unref(i);
 	}
 
 void do_atifndef(const char *id)
 	{
 	++current_depth;
 
-	ID* i;
+	auto i = lookup_ID(id, current_module.c_str());
 
-	if ( (i = lookup_ID(id, current_module.c_str())) )
+	if ( i )
 		{
 		if_stack.push_back(current_depth);
 		BEGIN(IGNORE);
 		}
-
-	Unref(i);
 	}
 
 void do_atelse()