Merge remote-tracking branch 'origin/topic/jsiwek/ticket946'

Closes #946.

* origin/topic/jsiwek/ticket946:
  Fix memory leaks resulting from 'when' and 'return when' statements.
  Fix three bugs with 'when' and 'return when' statements. Addresses #946

CHANGES

@@ -1,14 +1,22 @@
 
+2.1-336 | 2013-03-06 15:08:06 -0800
+
+  * Fix memory leaks resulting from 'when' and 'return when'
+    statements. Addresses #946. (Jon Siwek)
+
+  * Fix three bugs with 'when' and 'return when' statements. Addresses
+    #946. (Jon Siwek)
+
 2.1-333 | 2013-03-06 14:59:47 -0800
 
   * Add parsing for GTPv1 extension headers and control messages.  (Jon Siwek)
 
     This includes:
 
-      - a new generic gtpv1_message() event generated for any GTP
+      - A new generic gtpv1_message() event generated for any GTP
         message type.
 
-      - specific events for the create/update/delete PDP context
+      - Specific events for the create/update/delete PDP context
         request/response messages.
 
     Addresses #934.

VERSION

@@ -1 +1 @@
-2.1-333
+2.1-336

src/Debug.cc

@@ -763,7 +763,7 @@ int dbg_handle_debug_input()
 	Frame* curr_frame = g_frame_stack.back();
 	const BroFunc* func = curr_frame->GetFunction();
 	if ( func )
-		current_module = func->GetID()->ModuleName();
+		current_module = extract_module_name(func->Name());
 	else
 		current_module = GLOBAL_MODULE_NAME;
 

src/Expr.cc

@@ -4639,12 +4639,16 @@ Val* CallExpr::Eval(Frame* f) const
 		{
 		const ::Func* func = func_val->AsFunc();
 		calling_expr = this;
+		const CallExpr* current_call = f ? f->GetCall() : 0;
 
 		if ( f )
 			f->SetCall(this);
+
 		ret = func->Call(v, f); // No try/catch here; we pass exceptions upstream.
+
 		if ( f )
-			f->ClearCall();
+			f->SetCall(current_call);
+
 		// Don't Unref() the arguments, as Func::Call already did that.
 		delete v;
 

src/Frame.cc

@@ -87,8 +87,11 @@ Frame* Frame::Clone()
 
 void Frame::SetTrigger(Trigger* arg_trigger)
 	{
+	ClearTrigger();
+
 	if ( arg_trigger )
 		Ref(arg_trigger);
+
 	trigger = arg_trigger;
 	}
 

src/Func.cc

@@ -54,13 +54,13 @@ bool did_builtin_init = false;
 
 vector<Func*> Func::unique_ids;
 
-Func::Func() : scope(0), id(0), return_value(0)
+Func::Func() : scope(0), type(0)
 	{
 	unique_id = unique_ids.size();
 	unique_ids.push_back(this);
 	}
 
-Func::Func(Kind arg_kind) : scope(0), kind(arg_kind), id(0), return_value(0)
+Func::Func(Kind arg_kind) : scope(0), kind(arg_kind), type(0)
 	{
 	unique_id = unique_ids.size();
 	unique_ids.push_back(this);
@@ -68,6 +68,7 @@ Func::Func(Kind arg_kind) : scope(0), kind(arg_kind), id(0), return_value(0)
 
 Func::~Func()
 	{
+	Unref(type);
 	}
 
 void Func::AddBody(Stmt* /* new_body */, id_list* /* new_inits */,
@@ -129,6 +130,12 @@ bool Func::DoSerialize(SerialInfo* info) const
 	if ( ! SERIALIZE(char(kind) ) )
 		return false;
 
+	if ( ! type->Serialize(info) )
+		return false;
+
+	if ( ! SERIALIZE(Name()) )
+		return false;
+
 	// We don't serialize scope as only global functions are considered here
 	// anyway.
 	return true;
@@ -160,12 +167,25 @@ bool Func::DoUnserialize(UnserialInfo* info)
 		return false;
 
 	kind = (Kind) c;
+
+	type = BroType::Unserialize(info);
+	if ( ! type )
+		return false;
+
+	const char* n;
+	if ( ! UNSERIALIZE_STR(&n, 0) )
+		return false;
+
+	name = n;
+	delete [] n;
+
 	return true;
 	}
 
 void Func::DescribeDebug(ODesc* d, const val_list* args) const
 	{
-	id->Describe(d);
+	d->Add(Name());
+
 	RecordType* func_args = FType()->Args();
 
 	if ( args )
@@ -196,21 +216,6 @@ void Func::DescribeDebug(ODesc* d, const val_list* args) const
 		}
 	}
 
-void Func::SetID(ID *arg_id)
-	{
-	id = arg_id;
-
-	return_value =
-		new ID(string(string(id->Name()) + "_returnvalue").c_str(),
-			SCOPE_FUNCTION, false);
-	return_value->SetType(FType()->YieldType()->Ref());
-	}
-
-ID* Func::GetReturnValueID() const
-	{
-	return return_value;
-	}
-
 TraversalCode Func::Traverse(TraversalCallback* cb) const
 	{
 	// FIXME: Make a fake scope for builtins?
@@ -226,12 +231,6 @@ TraversalCode Func::Traverse(TraversalCallback* cb) const
 		tc = scope->Traverse(cb);
 		HANDLE_TC_STMT_PRE(tc);
 
-		if ( GetReturnValueID() )
-			{
-			tc = GetReturnValueID()->Traverse(cb);
-			HANDLE_TC_STMT_PRE(tc);
-			}
-
 		for ( unsigned int i = 0; i < bodies.size(); ++i )
 			{
 			tc = bodies[i].stmts->Traverse(cb);
@@ -249,7 +248,8 @@ BroFunc::BroFunc(ID* arg_id, Stmt* arg_body, id_list* aggr_inits,
 		int arg_frame_size, int priority)
 : Func(BRO_FUNC)
 	{
-	id = arg_id;
+	name = arg_id->Name();
+	type = arg_id->Type()->Ref();
 	frame_size = arg_frame_size;
 
 	if ( arg_body )
@@ -263,7 +263,6 @@ BroFunc::BroFunc(ID* arg_id, Stmt* arg_body, id_list* aggr_inits,
 
 BroFunc::~BroFunc()
 	{
-	Unref(id);
 	for ( unsigned int i = 0; i < bodies.size(); ++i )
 		Unref(bodies[i].stmts);
 	}
@@ -378,7 +377,8 @@ Val* BroFunc::Call(val_list* args, Frame* parent) const
 	     (flow != FLOW_RETURN /* we fell off the end */ ||
 	      ! result /* explicit return with no result */) &&
 	     ! f->HasDelayed() )
-		reporter->Warning("non-void function returns without a value: %s", id->Name());
+		reporter->Warning("non-void function returns without a value: %s",
+		                  Name());
 
 	if ( result && g_trace_state.DoTrace() )
 		{
@@ -421,8 +421,7 @@ void BroFunc::AddBody(Stmt* new_body, id_list* new_inits, int new_frame_size,
 
 void BroFunc::Describe(ODesc* d) const
 	{
-	if ( id )
-		id->Describe(d);
+	d->Add(Name());
 
 	d->NL();
 	d->AddCount(frame_size);
@@ -450,14 +449,14 @@ IMPLEMENT_SERIAL(BroFunc, SER_BRO_FUNC);
 bool BroFunc::DoSerialize(SerialInfo* info) const
 	{
 	DO_SERIALIZE(SER_BRO_FUNC, Func);
-	return id->Serialize(info) && SERIALIZE(frame_size);
+	return SERIALIZE(frame_size);
 	}
 
 bool BroFunc::DoUnserialize(UnserialInfo* info)
 	{
 	DO_UNSERIALIZE(Func);
-	id = ID::Unserialize(info);
-	return id && UNSERIALIZE(&frame_size);
+
+	return UNSERIALIZE(&frame_size);
 	}
 
 BuiltinFunc::BuiltinFunc(built_in_func arg_func, const char* arg_name,
@@ -465,15 +464,16 @@ BuiltinFunc::BuiltinFunc(built_in_func arg_func, const char* arg_name,
 : Func(BUILTIN_FUNC)
 	{
 	func = arg_func;
-	name = copy_string(make_full_var_name(GLOBAL_MODULE_NAME, arg_name).c_str());
+	name = make_full_var_name(GLOBAL_MODULE_NAME, arg_name);
 	is_pure = arg_is_pure;
 
-	id = lookup_ID(name, GLOBAL_MODULE_NAME, false);
+	ID* id = lookup_ID(Name(), GLOBAL_MODULE_NAME, false);
 	if ( ! id )
-		reporter->InternalError("built-in function %s missing", name);
+		reporter->InternalError("built-in function %s missing", Name());
 	if ( id->HasVal() )
-		reporter->InternalError("built-in function %s multiply defined", name);
+		reporter->InternalError("built-in function %s multiply defined", Name());
 
+	type = id->Type()->Ref();
 	id->SetVal(new Val(this));
 	}
 
@@ -491,7 +491,7 @@ Val* BuiltinFunc::Call(val_list* args, Frame* parent) const
 #ifdef PROFILE_BRO_FUNCTIONS
 	DEBUG_MSG("Function: %s\n", Name());
 #endif
-	SegmentProfiler(segment_logger, name);
+	SegmentProfiler(segment_logger, Name());
 
 	if ( sample_logger )
 		sample_logger->FunctionSeen(this);
@@ -522,8 +522,7 @@ Val* BuiltinFunc::Call(val_list* args, Frame* parent) const
 
 void BuiltinFunc::Describe(ODesc* d) const
 	{
-	if ( id )
-	id->Describe(d);
+	d->Add(Name());
 	d->AddCount(is_pure);
 	}
 
@@ -532,16 +531,13 @@ IMPLEMENT_SERIAL(BuiltinFunc, SER_BUILTIN_FUNC);
 bool BuiltinFunc::DoSerialize(SerialInfo* info) const
 	{
 	DO_SERIALIZE(SER_BUILTIN_FUNC, Func);
-
-	// We ignore the ID. Func::Serialize() will rebind us anyway.
-	return SERIALIZE(name);
+	return true;
 	}
 
 bool BuiltinFunc::DoUnserialize(UnserialInfo* info)
 	{
 	DO_UNSERIALIZE(Func);
-	id = 0;
-	return UNSERIALIZE_STR(&name, 0);
+	return true;
 	}
 
 void builtin_error(const char* msg, BroObj* arg)

src/Func.h

@@ -47,15 +47,11 @@ public:
 	virtual void SetScope(Scope* newscope)	{ scope = newscope; }
 	virtual Scope* GetScope() const		{ return scope; }
 
-	virtual FuncType* FType() const
-		{
-		return (FuncType*) id->Type()->AsFuncType();
-		}
+	virtual FuncType* FType() const { return type->AsFuncType(); }
 
 	Kind GetKind() const	{ return kind; }
 
-	const ID* GetID() const { return id; }
-	void SetID(ID *arg_id);
+	const char* Name() const { return name.c_str(); }
 
 	virtual void Describe(ODesc* d) const = 0;
 	virtual void DescribeDebug(ODesc* d, const val_list* args) const;
@@ -64,7 +60,6 @@ public:
 	bool Serialize(SerialInfo* info) const;
 	static Func* Unserialize(UnserialInfo* info);
 
-	ID* GetReturnValueID() const;
 	virtual TraversalCode Traverse(TraversalCallback* cb) const;
 
 	uint32 GetUniqueFuncID() const { return unique_id; }
@@ -79,8 +74,8 @@ protected:
 	vector<Body> bodies;
 	Scope* scope;
 	Kind kind;
-	ID* id;
-	ID* return_value;
+	BroType* type;
+	string name;
 	uint32 unique_id;
 	static vector<Func*> unique_ids;
 };
@@ -119,18 +114,16 @@ public:
 
 	int IsPure() const;
 	Val* Call(val_list* args, Frame* parent) const;
-	const char* Name() const	{ return name; }
 	built_in_func TheFunc() const	{ return func; }
 
 	void Describe(ODesc* d) const;
 
 protected:
-	BuiltinFunc()	{ func = 0; name = 0; is_pure = 0; }
+	BuiltinFunc()	{ func = 0; is_pure = 0; }
 
 	DECLARE_SERIAL(BuiltinFunc);
 
 	built_in_func func;
-	const char* name;
 	int is_pure;
 };
 

src/StateAccess.cc

@@ -926,17 +926,22 @@ void NotifierRegistry::Register(ID* id, NotifierRegistry::Notifier* notifier)
 	DBG_LOG(DBG_NOTIFIERS, "registering ID %s for notifier %s",
 		id->Name(), notifier->Name());
 
+	Attr* attr = new Attr(ATTR_TRACKED);
+
 	if ( id->Attrs() )
-		id->Attrs()->AddAttr(new Attr(ATTR_TRACKED));
+		{
+		if ( ! id->Attrs()->FindAttr(ATTR_TRACKED) )
+			id->Attrs()->AddAttr(attr);
+		}
 	else
 		{
 		attr_list* a = new attr_list;
-		Attr* attr = new Attr(ATTR_TRACKED);
 		a->append(attr);
 		id->SetAttrs(new Attributes(a, id->Type(), false));
-		Unref(attr);
 		}
 
+	Unref(attr);
+
 	NotifierMap::iterator i = ids.find(id->Name());
 
 	if ( i != ids.end() )
@@ -967,7 +972,9 @@ void NotifierRegistry::Unregister(ID* id, NotifierRegistry::Notifier* notifier)
 	if ( i == ids.end() )
 		return;
 
+	Attr* attr = id->Attrs()->FindAttr(ATTR_TRACKED);
 	id->Attrs()->RemoveAttr(ATTR_TRACKED);
+	Unref(attr);
 
 	NotifierSet* s = i->second;
 	s->erase(notifier);

src/Stats.cc

@@ -338,7 +338,7 @@ SampleLogger::~SampleLogger()
 
 void SampleLogger::FunctionSeen(const Func* func)
 	{
-	load_samples->Assign(new StringVal(func->GetID()->Name()), 0);
+	load_samples->Assign(new StringVal(func->Name()), 0);
 	}
 
 void SampleLogger::LocationSeen(const Location* loc)

src/Trigger.cc

@@ -242,6 +242,7 @@ bool Trigger::Eval()
 
 		trigger->Cache(frame->GetCall(), v);
 		trigger->Release();
+		frame->ClearTrigger();
 		}
 
 	Unref(v);
@@ -330,6 +331,7 @@ void Trigger::Timeout()
 #endif
 			trigger->Cache(frame->GetCall(), v);
 			trigger->Release();
+			frame->ClearTrigger();
 			}
 
 		Unref(v);
@@ -424,6 +426,12 @@ Val* Trigger::Lookup(const CallExpr* expr)
 	return (i != cache.end()) ? i->second : 0;
 	}
 
+void Trigger::Disable()
+	{
+	UnregisterAll();
+	disabled = true;
+	}
+
 const char* Trigger::Name() const
 	{
 	assert(location);

src/Trigger.h

@@ -49,7 +49,7 @@ public:
 
 	// Disable this trigger completely. Needed because Unref'ing the trigger
 	// may not immediately delete it as other references may still exist.
-	void Disable()	{ disabled = true; }
+	void Disable();
 
 	virtual void Describe(ODesc* d) const { d->Add("<trigger>"); }
 
@@ -79,7 +79,6 @@ private:
 	friend class TriggerTimer;
 
 	void Init();
-	void DeleteTrigger();
 	void Register(ID* id);
 	void Register(Val* val);
 	void UnregisterAll();

src/Type.cc

@@ -696,7 +696,9 @@ string FuncType::FlavorString() const
 
 FuncType::~FuncType()
 	{
+	Unref(args);
 	Unref(arg_types);
+	Unref(yield);
 	}
 
 BroType* FuncType::YieldType()

src/input/Manager.cc

@@ -483,7 +483,7 @@ bool Manager::CreateEventStream(RecordVal* fval)
 	Unref(fields); // ref'd by lookupwithdefault
 	stream->num_fields = fieldsV.size();
 	stream->fields = fields->Ref()->AsRecordType();
-	stream->event = event_registry->Lookup(event->GetID()->Name());
+	stream->event = event_registry->Lookup(event->Name());
 	stream->want_record = ( want_record->InternalInt() == 1 );
 	Unref(want_record); // ref'd by lookupwithdefault
 
@@ -644,7 +644,7 @@ bool Manager::CreateTableStream(RecordVal* fval)
 	stream->tab = dst->AsTableVal();
 	stream->rtype = val ? val->AsRecordType() : 0;
 	stream->itype = idx->AsRecordType();
-	stream->event = event ? event_registry->Lookup(event->GetID()->Name()) : 0;
+	stream->event = event ? event_registry->Lookup(event->Name()) : 0;
 	stream->currDict = new PDict(InputHash);
 	stream->currDict->SetDeleteFunc(input_hash_delete_func);
 	stream->lastDict = new PDict(InputHash);

src/logging/Manager.cc

@@ -365,7 +365,7 @@ bool Manager::CreateStream(EnumVal* id, RecordVal* sval)
 	streams[idx]->id = id->Ref()->AsEnumVal();
 	streams[idx]->enabled = true;
 	streams[idx]->name = id->Type()->AsEnumType()->Lookup(idx);
-	streams[idx]->event = event ? event_registry->Lookup(event->GetID()->Name()) : 0;
+	streams[idx]->event = event ? event_registry->Lookup(event->Name()) : 0;
 	streams[idx]->columns = columns->Ref()->AsRecordType();
 
 	DBG_LOG(DBG_LOGGING, "Created new logging stream '%s', raising event %s",

testing/btest/Baseline/language.returnwhen/bro..stdout

@@ -0,0 +1,12 @@
+dummy from async_func() from bro_init()
+async_func() return result in bro_init(), flag in my_set
+dummy from bro_init() when block
+hi!
+dummy from async_func() from do_another()
+async_func() return result in do_another(), flag in my_set
+dummy from do_another() when block
+hi!
+dummy from async_func() from do_another()
+async_func() return result in do_another(), timeout
+dummy from do_another() when block
+hi!

testing/btest/core/leaks/returnwhen.bro

@@ -0,0 +1,84 @@
+# Needs perftools support.
+#
+# @TEST-GROUP: leaks
+#
+# @TEST-REQUIRES: bro  --help 2>&1 | grep -q mem-leaks
+#
+# @TEST-EXEC: btest-bg-run bro HEAP_CHECK_DUMP_DIRECTORY=. HEAPCHECK=local bro -m -b %INPUT
+# @TEST-EXEC: btest-bg-wait 15
+
+redef exit_only_after_terminate = T;
+
+global my_set: set[string] = set();
+global flag: string = "flag";
+global done: bool = F;
+
+function dummyfunc(s: string): string
+	{
+	return "dummy " + s;
+	}
+
+function async_func(s: string): string
+	{
+	print dummyfunc("from async_func() " + s);
+
+	return when ( flag in my_set )
+		{
+		return flag + " in my_set";
+		}
+	timeout 3sec
+		{
+		return "timeout";
+		}
+	}
+
+event set_flag()
+	{
+	add my_set[flag];
+	}
+
+event do_another()
+	{
+	delete my_set[flag];
+
+	local local_dummy = dummyfunc;
+
+	local anon = function(s: string): string { return s + "!"; };
+
+	if ( ! done )
+		schedule 1sec { set_flag() };
+
+	when ( local result = async_func("from do_another()") )
+		{
+		print "async_func() return result in do_another()", result;
+		print local_dummy("from do_another() when block");
+		print anon("hi");
+		if ( result == "timeout" )
+			terminate();
+		else
+			{
+			done = T;
+			schedule 10msec { do_another() };
+			}
+		}
+	}
+
+event bro_init()
+	{
+	local local_dummy = dummyfunc;
+
+	local anon = function(s: string): string { return s + "!"; };
+
+	schedule 1sec { set_flag() };
+
+	when ( local result = async_func("from bro_init()") )
+		{
+		print "async_func() return result in bro_init()", result;
+		print local_dummy("from bro_init() when block");
+		print anon("hi");
+		if ( result == "timeout" ) terminate();
+		schedule 10msec { do_another() };
+		}
+	}
+
+

testing/btest/language/returnwhen.bro

@@ -0,0 +1,79 @@
+# @TEST-EXEC: btest-bg-run bro bro -b %INPUT
+# @TEST-EXEC: btest-bg-wait 15
+# @TEST-EXEC: btest-diff bro/.stdout
+
+redef exit_only_after_terminate = T;
+
+global my_set: set[string] = set();
+global flag: string = "flag";
+global done: bool = F;
+
+function dummyfunc(s: string): string
+	{
+	return "dummy " + s;
+	}
+
+function async_func(s: string): string
+	{
+	print dummyfunc("from async_func() " + s);
+
+	return when ( flag in my_set )
+		{
+		return flag + " in my_set";
+		}
+	timeout 3sec
+		{
+		return "timeout";
+		}
+	}
+
+event set_flag()
+	{
+	add my_set[flag];
+	}
+
+event do_another()
+	{
+	delete my_set[flag];
+
+	local local_dummy = dummyfunc;
+
+	local anon = function(s: string): string { return s + "!"; };
+
+	if ( ! done )
+		schedule 1sec { set_flag() };
+
+	when ( local result = async_func("from do_another()") )
+		{
+		print "async_func() return result in do_another()", result;
+		print local_dummy("from do_another() when block");
+		print anon("hi");
+		if ( result == "timeout" )
+			terminate();
+		else
+			{
+			done = T;
+			schedule 10msec { do_another() };
+			}
+		}
+	}
+
+event bro_init()
+	{
+	local local_dummy = dummyfunc;
+
+	local anon = function(s: string): string { return s + "!"; };
+
+	schedule 1sec { set_flag() };
+
+	when ( local result = async_func("from bro_init()") )
+		{
+		print "async_func() return result in bro_init()", result;
+		print local_dummy("from bro_init() when block");
+		print anon("hi");
+		if ( result == "timeout" ) terminate();
+		schedule 10msec { do_another() };
+		}
+	}
+
+