[Bro-Dev] [Bro-Commits] [git/bro] master: Experimental code to better handle interpreter errors. (15ab287)
Jonathan Siwek
jsiwek at ncsa.illinois.edu
Tue Oct 25 12:53:49 PDT 2011
I think this commit caused the failures of
testing/btest/scripts/base/frameworks/logging/pred.bro
testing/btest/scripts/base/frameworks/logging/remote.bro
Looks like something to do with log filtering got broken, not sure how.
- Jon
On Oct 21, 2011, at 1:43 PM, Robin Sommer wrote:
> Repository : ssh://git@bro-ids.icir.org/bro
>
> On branches: master,topic/robin/interpreter-exceptions
> Link : http://tracker.bro-ids.org/bro/changeset/15ab2874369b5d7a3e6a14df24b141fa759999bb/bro
>
>> ---------------------------------------------------------------
>
> commit 15ab2874369b5d7a3e6a14df24b141fa759999bb
> Author: Robin Sommer <robin at icir.org>
> Date: Sun Oct 9 20:28:06 2011 -0700
>
> Experimental code to better handle interpreter errors.
>
> Currently, a lot of interpreter runtime errors, such as an access to
> an unset optional record field, cause Bro to abort with an internal
> error. This is an experimental branch that turns such errors into
> non-fatal runtime errors by internally raising exceptions. These are
> caught upstream and processing continues afterwards.
>
> For now, not many errors actually raise exceptions (the example above
> does though). We'll need to go through them eventually and adapt the
> current Internal() calls (and potentially others). More generally, at
> some point we should cleanup the interpreter error handling (unifying
> errors reported at parse- and runtime; and switching to exceptions for
> all Expr/Stmt/Vals). But that's a larger change and left for later.
>
> The main question for now is if this code is already helpful enough to
> go into 2.0. It will quite likely prevent a number of crashes due to
> script errors.
>
>
>> ---------------------------------------------------------------
>
> 15ab2874369b5d7a3e6a14df24b141fa759999bb
> src/Discard.cc | 48 ++++++++++++++++++++++++++++++++++++++---
> src/Event.h | 11 ++++++++-
> src/EventHandler.cc | 1 +
> src/Expr.cc | 10 +++++---
> src/LogMgr.cc | 43 +++++++++++++++++++++++++++++++------
> src/PersistenceSerializer.cc | 8 ++++++-
> src/RemoteSerializer.cc | 8 ++++++-
> src/Reporter.cc | 44 +++++++++++++++++++++++++++++--------
> src/Reporter.h | 24 ++++++++++++++++++++-
> src/RuleCondition.cc | 16 +++++++++++--
> src/Sessions.cc | 9 +++++++-
> src/Val.cc | 29 +++++++++++++++++++++----
> 12 files changed, 213 insertions(+), 38 deletions(-)
>
> diff --git a/src/Discard.cc b/src/Discard.cc
> index 2705aa5..a71b810 100644
> --- a/src/Discard.cc
> +++ b/src/Discard.cc
> @@ -42,7 +42,17 @@ int Discarder::NextPacket(const IP_Hdr* ip, int len, int caplen)
> {
> val_list* args = new val_list;
> args->append(BuildHeader(ip4));
> - discard_packet = check_ip->Call(args)->AsBool();
> +
> + try
> + {
> + discard_packet = check_ip->Call(args)->AsBool();
> + }
> +
> + catch ( InterpreterException& e )
> + {
> + discard_packet = false;
> + }
> +
> delete args;
>
> if ( discard_packet )
> @@ -90,7 +100,17 @@ int Discarder::NextPacket(const IP_Hdr* ip, int len, int caplen)
> args->append(BuildHeader(ip4));
> args->append(BuildHeader(tp, len));
> args->append(BuildData(data, th_len, len, caplen));
> - discard_packet = check_tcp->Call(args)->AsBool();
> +
> + try
> + {
> + discard_packet = check_tcp->Call(args)->AsBool();
> + }
> +
> + catch ( InterpreterException& e )
> + {
> + discard_packet = false;
> + }
> +
> delete args;
> }
> }
> @@ -106,7 +126,17 @@ int Discarder::NextPacket(const IP_Hdr* ip, int len, int caplen)
> args->append(BuildHeader(ip4));
> args->append(BuildHeader(up));
> args->append(BuildData(data, uh_len, len, caplen));
> - discard_packet = check_udp->Call(args)->AsBool();
> +
> + try
> + {
> + discard_packet = check_udp->Call(args)->AsBool();
> + }
> +
> + catch ( InterpreterException& e )
> + {
> + discard_packet = false;
> + }
> +
> delete args;
> }
> }
> @@ -120,7 +150,17 @@ int Discarder::NextPacket(const IP_Hdr* ip, int len, int caplen)
> val_list* args = new val_list;
> args->append(BuildHeader(ip4));
> args->append(BuildHeader(ih));
> - discard_packet = check_icmp->Call(args)->AsBool();
> +
> + try
> + {
> + discard_packet = check_icmp->Call(args)->AsBool();
> + }
> +
> + catch ( InterpreterException& e )
> + {
> + discard_packet = false;
> + }
> +
> delete args;
> }
> }
> diff --git a/src/Event.h b/src/Event.h
> index 805396a..1740372 100644
> --- a/src/Event.h
> +++ b/src/Event.h
> @@ -41,7 +41,16 @@ protected:
> if ( handler->ErrorHandler() )
> reporter->BeginErrorHandler();
>
> - handler->Call(args, no_remote);
> + try
> + {
> + handler->Call(args, no_remote);
> + }
> +
> + catch ( InterpreterException& e )
> + {
> + // Already reported.
> + }
> +
> if ( obj )
> // obj->EventDone();
> Unref(obj);
> diff --git a/src/EventHandler.cc b/src/EventHandler.cc
> index 55f9902..2867b63 100644
> --- a/src/EventHandler.cc
> +++ b/src/EventHandler.cc
> @@ -68,6 +68,7 @@ void EventHandler::Call(val_list* vl, bool no_remote)
> }
>
> if ( local )
> + // No try/catch here; we pass exceptions upstream.
> Unref(local->Call(vl));
> else
> {
> diff --git a/src/Expr.cc b/src/Expr.cc
> index 9fecab4..f6d1fc5 100644
> --- a/src/Expr.cc
> +++ b/src/Expr.cc
> @@ -3118,8 +3118,9 @@ Val* FieldExpr::Fold(Val* v) const
> return def_attr->AttrExpr()->Eval(0);
> else
> {
> - Internal("field value missing");
> - return 0;
> + reporter->ExprRuntimeError(this, "field value missing");
> + assert(false);
> + return 0; // Will never get here, but compiler can't tell.
> }
> }
>
> @@ -3730,6 +3731,7 @@ Val* RecordMatchExpr::Fold(Val* v1, Val* v2) const
> }
> }
>
> + // No try/catch here; we pass exceptions upstream.
> Val* pred_val =
> match_rec->Lookup(pred_field_index)->AsFunc()->Call(&args);
> bool is_zero = pred_val->IsZero();
> @@ -4220,7 +4222,7 @@ Val* FlattenExpr::Fold(Val* v) const
> l->Append(fa->AttrExpr()->Eval(0));
>
> else
> - Internal("missing field value");
> + reporter->ExprRuntimeError(this, "missing field value");
> }
>
> return l;
> @@ -4646,7 +4648,7 @@ Val* CallExpr::Eval(Frame* f) const
>
> if ( f )
> f->SetCall(this);
> - ret = func->Call(v, f);
> + ret = func->Call(v, f); // No try/catch here; we pass exceptions upstream.
> if ( f )
> f->ClearCall();
> // Don't Unref() the arguments, as Func::Call already did that.
> diff --git a/src/LogMgr.cc b/src/LogMgr.cc
> index 3f088fd..473f480 100644
> --- a/src/LogMgr.cc
> +++ b/src/LogMgr.cc
> @@ -888,9 +888,18 @@ bool LogMgr::Write(EnumVal* id, RecordVal* columns)
> // to log this record.
> val_list vl(1);
> vl.append(columns->Ref());
> - Val* v = filter->pred->Call(&vl);
> - int result = v->AsBool();
> - Unref(v);
> +
> + int result = 1;
> +
> + try
> + {
> + Val* v = filter->pred->Call(&vl);
> + int result = v->AsBool();
> + Unref(v);
> + }
> +
> + catch ( InterpreterException& e )
> + { /* Already reported. */ }
>
> if ( ! result )
> continue;
> @@ -920,7 +929,17 @@ bool LogMgr::Write(EnumVal* id, RecordVal* columns)
>
> vl.append(rec_arg);
>
> - Val* v = filter->path_func->Call(&vl);
> + Val* v = 0;
> +
> + try
> + {
> + v = filter->path_func->Call(&vl);
> + }
> +
> + catch ( InterpreterException& e )
> + {
> + return false;
> + }
>
> if ( ! v->Type()->Tag() == TYPE_STRING )
> {
> @@ -1569,9 +1588,19 @@ bool LogMgr::FinishedRotation(LogWriter* writer, string new_name, string old_nam
> // Call the postprocessor function.
> val_list vl(1);
> vl.append(info);
> - Val* v = func->Call(&vl);
> - int result = v->AsBool();
> - Unref(v);
> +
> + int result = 0;
> +
> + try
> + {
> + Val* v = func->Call(&vl);
> + result = v->AsBool();
> + Unref(v);
> + }
> +
> + catch ( InterpreterException& e )
> + { /* Already reported. */ }
> +
> return result;
> }
>
> diff --git a/src/PersistenceSerializer.cc b/src/PersistenceSerializer.cc
> index c72f59c..c757467 100644
> --- a/src/PersistenceSerializer.cc
> +++ b/src/PersistenceSerializer.cc
> @@ -200,7 +200,13 @@ void PersistenceSerializer::GotEvent(const char* name, double time,
> void PersistenceSerializer::GotFunctionCall(const char* name, double time,
> Func* func, val_list* args)
> {
> - func->Call(args);
> + try
> + {
> + func->Call(args);
> + }
> +
> + catch ( InterpreterException& e )
> + { /* Already reported. */ }
> }
>
> void PersistenceSerializer::GotStateAccess(StateAccess* s)
> diff --git a/src/RemoteSerializer.cc b/src/RemoteSerializer.cc
> index e359a4a..7926894 100644
> --- a/src/RemoteSerializer.cc
> +++ b/src/RemoteSerializer.cc
> @@ -2809,7 +2809,13 @@ void RemoteSerializer::GotFunctionCall(const char* name, double time,
> return;
> }
>
> - function->Call(args);
> + try
> + {
> + function->Call(args);
> + }
> +
> + catch ( InterpreterException& e )
> + { /* Already reported. */ }
> }
>
> void RemoteSerializer::GotID(ID* id, Val* val)
> diff --git a/src/Reporter.cc b/src/Reporter.cc
> index b2bffd3..fca9a6a 100644
> --- a/src/Reporter.cc
> +++ b/src/Reporter.cc
> @@ -39,7 +39,7 @@ void Reporter::Info(const char* fmt, ...)
> {
> va_list ap;
> va_start(ap, fmt);
> - DoLog("", reporter_info, stderr, 0, 0, true, true, fmt, ap);
> + DoLog("", reporter_info, stderr, 0, 0, true, true, 0, fmt, ap);
> va_end(ap);
> }
>
> @@ -47,7 +47,7 @@ void Reporter::Warning(const char* fmt, ...)
> {
> va_list ap;
> va_start(ap, fmt);
> - DoLog("warning", reporter_warning, stderr, 0, 0, true, true, fmt, ap);
> + DoLog("warning", reporter_warning, stderr, 0, 0, true, true, 0, fmt, ap);
> va_end(ap);
> }
>
> @@ -56,7 +56,7 @@ void Reporter::Error(const char* fmt, ...)
> ++errors;
> va_list ap;
> va_start(ap, fmt);
> - DoLog("error", reporter_error, stderr, 0, 0, true, true, fmt, ap);
> + DoLog("error", reporter_error, stderr, 0, 0, true, true, 0, fmt, ap);
> va_end(ap);
> }
>
> @@ -66,7 +66,7 @@ void Reporter::FatalError(const char* fmt, ...)
> va_start(ap, fmt);
>
> // Always log to stderr.
> - DoLog("fatal error", 0, stderr, 0, 0, true, false, fmt, ap);
> + DoLog("fatal error", 0, stderr, 0, 0, true, false, 0, fmt, ap);
>
> va_end(ap);
>
> @@ -80,7 +80,7 @@ void Reporter::FatalErrorWithCore(const char* fmt, ...)
> va_start(ap, fmt);
>
> // Always log to stderr.
> - DoLog("fatal error", 0, stderr, 0, 0, true, false, fmt, ap);
> + DoLog("fatal error", 0, stderr, 0, 0, true, false, 0, fmt, ap);
>
> va_end(ap);
>
> @@ -88,13 +88,29 @@ void Reporter::FatalErrorWithCore(const char* fmt, ...)
> abort();
> }
>
> +void Reporter::ExprRuntimeError(const Expr* expr, const char* fmt, ...)
> + {
> + ++errors;
> +
> + ODesc d;
> + expr->Describe(&d);
> +
> + PushLocation(expr->GetLocationInfo());
> + va_list ap;
> + va_start(ap, fmt);
> + DoLog("expression error", reporter_error, stderr, 0, 0, true, true, d.Description(), fmt, ap);
> + va_end(ap);
> + PopLocation();
> + throw InterpreterException();
> + }
> +
> void Reporter::InternalError(const char* fmt, ...)
> {
> va_list ap;
> va_start(ap, fmt);
>
> // Always log to stderr.
> - DoLog("internal error", 0, stderr, 0, 0, true, false, fmt, ap);
> + DoLog("internal error", 0, stderr, 0, 0, true, false, 0, fmt, ap);
>
> va_end(ap);
>
> @@ -106,7 +122,7 @@ void Reporter::InternalWarning(const char* fmt, ...)
> {
> va_list ap;
> va_start(ap, fmt);
> - DoLog("internal warning", reporter_warning, stderr, 0, 0, true, true, fmt, ap);
> + DoLog("internal warning", reporter_warning, stderr, 0, 0, true, true, 0, fmt, ap);
> va_end(ap);
> }
>
> @@ -133,7 +149,7 @@ void Reporter::WeirdHelper(EventHandlerPtr event, Val* conn_val, const char* add
>
> va_list ap;
> va_start(ap, fmt_name);
> - DoLog("weird", event, stderr, 0, vl, false, false, fmt_name, ap);
> + DoLog("weird", event, stderr, 0, vl, false, false, 0, fmt_name, ap);
> va_end(ap);
>
> delete vl;
> @@ -147,7 +163,7 @@ void Reporter::WeirdFlowHelper(const uint32* orig, const uint32* resp, const cha
>
> va_list ap;
> va_start(ap, fmt_name);
> - DoLog("weird", flow_weird, stderr, 0, vl, false, false, fmt_name, ap);
> + DoLog("weird", flow_weird, stderr, 0, vl, false, false, 0, fmt_name, ap);
> va_end(ap);
>
> delete vl;
> @@ -173,7 +189,7 @@ void Reporter::Weird(const uint32* orig, const uint32* resp, const char* name)
> WeirdFlowHelper(orig, resp, "%s", name);
> }
>
> -void Reporter::DoLog(const char* prefix, EventHandlerPtr event, FILE* out, Connection* conn, val_list* addl, bool location, bool time, const char* fmt, va_list ap)
> +void Reporter::DoLog(const char* prefix, EventHandlerPtr event, FILE* out, Connection* conn, val_list* addl, bool location, bool time, const char* postfix, const char* fmt, va_list ap)
> {
> static char tmp[512];
>
> @@ -235,6 +251,9 @@ void Reporter::DoLog(const char* prefix, EventHandlerPtr event, FILE* out, Conne
> int n = vsnprintf(buffer, size, fmt, aq);
> va_end(aq);
>
> + if ( postfix )
> + n += strlen(postfix) + 10; // Add a bit of slack.
> +
> if ( n > -1 && n < size )
> // We had enough space;
> break;
> @@ -247,6 +266,11 @@ void Reporter::DoLog(const char* prefix, EventHandlerPtr event, FILE* out, Conne
> FatalError("out of memory in Reporter");
> }
>
> + if ( postfix )
> + // Note, if you change this fmt string, adjust the additional
> + // buffer size above.
> + sprintf(buffer + strlen(buffer), " [%s]", postfix);
> +
> if ( event && via_events && ! in_error_handler )
> {
> val_list* vl = new val_list;
> diff --git a/src/Reporter.h b/src/Reporter.h
> index d0fc662..52a2aff 100644
> --- a/src/Reporter.h
> +++ b/src/Reporter.h
> @@ -14,6 +14,22 @@
>
> class Connection;
> class Location;
> +class Reporter;
> +
> +// One cannot raise this exception directly, go through the
> +// Reporter's methods instead.
> +
> +class ReporterException {
> +protected:
> + friend class Reporter;
> + ReporterException() {}
> +};
> +
> +class InterpreterException : public ReporterException {
> +protected:
> + friend class Reporter;
> + InterpreterException() {}
> +};
>
> class Reporter {
> public:
> @@ -42,6 +58,10 @@ public:
> // reported and always generate a core dump.
> void FatalErrorWithCore(const char* fmt, ...);
>
> + // Report a runtime error in evaluating a Bro script expression. This
> + // function will not return but raise an InterpreterException.
> + void ExprRuntimeError(const Expr* expr, const char* fmt, ...);
> +
> // Report a traffic weirdness, i.e., an unexpected protocol situation
> // that may lead to incorrectly processing a connnection.
> void Weird(const char* name); // Raises net_weird().
> @@ -87,7 +107,9 @@ public:
> void EndErrorHandler() { --in_error_handler; }
>
> private:
> - void DoLog(const char* prefix, EventHandlerPtr event, FILE* out, Connection* conn, val_list* addl, bool location, bool time, const char* fmt, va_list ap);
> + void DoLog(const char* prefix, EventHandlerPtr event, FILE* out,
> + Connection* conn, val_list* addl, bool location, bool time,
> + const char* postfix, const char* fmt, va_list ap);
>
> // The order if addl, name needs to be like that since fmt_name can
> // contain format specifiers
> diff --git a/src/RuleCondition.cc b/src/RuleCondition.cc
> index 1b94fcf..8852747 100644
> --- a/src/RuleCondition.cc
> +++ b/src/RuleCondition.cc
> @@ -149,9 +149,19 @@ bool RuleConditionEval::DoMatch(Rule* rule, RuleEndpointState* state,
> else
> args.append(new StringVal(""));
>
> - Val* val = id->ID_Val()->AsFunc()->Call(&args);
> - bool result = val->AsBool();
> - Unref(val);
> + bool result = 0;
> +
> + try
> + {
> + Val* val = id->ID_Val()->AsFunc()->Call(&args);
> + result = val->AsBool();
> + Unref(val);
> + }
> +
> + catch ( InterpreterException& e )
> + {
> + result = false;
> + }
>
> return result;
> }
> diff --git a/src/Sessions.cc b/src/Sessions.cc
> index 1d2604b..572e8de 100644
> --- a/src/Sessions.cc
> +++ b/src/Sessions.cc
> @@ -331,7 +331,14 @@ void NetSessions::NextPacketSecondary(double /* t */, const struct pcap_pkthdr*
> args->append(cmd_val);
> args->append(BuildHeader(ip));
> // ### Need to queue event here.
> - sp->Event()->Event()->Call(args);
> + try
> + {
> + sp->Event()->Event()->Call(args);
> + }
> +
> + catch ( InterpreterException& e )
> + { /* Already reported. */ }
> +
> delete args;
> }
> }
> diff --git a/src/Val.cc b/src/Val.cc
> index 164994b..ddf04bb 100644
> --- a/src/Val.cc
> +++ b/src/Val.cc
> @@ -2007,7 +2007,16 @@ Val* TableVal::Default(Val* index)
> else
> vl->append(index->Ref());
>
> - Val* result = f->Call(vl);
> + Val* result = 0;
> +
> + try
> + {
> + result = f->Call(vl);
> + }
> +
> + catch ( InterpreterException& e )
> + { /* Already reported. */ }
> +
> delete vl;
>
> if ( ! result )
> @@ -2466,10 +2475,20 @@ double TableVal::CallExpireFunc(Val* idx)
>
> vl->append(idx);
>
> - Val* vs = expire_expr->Eval(0)->AsFunc()->Call(vl);
> - double secs = vs->AsInterval();
> - Unref(vs);
> - delete vl;
> + double secs;
> +
> + try
> + {
> + Val* vs = expire_expr->Eval(0)->AsFunc()->Call(vl);
> + secs = vs->AsInterval();
> + Unref(vs);
> + delete vl;
> + }
> +
> + catch ( InterpreterException& e )
> + {
> + secs = 0;
> + }
>
> return secs;
> }
>
> _______________________________________________
> bro-commits mailing list
> bro-commits at bro-ids.org
> http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-commits
>
More information about the bro-dev
mailing list