-
-
Notifications
You must be signed in to change notification settings - Fork 133
Fix stack buffer overflow in String::getBytes() test #193
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
Conversation
Use string concatenation instead of creating a list.
CLA assistant check
All committers have signed the CLA.
codecov-commenter
commented
Jul 31, 2023
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
@@ Coverage Diff @@ ## master #193 +/- ## ======================================= Coverage 95.77% 95.77% ======================================= Files 13 13 Lines 970 970 ======================================= Hits 929 929 Misses 41 41
☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thanks for catching this @tttapa
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 ;) )
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).
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?
[String-getBytes-02]test.CMAKE_{C,CXX}_FLAGSvariable intest/CMakeLists.txt(string concatenation instead of list concatenation).This was caught by the GCC sanitizers. It might be a good idea to run the tests with
-fsanitize=address,undefinedin the CI (in addition to Valgrind) to catch these kinds of bugs early.