dist/cppcheck: fix missing cppcheck-suppression reasons#10010
dist/cppcheck: fix missing cppcheck-suppression reasons#10010miri64 merged 4 commits intoRIOT-OS:masterfrom
Conversation
smlng
added
Type: cleanup
miri64
left a comment
miri64
left a comment
There was a problem hiding this comment.
ACK and go when Murdock is happy
|
Murdock isn't happy though (apparently there is another error in |
|
seems to be the old cppcheck versions in travis and murdock, my macOS cppcheck (version 1.84) is totally fine with all the changes. |
|
@miri64 fixed travis and murdock complains, may I squash? |
dist/tools/tunslip/tunslip6.c
Outdated
| /* need to consider `\0` terminator and strlen("dev") = 5, hence 6 | ||
| * cppcheck-suppress bufferAccessOutOfBounds | ||
| * (reason: seems to be a bug in cppcheck 1.7x) */ | ||
| strncat(t, dev, sizeof(t) - 6); |
There was a problem hiding this comment.
I think the comment on top is completely wrong (the tool obviously works without the fix). Anyway, this should rather go in a separate PR so it can be independently tested.
There was a problem hiding this comment.
just because it works, the comment is wrong?
Looking at http://www.cplusplus.com/reference/cstring/strncat/ it states that the destination must be large enough to include both strings plus the \0 terminator, so the calculation in the comment is correct, or not. One could rewrite this to sizeof(t) - (strlen(dev) + 1) to make it more obvious, but why bother to run strlen(dev) when we know its fixed to 5.
There was a problem hiding this comment.
Please let's discuss this in a separate PR. Regardless of right or wrong, this change does not belong here.
dist/tools/tunslip/tunslip6.c
Outdated
| /* need to consider `\0` terminator and strlen("dev") = 5, hence 6 | ||
| * cppcheck-suppress bufferAccessOutOfBounds | ||
| * (reason: seems to be a bug in cppcheck 1.7x) */ | ||
| strncat(t, dev, sizeof(t) - 6); |
There was a problem hiding this comment.
Also: cppcheck still isn't happy with this line: https://ci.riot-os.org/RIOT-OS/RIOT/10010/acdbd77214e4aefa76f539075643591cc64d8bc2/output.html
There was a problem hiding this comment.
strange, I simply squashed and rebased, why was murdock happy before?
|
removed the controversy 😬 Both CI look happy, squash? |
|
happy besides fixup commits, that is. |
Yes please! |
smlng
force-pushed
the
pr/fix/cppcheck-suppress
branch
from
3e5e610 to
a4ef4cc
Compare
|
to verify run so should resolve #1895 |
|
Good point! Found one remaining tiny deviation ;-): |
Adding and correcting description/rational on why certain cppcheck warnings or errors are intentionally suppressed.
Extend the cppcheck suppression example to show that each suppression should have a reason describing the intentional suppression of a cppcheck warning or error.
Cppcheck was (correctly) warning here that concat to strings might result in buffer overflow because the terminating `\0` was not considered. This is fixed here, making the cppcheck suppression also obsolete.
This was found by cppcheck 1.6x used in Travis-CI, however this seems to be a false positive, as it is not found by newer versions of cppcheck.
smlng
force-pushed
the
pr/fix/cppcheck-suppress
branch
from
a4ef4cc to
5b1cd04
Compare
|
amended fix (colon) directly |
|
Thanks for finally tackling this! |
3 participants
Contribution description
Fixes #1895
This adds or corrects missing rationale on existing cppcheck-suppression comments.
Testing procedure
run
git grep -A1 cppcheck-suppressionon master and review if the line belowcppcheck-suppress <something>looks like* (reason: <explanation>) */and compare with this PR, which should fix all mismatches.Issues/PRs references