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

Add some simple fixes to possible bugs identified with cppcheck #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aiden-ct
Copy link

@aiden-ct aiden-ct commented Oct 28, 2020

Run cppcheck --force . to see full list of errors and warnings identified. Needs some configuration to parse the source fully.

@schaecsn
Copy link

I'm somewhat against automatically fixing code.

Let's have a look at "Add some protection against NULL pointer dereference".

if aad_len > 0 then aad should be not NULL. Let's assume that is the case, then the current code would crash, we notice that, root-cause and fix it.

With the proposed fix, we would skip the if block and happily continue execution. But doesn't the following code assume that the skipped if block got actually executed? Would that not result in erronous computations?

This issue should be fixed manually by verifying function arguments, e.g. the beginning of the function should check that if aad_len > 0 then aad != NULL and return -EINVAL otherwise.

And so every potential issue should be manually inspected and fixed the right way.

@aiden-ct
Copy link
Author

@schaecsn - these patches weren't generated automatically, but I take your point on that specific example. I've updated the PR to reflect.

@gamelaster gamelaster self-requested a review October 30, 2020 11:38
Avamander pushed a commit that referenced this pull request Nov 15, 2020
Clean components: fix whitespace in components/
@gamelaster gamelaster force-pushed the master branch 2 times, most recently from 5acb2ea to 3c827b2 Compare November 19, 2020 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants