mcci-catena/arduino-lmic

engineUpdate() doesn't properly defend against reentrancy.

terrillmoore opened this issue · 1 comments

The engineUpdate() code is essentially an FSM evaluator. One of the things that can happen while evaluating is reportEvent(), which in turn dispatches to client code.

There are paths from client code that directly trigger calls to engineUpdate(). This means that engineUpdate() can be invoked recursively if clients call those APIs from onEvent() or the new equivalents. Based on my testing, that can be a bad thing, depending on the whether the recursion is effectively a tail-recursion.

The current design of engineUpdate() is not a fully elaborated FSM; instead, callbacks trigger other code that may or may not trigger calls to engineUpdate(). Clearly, in simple cases things are correct, but in more complex cases (such as the certification tests), it becomes quite a burden on the client. Instead, engineUpdate() and the APIs should be designed to be forgiving; if you call things from your event handler at an inconvenient time, processing should be deferred to a later time. Luckily, this is unlikely to break any client code, but rather make it more robust.

At the same time we'll want to address #240.

This is also needed for proper Class B and Class C support. In Class A, it's easy to predict when the LMIC will be active and avoid doing things that will endanger engineUpdate processing. But with Class B and Class C, messages can be received at any time. The good thing is that this is really not very hard to fix, and won't really change the "flavor" of the LMIC.