Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

engineUpdate() doesn't properly defend against reentrancy. #249

Closed
terrillmoore opened this issue Mar 16, 2019 · 1 comment
Closed

engineUpdate() doesn't properly defend against reentrancy. #249

terrillmoore opened this issue Mar 16, 2019 · 1 comment
Assignees

Comments

@terrillmoore
Copy link
Member

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.

@terrillmoore
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant