Re: [Bro-Dev] Consistent error handling for scripting mistakes
On Mon, Nov 12, 2018 at 12:27 -0600, Jonathan Siwek wrote: > I recently noticed there's a range of behaviors in how various > scripting mistakes are treated. There's a 4th: InterpreterException. > 1st question: should these be made more consistent? I'd say yes. Yes, definitely. > that it's only the *current function body* (yes, *function*, not > event) that exits early -- hard to reason about what sort of arbitrary > code was depending on that function to be fully evaluated and what > other sort of inconsistent state is caused by exiting early. ... and what happens if the function is supposed to return a value, but now doesn't? > I propose, for 2.7, to aim for consistent error handling for scripting > mistakes and that the expected behavior is to unwind all the way to > exiting the current event handler (all its function bodies). Agree with that. Can we do that cleanly though? The problem with InterpreterException is that it may leak memory. We'd need to do the unwinding manually throughout the interpreter code, but that means touching a number of places to pass the error information back. > One exception may be within bro_init(), if an error happens at that > time, I'd say it's fine to completely abort -- it's unlikely or hard > to say whether Bro would operate well if it proceeded after an error > that early in initialization. Yeah, that makes sense. Robin -- Robin Sommer * Corelight, Inc. * ro...@corelight.com * www.corelight.com ___ bro-dev mailing list bro-dev@bro.org http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev
Re: [Bro-Dev] Consistent error handling for scripting mistakes
On Mon, Nov 12, 2018 at 1:44 PM Jim Mellander wrote:
> I was tinkering with the sumstats code, and inadvertantly deleted the final
> "}" closing out the last function. When running the code, the misleading
> error message is received:
Yes, that's a bit of a different topic, but still tracked (at
low-normal priority):
https://github.com/bro/bro/issues/167
> There are also silent fails which probably should give a warning, such as
> failing to include the fully-qualified event name silently preventing the
> event from being triggered.
Also a bit different that what I was talking about, but also tracked
(at higher priority since it's a common mistake):
https://github.com/bro/bro/issues/163
> My idea on runtime scripting errors would be to apply a sensible default to
> the offending expression (null or 0, as the case may be, might be
> sufficient), log the error, and continue
In the following example (comments reflect current behavior) you'd
expect the "false" branch in foo() to be taken?
#
function foo()
{
local t: table[string] of string = table();
# Non-existing index access: (sub)expressions are not evaluated
if ( t["nope"] == "nope" )
# Unreachable
print "yes";
else
# Unreachable
print "no";
# Reachable
print "foo done";
}
event bro_init()
{
foo();
# Reachable
print "bro_init done";
}
#
My thought was that should behave more like a "key error" run-time
exception (e.g. like Python). Bro scripting doesn't have exception
support, but internally we can use an exception to unwind the call
stack (additionally I was thinking that the unwind needs to proceed
further than what it does already in some cases, which is just the
current function body). In any case, logging of the error would also
occur (as it already does).
- Jon
___
bro-dev mailing list
bro-dev@bro.org
http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev
Re: [Bro-Dev] Consistent error handling for scripting mistakes
Along the same vein of sensible Bro script error handling, I'm resending an issue I found in January: I was tinkering with the sumstats code, and inadvertantly deleted the final "}" closing out the last function. When running the code, the misleading error message is received: error in bro/share/bro/base/frameworks/tunnels/./main.bro, line 8: syntax error, at or near "module" presumably due to the function still being open when the next policy script is loaded. Wouldn't it be more reasonable to check at the end of each script when loaded that there are no dangling functions, expressions, etc. == There are also silent fails which probably should give a warning, such as failing to include the fully-qualified event name silently preventing the event from being triggered. == The above are more in the area of parsing vs runtime. My idea on runtime scripting errors would be to apply a sensible default to the offending expression (null or 0, as the case may be, might be sufficient), log the error, and continue Jim On Mon, Nov 12, 2018 at 10:27 AM, Jon Siwek wrote: > Trying to broaden the scope of: > > https://github.com/bro/bro/issues/208 > > I recently noticed there's a range of behaviors in how various > scripting mistakes are treated. They may (1) abort, as in case of bad > subnet mask or incompatible vector element assignment (2) skip over > evaluating (sub)expression(s), but otherwise continue current function > body, as in case of non-existing table index access or (3) exit the > current function body, as in the classic case of uninitialized record > field access. > > 1st question: should these be made more consistent? I'd say yes. > > 2nd question: what is the expected way for these to be handled? I'd > argue that (3) is close to expected behavior, but it's still weird > that it's only the *current function body* (yes, *function*, not > event) that exits early -- hard to reason about what sort of arbitrary > code was depending on that function to be fully evaluated and what > other sort of inconsistent state is caused by exiting early. > > I propose, for 2.7, to aim for consistent error handling for scripting > mistakes and that the expected behavior is to unwind all the way to > exiting the current event handler (all its function bodies). That > makes it easier to explain how to write event handlers such that they > won't enter too wild/inconsistent of a state should a scripting error > occur: "always write an event handler such that it makes no > assumptions about order/priority of other events handlers". That's > already close to current suggestions/approaches. > > One exception may be within bro_init(), if an error happens at that > time, I'd say it's fine to completely abort -- it's unlikely or hard > to say whether Bro would operate well if it proceeded after an error > that early in initialization. > > Thoughts? > > - Jon > ___ > bro-dev mailing list > bro-dev@bro.org > http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev > ___ bro-dev mailing list bro-dev@bro.org http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev
[Bro-Dev] Consistent error handling for scripting mistakes
Trying to broaden the scope of: https://github.com/bro/bro/issues/208 I recently noticed there's a range of behaviors in how various scripting mistakes are treated. They may (1) abort, as in case of bad subnet mask or incompatible vector element assignment (2) skip over evaluating (sub)expression(s), but otherwise continue current function body, as in case of non-existing table index access or (3) exit the current function body, as in the classic case of uninitialized record field access. 1st question: should these be made more consistent? I'd say yes. 2nd question: what is the expected way for these to be handled? I'd argue that (3) is close to expected behavior, but it's still weird that it's only the *current function body* (yes, *function*, not event) that exits early -- hard to reason about what sort of arbitrary code was depending on that function to be fully evaluated and what other sort of inconsistent state is caused by exiting early. I propose, for 2.7, to aim for consistent error handling for scripting mistakes and that the expected behavior is to unwind all the way to exiting the current event handler (all its function bodies). That makes it easier to explain how to write event handlers such that they won't enter too wild/inconsistent of a state should a scripting error occur: "always write an event handler such that it makes no assumptions about order/priority of other events handlers". That's already close to current suggestions/approaches. One exception may be within bro_init(), if an error happens at that time, I'd say it's fine to completely abort -- it's unlikely or hard to say whether Bro would operate well if it proceeded after an error that early in initialization. Thoughts? - Jon ___ bro-dev mailing list bro-dev@bro.org http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev
