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

Options in new_messenger() must never be null. #274

Merged
merged 1 commit into from Nov 15, 2016

Conversation

GrayHatter
Copy link

@GrayHatter GrayHatter commented Nov 15, 2016

cppcheck complains about being redundant or incorrcet. I'd rather be correct.


This change is Reviewable

@GrayHatter GrayHatter added this to the v0.0.5 milestone Nov 15, 2016
@nurupo
Copy link
Member

nurupo commented Nov 15, 2016

toxcore/Messenger.c, line 2740 at r1 (raw file):

/*  return size of the messenger data (for saving) */
uint32_t m(const Messenger *m)

Huh?


Comments from Reviewable

@GrayHatter
Copy link
Author

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


toxcore/Messenger.c, line 2740 at r1 (raw file):

Previously, nurupo wrote…

Huh?

Done.

Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Nov 15, 2016

(Doing @GrayHatter's work of writing a proper PR description)

For anyone reading this who didn't follow our IRC conversation, cppcheck and clang scan-build started to warn about potential null pointer deference in new_messenger() [cppcheck], [scan-build]. These complains are caused by the 82515f9 commit. It has introduced a

if (options && options->log_callback) {

check to new_messenger(), which makes static analyzers consider the case that options might actually be NULL. There are execution paths in new_messenger() where options is dereferenced without checking if it is NULL or not, which causes the warnings.

This PR adds a function-wide check to make sure options is not NULL. This will remove the confusion static analyzers are having, as well as anyone reading new_messenger() might have, since static analyzers complain for a valid reason as this is a really smelly code.

@nurupo
Copy link
Member

nurupo commented Nov 15, 2016

:lgtm_strong:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf iphydf merged commit 8822f59 into TokTok:master Nov 15, 2016
@GrayHatter GrayHatter deleted the static_clean branch November 15, 2016 23:27
@iphydf iphydf changed the title Options in new_messenger() must never be null. Options in new_messenger() must never be null. Dec 2, 2016
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.

None yet

3 participants