-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Assorted Coverity fixes #4702
Assorted Coverity fixes #4702
Conversation
It looks like the smart pkt changes are causing test failures - could you take a look? |
e227b64
to
1244f80
Compare
Rebased. Note that I had to partially revert a recent change because of the allocation semantics of |
That should be ok now. Feel free to |
src/blame.c
Outdated
if (opts.flags & GIT_BLAME_USE_MAILMAP) | ||
git_mailmap_from_repository(&gbr->mailmap, repo); | ||
if (opts.flags & GIT_BLAME_USE_MAILMAP && | ||
git_mailmap_from_repository(&gbr->mailmap, repo) < 0) { |
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.
No need to change that now, but our agreed-upon code style is to indent continuation lines in conditionals with four spaces:
if (opts.flags & GIT_BLAME_USE_MAILMAP &&
git_mailmap_from_repository(&gbr->mailmap, repo) < 0) {
git_blame_free(gbr);
return NULL;
}
@@ -133,8 +133,11 @@ git_blame* git_blame__alloc( | |||
return NULL; | |||
} | |||
|
|||
if (opts.flags & GIT_BLAME_USE_MAILMAP) | |||
git_mailmap_from_repository(&gbr->mailmap, repo); |
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.
I think ignoring the error was actually intended. In case where we're unable to load the mail map, we simply ignore it and output commits with their original author/committer. You could add a comment and cast to void
, though
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.
Ignore the error even though I (the user) has asked for it ? I do not know if that's explicit, but I find this strange… How can I tell if the mailmap was applied when that happens ?
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.
Huh, you're right. And in fact git_mailmap_from_repository
also only errors out in case where git_mailmap_new
fails, but never if getting the actual mailmap fails. So checking the error is the right thing to do, and our future warning API should then handle errors when loading the mailmap itself (as described in mailmap_add_from_repository
)
@@ -260,7 +260,8 @@ static int load_submodule_names(git_strmap **out, git_repository *repo, git_conf | |||
git_strmap_insert(names, entry->value, git_buf_detach(&buf), &rval); | |||
if (rval < 0) { | |||
giterr_set(GITERR_NOMEMORY, "error inserting submodule into hash table"); | |||
return -1; | |||
error = -1; | |||
goto out; |
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.
We usually avoid freeing memory when we run in OOM situations. But as you cannot use GITERR_CHECK_ALLOC
here I'm fine with this fix
src/transports/smart.c
Outdated
git_smart__update_heads(t, &symrefs); | ||
} else if (error == GIT_ENOTFOUND) { | ||
/* There was no ref packet received, or the cap list was empty */ | ||
error = 0; |
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.
Indentation is off here
src/transports/smart.c
Outdated
error = 0; | ||
} else { | ||
giterr_set(GITERR_NET, "invalid response"); | ||
goto cleanup; |
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.
And here as well
if (git_smart__detect_caps(first, &t->caps, &symrefs) < 0) { | ||
free_symrefs(&symrefs); | ||
return -1; | ||
} |
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.
I actually find this commit a bit hard to digest, as it simultaneously moves around code and changes it at the same time. I'd welcome it if you split it up into multiple commits to make it easier to understand what is going on 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.
Sorry, this seemed a simple fix, then memory-management happened. I'll try to split (the crux of the change is that git_smart__detect_caps
returns GIT_ENOTFOUND
if it stumble on a weird pkt, which allows its caller to know if the symrefs
it got are valid or not.
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.
Thanks :)
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.
Hopefully the commits are more separated now, which clarifies what is happening. Note that I erroneously said symref
above, but my understanding of the issue is that it might (maliciously) happen that the remote sent back one ref (refs is a vector of git_pkt in this code 😮) but doesn't follow through, so this strcmp
will segfault on a NULL.
Thinking back a bit more, I don't know if that's plausible protocol-wise, but at least coverity should be pleased…
tests/describe/t6120.c
Outdated
@@ -104,7 +104,7 @@ static void commit_and_tag( | |||
|
|||
cl_git_append2file("describe/file", "\n"); | |||
|
|||
cl_git_pass(git_index_add_bypath(index, "describe/file")); | |||
cl_git_pass(git_index_add_bypath(index, "file")); |
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.
I'd love to hear some details why this is necessary. Tests passed before, so why is it needed now?
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.
The path given to git_index_add_bypath
is relative to the root of the repository. That describe/file
path is relative to the root of the sandbox directory, hence if I add the missing cl_git_pass
I rightfully get an error that $SANDBOX/describe/describe/file doesn't exist
. The path is thus changed to be made relative to the repository, which makes the failure go away and "restore" the test.
Hence the test doesn't work as expected, because it does not care about file
being added or not to the index, which makes dubious in my eyes.
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.
Thanks for the explanation!
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.
I'll put it in the commit message then 😉. I still feel like there's something dubious going on though, but this is about coverity.
Reported by Coverity, CID 1393484
Reported by Coverity, CID 1393483
Reported by Coverity, CID 1393237
By clarifying what detect_caps returns on empty/missing packet, we can be sure there are actually refs to process. The old code could blindly dereference `first`, which might have been NULL. Reported by Coverity, CID 1393614
Reported by Coverity, CID 1393678-1393697.
The path given to `git_index_add_bypath` is relative to the root of the repository. That `describe/file` path is relative to the root of the sandbox directory, hence if I add the missing `cl_git_pass` I rightfully get an error that `$SANDBOX/describe/describe/file doesn't exist`. The path is thus changed to be made relative to the repository, which makes the failure go away and "restore" the test.
src/transports/smart.c
Outdated
if (git_smart__detect_caps(first, &t->caps, &symrefs) < 0) { | ||
free_symrefs(&symrefs); | ||
return -1; | ||
if ((error = git_smart__detect_caps(first, &t->caps, &symrefs)) == 0) { |
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.
This change is wrong. We're new erroring out when the function ran successfully. It's getting fixed by the next commit, though
@@ -142,7 +142,7 @@ int git_smart__detect_caps(git_pkt_ref *pkt, transport_smart_caps *caps, git_vec | |||
|
|||
/* No refs or capabilites, odd but not a problem */ | |||
if (pkt == NULL || pkt->capabilities == NULL) | |||
return 0; | |||
return GIT_ENOTFOUND; |
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.
This is fine, as there is only one caller of git_smart__detect_caps
, which is the one you're adjusting in the same commit
Thank you for your fixes, @tiennou! |
This fixes May/June Coverity reports.