On Mon, Nov 12, 2018 at 10:39 PM Robin Sommer <robin(a)corelight.com> wrote:
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?
Accesses to it emit a "value used but not set" error, but subsequent
statements within same function/event still get evaluated (unless they
are themselves now incoherent and trigger various cascading error
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.
Should be possible, just a question of effort/difficulty. An
alternative to manually passing error information back via return
values is migrating from explicit reference counting to shared_ptr.
Either approach requires touching similar code locations, but also the
later may be easier to proceed with after the old serialization system
gets removed from the BroObj class hierarchy.
Since we already have a class of errors that may induce leaks, we
could still move forward with applying consistent error handling
behavior via InterpreterException, but then later expect to resolve
the leakage issue independently via implementation detail
The final resolution is still for people to correct underlying
scripting mistakes, it's just that having more consistent and improved
error handling makes it easier to reason about the subsequent
operational state of Bro with more confidence.