diff --git a/CHANGES b/CHANGES index 997ed7c418..24bc414394 100644 --- a/CHANGES +++ b/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. diff --git a/VERSION b/VERSION index e0d2515005..7e23b50c4c 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.1-333 +2.1-336 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..237618fd41 100644 --- a/src/Expr.cc +++ b/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; diff --git a/src/Frame.cc b/src/Frame.cc index f86fa32805..8754c02a9f 100644 --- a/src/Frame.cc +++ b/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; } diff --git a/src/Func.cc b/src/Func.cc index 9b94b15d97..02f8dd4f29 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,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) 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/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/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..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); @@ -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); diff --git a/src/Trigger.h b/src/Trigger.h index 8e04fb9189..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(""); } @@ -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/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/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/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() }; + } + } + + 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() }; + } + } + +