search

bluerange-io / bluerange-mesh

BlueRange Mesh (formerly FruityMesh) - The first completely connection-based open source mesh on top of Bluetooth Low Energy (4.1/5.0 or higher)
https://bluerange.io/
Other
288 stars 109 forks source link

Fixed undefined behavior bug and a possible pitfall for HAL developers #183

Closed Brotcrunsher closed 2 years ago

Brotcrunsher commented 3 years ago

The HAL memory is located on the stack as an array of u8. Previously it was interpreted as a HAL Memory Object without creating it properly. This is a violation of the strict aliasing rule. Problems with this did not happen on GCC previously because FruityMesh is using -fno-strict-aliasing. However, even with this setting it opened a pitfall for HAL developers that assumed they could initialize members inside the struct definition, like:

struct NrfHalMemory
{
    //...
    volatile bool nrfSerialDataAvailable = false;
    volatile bool nrfSerialErrorDetected = false;
    //...
};

(actual code on current master!). This was wrong and has never worked that way. Now it does. Now the object is constructed properly with placement new and the member initialization within the struct definition has the intended effect. This also solves potential UB bugs on other compilers like MSVC.

NOTE: I currently don't have the NRF Toolchain setup correctly and thus did not compile it on GCC. Careful testing is thus required.

NOTE: I did not adjust the template hal (fruitymesh\src\hal\template\FruityHalTemplate.cpp) because it seems to be out of date (could not find GetHalMemorySize in it.). The proper implementation for the FruityHalTemplate would be a noop.

siretty commented 3 years ago

Thank you for the contribution! Good catch.

siretty commented 3 years ago

FYI: We are currently running the changes through our internal test setups. I don't see any issues though.

siretty commented 2 years ago

Hi @Brotcrunsher,

I'm sure you're happy to hear that your PR was merged internally. The changes will be merged on GitHub together with the next GitHub release.

For now I'll close the PR.

Kind regards Daniel