ETLCPP / etl

Embedded Template Library
https://www.etlcpp.com
MIT License
2.04k stars 371 forks source link

`etl::fsm` `receive` function can call `p_state->process_event()` also when `p_state` is nullptr #869

Closed kingtom22 closed 2 months ago

kingtom22 commented 3 months ago

Hi,

I noticed, that there is no check of the p_state before calling its function.

void receive(const etl::imessage& message) ETL_OVERRIDE
{
  etl::fsm_state_id_t next_state_id = p_state->process_event(message);

  if (have_changed_state(next_state_id))
  {
    ETL_ASSERT(next_state_id < number_of_states, ETL_ERROR(etl::fsm_state_id_exception));
    etl::ifsm_state* p_next_state = state_list[next_state_id];

    do
    {
      p_state->on_exit_state();
      p_state = p_next_state;

      next_state_id = p_state->on_enter_state();

      if (have_changed_state(next_state_id))
      {
        ETL_ASSERT(next_state_id < number_of_states, ETL_ERROR(etl::fsm_state_id_exception));
        p_next_state = state_list[next_state_id];
      }
    } while (p_next_state != p_state); // Have we changed state again?
  }
  else if (is_self_transition(next_state_id))
  {
    p_state->on_exit_state();
    p_state->on_enter_state();
  }
}

What is the intended functionality? I did not found specifications of this functionality. Other functions of the etl::fsm asserting on the p_state before calling it. for example:

etl::fsm_state_id_t get_state_id() const
{
  ETL_ASSERT(p_state != ETL_NULLPTR, ETL_ERROR(etl::fsm_null_state_exception));
  return p_state->get_state_id();
}
jwellbelove commented 3 months ago

You are correct in that, if an etl::fsm is constructed, but has not had set_states and start called, then a call to receive will try to call process_event on a nullptr state.

kingtom22 commented 3 months ago

And is it the intended functionality? Wouldnt be better to have an assert and a big if inside the function checking the pointer? I think it is a low hanging fruit and this would make the library safer.

jwellbelove commented 3 months ago

I've already created a hotfix branch with the same change you describe. The receive function will raise an etl::fsm_not_started if is_started() returns false and only execute the main function if is_started() returns true.

kingtom22 commented 3 months ago

Thank You for the quick reaction.

jwellbelove commented 2 months ago

Fixed 20.38.11