Merge remote-tracking branch 'origin/topic/robin/interpreter-exceptions'

* origin/topic/robin/interpreter-exceptions:
  Adding test for new error handling.
  Experimental code to better handle interpreter errors.

This seems to work fine and it catches some potentially nasty crashes
so I'm merging it in even though it's not the final word on error
handling yet. #646 tracks the work scheduled for later.

aux/binpac

@@ -1 +1 @@
-Subproject commit c8760bd06861e2cfe9a4bc652fb634a6f6422ea6
+Subproject commit 41a046e47af2bdb6c54f306ae600bbe69798a190

aux/bro-aux

@@ -1 +1 @@
-Subproject commit c0b0d58bc60c006445aa767fcc0ce09b3d317a36
+Subproject commit 9730115948982d06faaf3845d73961dbd81ed236

aux/broccoli

@@ -1 +1 @@
-Subproject commit 32233c4459d5196712b7f8de7b85875cc91d4c3a
+Subproject commit 27826f49a835fa3653661231d46dcb83b92d8324

aux/broctl

@@ -1 +1 @@
-Subproject commit 44797ae8a21adcb478dcea63526d8ff4063785e8
+Subproject commit 7c9062e92f7b69af47831d46c331e6b8a3b09296

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));
+
+		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));
+
+			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));
+
+			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));
+
+			try
+				{
 				discard_packet = check_icmp->Call(args)->AsBool();
+				}
+
+			catch ( InterpreterException& e )
+				{
+				discard_packet = false;
+				}
+
 			delete args;
 			}
 		}

src/Event.h

@@ -41,7 +41,16 @@ protected:
 		if ( handler->ErrorHandler() )
 			reporter->BeginErrorHandler();
 
+		try
+			{
 			handler->Call(args, no_remote);
+			}
+
+		catch ( InterpreterException& e )
+			{
+			// Already reported.
+			}
+
 		if ( obj )
 			// obj->EventDone();
 			Unref(obj);

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
 		{

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.

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());
+
+			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);
+
+	int result = 0;
+
+	try
+		{
 		Val* v = func->Call(&vl);
-	int result = v->AsBool();
+		result = v->AsBool();
 		Unref(v);
+		}
+
+	catch ( InterpreterException& e )
+		{ /* Already reported. */ }
+
 	return result;
 	}
 

src/PersistenceSerializer.cc

@@ -200,9 +200,15 @@ void PersistenceSerializer::GotEvent(const char* name, double time,
 void PersistenceSerializer::GotFunctionCall(const char* name, double time,
 					Func* func, val_list* args)
 	{
+	try
+		{
 		func->Call(args);
 		}
 
+	catch ( InterpreterException& e )
+		{ /* Already reported. */ }
+	}
+
 void PersistenceSerializer::GotStateAccess(StateAccess* s)
 	{
 	s->Replay();

src/RemoteSerializer.cc

@@ -2809,9 +2809,15 @@ void RemoteSerializer::GotFunctionCall(const char* name, double time,
 		return;
 		}
 
+	try
+		{
 		function->Call(args);
 		}
 
+	catch ( InterpreterException& e )
+		{ /* Already reported. */ }
+	}
+
 void RemoteSerializer::GotID(ID* id, Val* val)
 	{
 	++stats.ids.in;

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;

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()	{}
+};
 
 // Check printf-style variadic arguments if we can.
 #if __GNUC__
@@ -49,6 +65,10 @@ public:
 	// reported and always generate a core dump.
 	void FatalErrorWithCore(const char* fmt, ...) FMT_ATTR;
 
+	// 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().
@@ -94,7 +114,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

src/RuleCondition.cc

@@ -149,9 +149,19 @@ bool RuleConditionEval::DoMatch(Rule* rule, RuleEndpointState* state,
 	else
 		args.append(new StringVal(""));
 
+	bool result = 0;
+
+	try
+		{
 		Val* val = id->ID_Val()->AsFunc()->Call(&args);
-	bool result = val->AsBool();
+		result = val->AsBool();
 		Unref(val);
+		}
+
+	catch ( InterpreterException& e )
+		{
+		result = false;
+		}
 
 	return result;
 	}

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.
+			try
+				{
 				sp->Event()->Event()->Call(args);
+				}
+
+			catch ( InterpreterException& e )
+				{ /* Already reported. */ }
+
 			delete args;
 			}
 		}

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);
 
+	double secs;
+
+	try
+		{
 		Val* vs = expire_expr->Eval(0)->AsFunc()->Call(vl);
-	double secs = vs->AsInterval();
+		secs = vs->AsInterval();
 		Unref(vs);
 		delete vl;
+		}
+
+	catch ( InterpreterException& e )
+		{
+		secs = 0;
+		}
 
 	return secs;
 	}

testing/btest/Baseline/core.expr-exception/reporter.log

@@ -0,0 +1,13 @@
+#separator \x09
+#path	reporter
+#fields	ts	level	message	location
+#types	time	enum	string	string
+1300475168.783842	Reporter::ERROR	field value missing [c$ftp]	/home/robin/bro/master/testing/btest/.tmp/core.expr-exception/expr-exception.bro, line 8
+1300475168.915940	Reporter::ERROR	field value missing [c$ftp]	/home/robin/bro/master/testing/btest/.tmp/core.expr-exception/expr-exception.bro, line 8
+1300475168.916118	Reporter::ERROR	field value missing [c$ftp]	/home/robin/bro/master/testing/btest/.tmp/core.expr-exception/expr-exception.bro, line 8
+1300475168.918295	Reporter::ERROR	field value missing [c$ftp]	/home/robin/bro/master/testing/btest/.tmp/core.expr-exception/expr-exception.bro, line 8
+1300475168.952193	Reporter::ERROR	field value missing [c$ftp]	/home/robin/bro/master/testing/btest/.tmp/core.expr-exception/expr-exception.bro, line 8
+1300475168.952228	Reporter::ERROR	field value missing [c$ftp]	/home/robin/bro/master/testing/btest/.tmp/core.expr-exception/expr-exception.bro, line 8
+1300475168.954761	Reporter::ERROR	field value missing [c$ftp]	/home/robin/bro/master/testing/btest/.tmp/core.expr-exception/expr-exception.bro, line 8
+1300475168.962628	Reporter::ERROR	field value missing [c$ftp]	/home/robin/bro/master/testing/btest/.tmp/core.expr-exception/expr-exception.bro, line 8
+1300475169.780331	Reporter::ERROR	field value missing [c$ftp]	/home/robin/bro/master/testing/btest/.tmp/core.expr-exception/expr-exception.bro, line 8

testing/btest/core/expr-exception.bro

@@ -0,0 +1,9 @@
+# Bro shouldn't crash when doing nothing, nor outputting anything.
+#
+# @TEST-EXEC: cat /dev/null | bro -r $TRACES/wikipedia.trace %INPUT
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff reporter.log
+
+event connection_established(c: connection)
+	{
+	print c$ftp;
+	}