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

Make MSP more robust #2948

Merged
merged 2 commits into from
Mar 22, 2018
Merged

Conversation

shellixyz
Copy link
Collaborator

Make all MSP SET messages read operations safe and add a couple of bound checks.

@digitalentity
Copy link
Member

@shellixyz if it's tested - I'm ok with merging it

@fiam
Copy link
Member

fiam commented Mar 20, 2018

I’m just a bit worried of the exact size checks. Some buggy client implementation might be sending extra bytes in the payload, which have been harmless until now. With these changes, we might break them. Of course, if we were starting from scratch I’d enforce exact sizes, but at this point I’m not sure if checking for >= would be a better idea.

@digitalentity
Copy link
Member

@fiam good point. I'd just ensure we have no less bytes than we need

@stronnag
Copy link
Collaborator

No problems with the == diff on mwp or iNav configurator.

@shellixyz
Copy link
Collaborator Author

I tested this while doing normal configuration through the GUI. Not all the messages were tested. Basically all I did was add the dataSize checks so if I calculated the data length of the messages correctly (which I checked twice) it should be good. Does an utility already exist to generate MSP frames that could be used to test all the messages ?

Should I change the conditions to >= or could be keep ==. The benefit of using == is that developers can know when the message data is not right (extraneous data).

@digitalentity
Copy link
Member

I think for compatibility reasons it's better to have >=

@shellixyz
Copy link
Collaborator Author

I changed == to >= for the MSPv1 messages. I don't think it makes sense to do it for MSPv2 messages as they have been added only recently and most likely haven't been used anywhere else than in the configurator.

@digitalentity digitalentity merged commit f393ecc into iNavFlight:development Mar 22, 2018
@shellixyz shellixyz deleted the more_robust_msp branch May 6, 2018 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants