arduino / ArduinoCore-API

Hardware independent layer of the Arduino cores defining the official API
https://www.arduino.cc/reference/en/
GNU Lesser General Public License v2.1
216 stars 120 forks source link

Fix stack buffer overflow in String::getBytes() test #193

Closed tttapa closed 1 year ago

tttapa commented 1 year ago

This was caught by the GCC sanitizers. It might be a good idea to run the tests with -fsanitize=address,undefined in the CI (in addition to Valgrind) to catch these kinds of bugs early.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

codecov-commenter commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (5b9faf6) 95.77% compared to head (363c2c4) 95.77%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #193 +/- ## ======================================= Coverage 95.77% 95.77% ======================================= Files 13 13 Lines 970 970 ======================================= Hits 929 929 Misses 41 41 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

aentinger commented 1 year ago

This was caught by the GCC sanitizers. It might be a good idea to run the tests with -fsanitize=address,undefined in the CI (in addition to Valgrind) to catch these kinds of bugs early.

@tttapa care to send another PR adding -fsanitize=address,undefined to CMakelists.txt? (I could do it, but for bragging rights ;) )

tttapa commented 1 year ago

I appreciate the offer of bragging rights :) but I'm afraid I don't have the time right now.
Since you can't have the sanitizers enabled when running under valgrind, this would be a nontrivial change to https://github.com/arduino/cpp-test-action (you'd need one build with sanitizers, and one without for valgrind).

aentinger commented 1 year ago

Hi @tttapa ☕ 👋

I've created a feature request for arduino/cpp-test-action. Once the action incorporates that feature we could just use the action twice in our unit-test.yml, to once run with valgrind and once without it (but with sanitizing enabled). What do you think?