Skip to content

Resolve static check warnings in example code#5142

Merged
pks-t merged 1 commit intolibgit2:masterfrom
scottfurry:StaticChkFixExamples
Jun 27, 2019
Merged

Resolve static check warnings in example code#5142
pks-t merged 1 commit intolibgit2:masterfrom
scottfurry:StaticChkFixExamples

Conversation

@scottfurry
Copy link
Contributor

Using cppcheck on libgit2 sources indicated two warnings in example code.

merge.c was reported as having a memory leak. Fix applied was to apply git_commit_free to value of variable parents.

init.c was reported as having a null pointer dereference on variable arg. Function usage was being called with a null variable. Changed supplied parameter to empty string.

Copy link
Member

@pks-t pks-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution! The usage fix is obviously correct, the other one needs some amends though


if (!o->dir)
usage("must specify directory to init", NULL);
usage("must specify directory to init", "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, passing NULL to fprintf is definitely wrong

examples/merge.c Outdated
check_lg2(git_repository_head(&head_ref, repo), "failed to get repo HEAD", NULL);
if (resolve_refish(&merge_commit, repo, opts->heads[0])) {
fprintf(stderr, "failed to resolve refish %s", opts->heads[0]);
git_commit_free(*parents);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to call free(parents) here. We haven't yet populated the parents array, only allocated the array itself

@scottfurry
Copy link
Contributor Author

scottfurry commented Jun 27, 2019

Reading on free usage, makes sense. parents contents were only allocated. Changes made.

examples/merge.c Outdated
check_lg2(git_repository_head(&head_ref, repo), "failed to get repo HEAD", NULL);
if (resolve_refish(&merge_commit, repo, opts->heads[0])) {
fprintf(stderr, "failed to resolve refish %s", opts->heads[0]);
free(*parents);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to free(parents), though, not the the first element of the parents array :) Note the missing *

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't thought this low-level in awhile but I I think I get it now.

There's a reason I took "low hanging fruit" for a fix.

Using cppcheck on libgit2 sources indicated two warnings in
example code.

merge.c was reported as having a memory leak. Fix applied
was to `free()` memory pointed to by `parents`.

init.c was reported as having a null pointer dereference
on variable arg. Function 'usage' was being called with
a null variable. Changed supplied parameter to empty string.
Copy link
Member

@pks-t pks-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @scottfurry, looks good to me now!

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