Assorted Coverity fixes by tiennou · Pull Request #4702 · libgit2/libgit2

tiennou · 2018-06-25T21:36:17Z

This fixes May/June Coverity reports.

ethomson · 2018-06-27T09:48:52Z

It looks like the smart pkt changes are causing test failures - could you take a look?

tiennou · 2018-06-27T12:37:49Z

Rebased. Note that I had to partially revert a recent change because of the allocation semantics of symrefs (it's allocated/populated in smart_protocol.c/detect_caps, but its destructor is in smart.c).
So please don't merge, event though tests should be ok now.

tiennou · 2018-06-27T19:17:08Z

That should be ok now. Feel free to after reviewing.

pks-t · 2018-07-06T08:00:31Z

-	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) {


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; }

pks-t · 2018-07-06T08:01:53Z

 	}

-	if (opts.flags & GIT_BLAME_USE_MAILMAP)
-		git_mailmap_from_repository(&gbr->mailmap, repo);


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

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 ?

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)

pks-t · 2018-07-06T08:03:36Z

 			giterr_set(GITERR_NOMEMORY, "error inserting submodule into hash table");
-			return -1;
+			error = -1;
+			goto out;


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

pks-t · 2018-07-06T08:04:03Z

+		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;


Indentation is off here

pks-t · 2018-07-06T08:04:08Z

+        error = 0;
+	} else {
+        giterr_set(GITERR_NET, "invalid response");
+        goto cleanup;


And here as well

pks-t · 2018-07-06T08:07:07Z

-	if (git_smart__detect_caps(first, &t->caps, &symrefs) < 0) {
-		free_symrefs(&symrefs);
-		return -1;
-	}


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

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.

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…

pks-t · 2018-07-06T08:08:33Z

 	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"));


I'd love to hear some details why this is necessary. Tests passed before, so why is it needed now?

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.

Thanks for the explanation!

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.

pks-t · 2018-07-13T07:52:06Z

-	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) {


This change is wrong. We're new erroring out when the function ran successfully. It's getting fixed by the next commit, though

pks-t · 2018-07-13T07:57:02Z

 	/* No refs or capabilites, odd but not a problem */
 	if (pkt == NULL || pkt->capabilities == NULL)
-		return 0;
+		return GIT_ENOTFOUND;


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

pks-t · 2018-07-20T10:57:17Z

Thank you for your fixes, @tiennou!

tiennou force-pushed the fix/coverity branch 2 times, most recently from e227b64 to 1244f80 Compare June 27, 2018 12:35

tiennou force-pushed the fix/coverity branch from 1244f80 to 6ce4da8 Compare June 27, 2018 19:15

tiennou force-pushed the fix/coverity branch from 6ce4da8 to 6a13527 Compare July 1, 2018 10:04

pks-t requested changes Jul 6, 2018

View reviewed changes

tiennou added 7 commits July 6, 2018 22:13

blame: check error code when loading the mailmap

ca9bbcb

Reported by Coverity, CID 1393484

tests: mailmap/parsing: add a missing cl_git_pass

17bd3b9

Reported by Coverity, CID 1393483

submodule: don't leak memory when failing to insert the names

36a5b55

Reported by Coverity, CID 1393237

smart: clarify error handling in git_smart__connect

68c7480

smart: don't dereference a NULL pkt pointer

6ae6491

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

tests: add missing cl_git_pass to tests

8455a27

Reported by Coverity, CID 1393678-1393697.

tiennou force-pushed the fix/coverity branch from 73fd88e to 89091d3 Compare July 6, 2018 20:25

pks-t approved these changes Jul 13, 2018

View reviewed changes

pks-t merged commit 0652aba into libgit2:master Jul 20, 2018

tiennou deleted the fix/coverity branch July 20, 2018 11:05

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Conversation

tiennou commented Jun 25, 2018

Uh oh!

ethomson commented Jun 27, 2018

Uh oh!

tiennou commented Jun 27, 2018

Uh oh!

tiennou commented Jun 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t commented Jul 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants