From 7e5115460c2da630825b7ef994591f95aef4f4dc Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Tue, 19 Feb 2013 11:38:17 -0600 Subject: [PATCH 1/2] Fix three bugs with 'when' and 'return when' statements. Addresses #946 - 'when' statements were problematic when used in a function/event/hook that had local variables with an assigned function value. This was because 'when' blocks operate on a clone of the frame and the cloning process serializes locals and the serialization of functions had an infinite cycle in it (ID -> BroFunc -> ID -> BroFunc ...). The ID was only used for the function name and type information, so refactoring Func and subclasses to depend on those two things instead fixes the issue. - 'return when' blocks, specifically, didn't work whenever execution of the containing function's body does another function call before reaching the 'return when' block, because of an assertion. This was was due to logic in CallExpr::Eval always clearing the CallExpr associated with the Frame after doing the call, instead of restoring any previous CallExpr, which the code in Trigger::Eval expected to have available. - An assert could be reached when the condition of a 'when' statement depended on checking the value of global state variables. The assert in Trigger::QueueTrigger that checks that the Trigger isn't disabled would get hit because Trigger::Eval/Timeout disable themselves after running, but don't unregister themselves from the NotifierRegistry, which keeps calling QueueTrigger for every state access of the global. --- src/Debug.cc | 2 +- src/Expr.cc | 3 +- src/Func.cc | 83 +++++++++---------- src/Func.h | 17 ++-- src/Stats.cc | 2 +- src/Trigger.cc | 2 + src/Trigger.h | 1 - src/input/Manager.cc | 4 +- src/logging/Manager.cc | 2 +- .../Baseline/language.returnwhen/bro..stdout | 12 +++ testing/btest/language/returnwhen.bro | 79 ++++++++++++++++++ 11 files changed, 144 insertions(+), 63 deletions(-) create mode 100644 testing/btest/Baseline/language.returnwhen/bro..stdout create mode 100644 testing/btest/language/returnwhen.bro diff --git a/src/Debug.cc b/src/Debug.cc index 535e193685..8cf2e38596 100644 --- a/src/Debug.cc +++ b/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; diff --git a/src/Expr.cc b/src/Expr.cc index 3df4d781a0..1a647e8c01 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -4639,12 +4639,13 @@ 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; diff --git a/src/Func.cc b/src/Func.cc index 9b94b15d97..69af3fb93c 100644 --- a/src/Func.cc +++ b/src/Func.cc @@ -54,13 +54,13 @@ bool did_builtin_init = false; vector 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,24 @@ 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 +215,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 +230,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 +247,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 +262,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 +376,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 +420,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 +448,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 +463,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 +490,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 +521,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 +530,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) diff --git a/src/Func.h b/src/Func.h index 7f9627b66d..3413e0bef1 100644 --- a/src/Func.h +++ b/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 bodies; Scope* scope; Kind kind; - ID* id; - ID* return_value; + BroType* type; + string name; uint32 unique_id; static vector 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; }; diff --git a/src/Stats.cc b/src/Stats.cc index 8d48c47a25..1bccb8f9be 100644 --- a/src/Stats.cc +++ b/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) diff --git a/src/Trigger.cc b/src/Trigger.cc index b7e08b557e..e7e887e055 100644 --- a/src/Trigger.cc +++ b/src/Trigger.cc @@ -251,6 +251,7 @@ bool Trigger::Eval() timer_mgr->Cancel(timer); Disable(); + UnregisterAll(); Unref(this); return true; @@ -337,6 +338,7 @@ void Trigger::Timeout() } Disable(); + UnregisterAll(); Unref(this); } diff --git a/src/Trigger.h b/src/Trigger.h index 8e04fb9189..dd2e171ef6 100644 --- a/src/Trigger.h +++ b/src/Trigger.h @@ -79,7 +79,6 @@ private: friend class TriggerTimer; void Init(); - void DeleteTrigger(); void Register(ID* id); void Register(Val* val); void UnregisterAll(); diff --git a/src/input/Manager.cc b/src/input/Manager.cc index d9006d66a2..e8e12b64c4 100644 --- a/src/input/Manager.cc +++ b/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); diff --git a/src/logging/Manager.cc b/src/logging/Manager.cc index f62cd1685d..1ab83d84ba 100644 --- a/src/logging/Manager.cc +++ b/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", diff --git a/testing/btest/Baseline/language.returnwhen/bro..stdout b/testing/btest/Baseline/language.returnwhen/bro..stdout new file mode 100644 index 0000000000..d213d7bd02 --- /dev/null +++ b/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! diff --git a/testing/btest/language/returnwhen.bro b/testing/btest/language/returnwhen.bro new file mode 100644 index 0000000000..593841eb7e --- /dev/null +++ b/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() }; + } + } + + From d158c7ffdfed8ac58e107efaba660a1d541cb7d0 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Tue, 19 Feb 2013 16:19:16 -0600 Subject: [PATCH 2/2] Fix memory leaks resulting from 'when' and 'return when' statements. Addresses #946. --- src/Frame.cc | 1 + src/StateAccess.cc | 13 +++- src/Trigger.cc | 10 ++- src/Trigger.h | 2 +- src/Type.cc | 2 + testing/btest/core/leaks/returnwhen.bro | 84 +++++++++++++++++++++++++ 6 files changed, 106 insertions(+), 6 deletions(-) create mode 100644 testing/btest/core/leaks/returnwhen.bro diff --git a/src/Frame.cc b/src/Frame.cc index f86fa32805..8a21b02b9e 100644 --- a/src/Frame.cc +++ b/src/Frame.cc @@ -87,6 +87,7 @@ Frame* Frame::Clone() void Frame::SetTrigger(Trigger* arg_trigger) { + ClearTrigger(); if ( arg_trigger ) Ref(arg_trigger); trigger = arg_trigger; diff --git a/src/StateAccess.cc b/src/StateAccess.cc index 2d0a8dfc5a..0deaef5bb5 100644 --- a/src/StateAccess.cc +++ b/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); diff --git a/src/Trigger.cc b/src/Trigger.cc index e7e887e055..a567528232 100644 --- a/src/Trigger.cc +++ b/src/Trigger.cc @@ -242,6 +242,7 @@ bool Trigger::Eval() trigger->Cache(frame->GetCall(), v); trigger->Release(); + frame->ClearTrigger(); } Unref(v); @@ -251,7 +252,6 @@ bool Trigger::Eval() timer_mgr->Cancel(timer); Disable(); - UnregisterAll(); Unref(this); return true; @@ -331,6 +331,7 @@ void Trigger::Timeout() #endif trigger->Cache(frame->GetCall(), v); trigger->Release(); + frame->ClearTrigger(); } Unref(v); @@ -338,7 +339,6 @@ void Trigger::Timeout() } Disable(); - UnregisterAll(); Unref(this); } @@ -426,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); diff --git a/src/Trigger.h b/src/Trigger.h index dd2e171ef6..b752ea8ada 100644 --- a/src/Trigger.h +++ b/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(""); } diff --git a/src/Type.cc b/src/Type.cc index 1fb813efa1..6ac6070660 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -696,7 +696,9 @@ string FuncType::FlavorString() const FuncType::~FuncType() { + Unref(args); Unref(arg_types); + Unref(yield); } BroType* FuncType::YieldType() diff --git a/testing/btest/core/leaks/returnwhen.bro b/testing/btest/core/leaks/returnwhen.bro new file mode 100644 index 0000000000..9fd9a794cd --- /dev/null +++ b/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() }; + } + } + +