pez-globo / pufferfish-software

All software for the Pufferfish ventilator.
Apache License 2.0
0 stars 1 forks source link

Add proper type checking in the Messages.parse method #278

Closed ethanjli closed 3 years ago

ethanjli commented 3 years ago

Describe the bug When a uint8_t invalid type code which is not in TaggedUnion::Tag tries to get static casted into TaggedUnion::Tag, a segfault occurs.

Expected behavior We could define a data structure which associates all tag uint8_t values with TaggedUnion::Tag enum values; if a uint8_t value isn't in the data structure, we return an error instead of trying to do a static_cast. We'll need to figure out what kind of data structure to use.

ethanjli commented 3 years ago

I'm unable to reproduce this bug in the actual deployment setup (firmware + Python backend server, configured so that the backend server sends messages with unrecognized type code value 1 to the firmware). @rohanpurohit Can you reproduce the issue via a segfaulting Catch2 test (I remember we discussed the scenario which triggers this, and I think there's commented-out code for it in PR #269) and check whether applying the changes from the feature/messages-type-checking branch causes the test to run successfully?

rohanpurohit commented 3 years ago

Yes @ethanjli the changes from feature/messages-type-checking fixes the issue. the segfaulting Catch2 tests :

ethanjli commented 3 years ago

Great, I'll create a PR tomorrow. I'm surprised the first one is segfaulting, I thought the parse method specifically checked for that case - I'll take a closer look at what's going on

rohanpurohit commented 3 years ago

Oh ya I think this was the edge case where it's equal to the array size.

https://github.com/pez-globo/pufferfish-software/blob/develop/firmware/ventilator-controller-stm32/Core/Inc/Pufferfish/Protocols/Messages.tpp#L62 should be if (type >= pb_protobuf_descriptors.size()) {

ethanjli commented 3 years ago

Right, I remember discussing that but I don't think the fix has been made yet. Maybe that was the final GIthub Issues item I had lost and forgotten about hahaha. I'll include that fix in this PR.