pez-globo / pufferfish-software

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

Avoid passing pointers+lengths for byte buffers in the firmware #287

Open ethanjli opened 3 years ago

ethanjli commented 3 years ago

In various HAL, driver, and utility functions/methods of the firmware (e.g. Util/COBS.h, Util/Endian.h, Util/Parse.h, Util/Vector.h, I2C/ExtendedI2CDevice.h, HAL/Interfaces/BufferedUART.h, HAL/Interfaces/CRCChecker.h, HAL/Interfaces/I2CDevice.h, HAL/Interfaces/SPIDevice.h), we are passing around arrays (or segments of arrays) as raw uint8_t pointers and array lengths, and we are doing pointer arithmetic; this is risky for bounds safety and memory safety. Currently, because we are passing around these raw pointers for arrays, we have to disable the cppcoreguidelines-pro-bounds-pointer-arithmetic and cppcoreguidelines-pro-bounds-constant-array-index checks in clang-tidy. Because of this, Util::Vectors also exposes a pointer to internal buffer, which breaks the abstraction provided by Util::Vector.

We should instead use something which gives us a way to pass around views to arrays; C++20 provides the span class which does exactly that (see here for more discussion), but we have to use C++17 instead of C++20 because of our current tooling. Ideally, we would both be able to read segments of Util::Vectors through spans and modify segments of Util::Vectors through spans. We would use this to provide a bounds-safe wrapper around the STM32H7 HAL's C-style functions, which take raw pointers and lengths.

To get a span implementation we can use, we could consider either https://github.com/martinmoene/span-lite or https://github.com/tcbrindle/span , and either use them directly and wrap around them if necessary to get IndexStatus return codes (preferred) or adapt them (if we can't just wrap around them). We need to make sure that we can disable exceptions and still have some type of bounds-checking (preferably from the span, or else by requiring the caller code to do some checks beforehand).

ethanjli commented 3 years ago

It looks like the other option short of using spans is to make functions templated on array size; then we can either use c-array iterators like template< class T, std::size_t N > constexpr T* begin( T (&array)[N] ) noexcept; and template< class T, std::size_t N >constexpr T* end( T (&array)[N] ) noexcept;, or maybe we can try to make our functions take T (&array)[N] as their params rather than a pointer and a size parameter (I'm not sure if this latter approach is feasible though).

Using std::begin and std::end is how we can, for example, use std::equal to compare two C-style arrays - refer to https://stackoverflow.com/a/12870958