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

[Bug]: More potential non aligned memory accesses (focused on RP2040 but not only) #4911

Closed
TheMalkavien opened this issue Sep 30, 2024 · 0 comments · Fixed by #4912
Closed
Assignees
Labels
bug Something isn't working

Comments

@TheMalkavien
Copy link
Contributor

TheMalkavien commented Sep 30, 2024

Category

Hardware Compatibility

Hardware

Other

Firmware Version

2.5+

Description

Hi,

After the issue #4855 was resolved by my little patch, I was thinking this was just the visible part. After discussing it on the discord I tried the -Wcast-align gcc flag which throws a warning when the code uses some potential non aligned memory pointer.
The potential is important here because it checks at the compilation time, not run time, so it does not know if it will unaligned or not, it just knows it could be depending on the rest of the code.

The result is interesting, some warning have to be fixed, some not (imho). I will not paste here all the warnings, because there is 90 of them when compiling for the rp2040. Il will just put them by files :

  1. .pio\libdeps\rp2040-lora\Crypto\ChaCha.cpp : 3 warning
  2. .pio/libdeps/rp2040-lora/INA3221/src/INA3221.cpp : 77 warnings
  3. .pio\libdeps\rp2040-lora\RadioLib\src\modules\LR11x0\LR11x0.cpp : 2 warnings
  4. /src/gps/GPS.cpp : 1 warning
  5. /src/gps/NMEAWPL.cpp : 2 warnings
  6. /src/mesh/aes-ccm.cpp : 2 warnings
  7. /src/mesh/CryptoEngine.cpp : 1 warning
  8. /src/mesh/RadioInterface.cpp : 1 warning
  9. /src/mesh/RadioLibInterface.cpp : 1 warning

At first, I thought it was a hard work to come. But I suggest we do not care about warnings in the libraries. We can trust the library to "know better than me how to handle pointers". That means we have to be careful of what we gave it in input.
Then I suggest not to touch aes-ccm, as it contrains important crypto functions, I saw the copyright so this is maybe a library too. Trying to change a crypto function may lead to very big problems, I prefer to avoid this one.

I will focus on the others, which will be profitable for others platform as well :
4. /src/gps/GPS.cpp : 1 warning
5. /src/gps/NMEAWPL.cpp : 2 warnings
7. /src/mesh/CryptoEngine.cpp : 1 warning
8. /src/mesh/RadioInterface.cpp : 1 warning
9. /src/mesh/RadioLibInterface.cpp : 1 warning

With very little effort, I think we can fix 6 warnings, but more important with some simple change we can better sanitize a lot of buffer accesses, especially the radiobuffer.

I'll make a PR very soon to show you what I am thinking of so you will decide what to do on this topic.

Regards,

Relevant log output

No response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants