USSTRocketry / MiniRockets

Making rockets that hopefully go UP!
MIT License
2 stars 11 forks source link

statemachine vulnerability #13

Closed snowy-shadow closed 2 years ago

snowy-shadow commented 2 years ago

please replace the statemachine with a

namespace xyz
{ 
    enum class abc 
    {
      states
    }
}

Always prefer enum class over enum

frroossst commented 2 years ago

Will fix after migration to CPP is complete! And MVP code for a mini rocket is complete.

Expected completion date : 30 November 2022

frroossst commented 2 years ago

Enum classes not supported by C!

Our cpp should maintain compatibility with core C, to keep the code portable for future micro controller changes. Might reopen this later.

snowy-shadow commented 2 years ago

enum is supported by C. problem is

enum States
{
  a,
  b
}

a == 0 is evaluated true, which is another vulnerability which enum class fixes this means if(states == a) is the same as if(states == 0) but this is still better than have 5 states as 5 bools

frroossst commented 2 years ago

wouldn't this help solve the issue you mention? Now every state will be valid only once

 #include<stdio.h>

  enum state{Ground, Flight, Landed};

  int main()
  {
      enum state s;

      s = Ground;
      printf("%d \n",s);
      return 0;
 }
snowy-shadow commented 2 years ago

that is the main idea yes. we want to only allow one state to exist at a time so

typedef struct state
{
   state1,
  state2,
}state;
state s = state1;
func(s);
func2(s);

there are also fancier implementations that I've seen(with pointers and classes, for C, it would be c with classes, but classes are structs so c++ impelentations wil be different but much cleaner), but for our purpose it would suffice here i think. I could look more into FSM designs since im also interested in using them in my own projects

its just our current design is very dangerous. forgetting to reset 1 bool value means our rocket will execute two states. Which possibly leads to self destruction

frroossst commented 2 years ago

I agree it is quite dumb to use a struct for the actual state than enum, as multiple states would not be valid at the same time, we can probably blame @djh293 for that lol

yes! and in our current design we can also conceivable set the wrong states

rocketstate.landed = true;
rocketstate.flight = true;

A simple error or oversight in copy pasting code would lead to some major problems

snowy-shadow commented 2 years ago

I got the statemachine figured out, im going to start working on the statemachine if there are no other urgent tasks

this is the most elegent version of statemachine that I could find on the internet...

Here is the proof of concept: https://godbolt.org/z/zonscWdPW

frroossst commented 2 years ago

Hold off on it for now. Dylan and I have different priorities right now. We're working out some managerial kinks on the project.