patch, 11 partsCoverity fixes

The release window is a fine time to concentrate on bug fixes, and Coverity can help identify bugs. These are only a couple of fixes, Coverity reported many more, but most of the reports seemed either bogus or harmless (such as resource leaks in test helpers).

Johannes Schindelin (11):
  mingw: avoid accessing uninitialized memory in `is_executable()`
  fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()`
  submodule--helper: avoid memory leak in `update_submodule()`
  get_oid_with_context_1(): avoid use-after-free
  submodule-config: avoid memory leak
  pack-redundant: avoid using uninitialized memory
  submodule--helper: avoid memory leak when fetching submodules
  read_index_from(): avoid memory leak
  pack-mtimes: avoid closing a bogus file descriptor
  relative_url(): fix incorrect condition
  bug_fl(): add missing `va_end()` call

 builtin/pack-redundant.c    | 1 +
 builtin/submodule--helper.c | 3 +++
 fsmonitor-settings.c        | 8 ++++++--
 object-name.c               | 6 ++++--
 pack-mtimes.c               | 3 ++-
 read-cache.c                | 6 +++---
 remote.c                    | 2 +-
 run-command.c               | 2 +-
 submodule-config.c          | 8 ++++----
 usage.c                     | 1 +
 10 files changed, 26 insertions(+), 14 deletions(-)

base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1264%2Fdscho%2Fcoverity-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1264/dscho/coverity-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1264

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, we open files we suspect might be scripts, read the first two bytes, and see whether they indicate a hash-bang line. We do not initialize the byte _after_ those two bytes, therefore `strcmp()` is inappropriate here.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 run-command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

This has been with us since cc3b7a97 (Windows: Make 'git help -a' work., 2008-01-14) and apparently nobody made loud enough noises to make us aware since then.

The fix is trivially correct, of course.

Will queue.

Am 16.06.22 um 01:35 schrieb Johannes Schindelin via GitGitGadget:

Hmm, but buf _is_ initialized fully?  Line 149:

        char buf[3] = { 0 };

C99 §6.7.8 21: "If there are [...] fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration."

René Scharfe <l.s.r@web.de> writes:

Ahh, yeah, that changes the landscape quite a bit.

We explicitly ask to read 2 bytes and look at the buf[] when read says it read 2 bytes, so this is another false positive X-<.

We can update the variable to

	char buf[2];

to match the updated code, I guess. The fewer bytes we use on stack, the better ;-).

Junio C Hamano <gitster@pobox.com> writes:

Just for reference, this piece of code has been correct ever since it was introduced to help.c by cc3b7a97 (Windows: Make 'git help -a' work., 2008-01-14). With a larger context, we can see buf[3] that is NUL filled, which receives 2 bytes by read(), and strcmp() does look semantically more correct than memcmp(), even though there probably shouldn't be any correctness or performance difference.

So, strcmp -> memcmp in this case is a strict dis-improvement. I merged it already to 'next', but I haven't pushed the result out, so I'll redo 'next' before I push the result out.

Thanks.

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 fsmonitor-settings.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Obviously correct, but the placement of these ...

... braces are misleading and confusing.

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/submodule--helper.c | 2 ++
 1 file changed, 2 insertions(+)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

This function has two very similar code block that computes prefixed_path depending on the same condition, and one frees the variable correctly while the other one (i.e. this one) forgets to do so, which is irritating to see.

Perhaps the whole "we have update_data structure, in which recursive_prefix, sm_path and prefix members in it; please set the displaypath member based on these values" should become a helper function, e.g.

	static const char *displaypath_from_update_data(struct update_data *u)
	{
		char *pp, *ret;

		if (u->recursive_prefix)
			pp = xstrfmt("%s%s", u->recursive_prefix, u->sm_path);
		else
			pp = xstrdup(u->sm_path);

		ret = get_submodule_displaypath(pp, u->prefix);
		free(pp);
		return ret;
	}

to avoid duplicated computation.

But the whole thing may become moot, as there seems to be a move to get rid of submodule--helper.c altogether?

I'll refrain from touching this patch and instead redirect it to Glen; perhaps removal of submodule--helper.c involves moving the code here to another file or something, in which case it is far easier if I outsource that to somebody who is actually working on the file ;-)

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Good eye, I hadn't noticed this.

In this case, we're computing the same thing twice in the function - first unconditionally, then conditionally. So we can actually just reuse the first result by doing something like

		next.recursive_prefix = xstrfmt("%s/", update_data->displaypath);

(note that we need to append a "/" at the end)

Even better, I recently noticed that .recursive_prefix can probably just be replaced with "--super-prefix", and that will let us dispatch to get_submodule_displaypath() without this extra dance here.

I'll flesh that out into a series and send it soon.

cc-ed Ævar, who knows even more than I do :)

git-submodule.sh is definitely going away soon \o/ but I don't think we have plans to get rid of submodule--helper.c just yet, so I think we should probably still keep this patch.

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 561287d342cc (object-name: reject trees found in the index, 2022-04-26), we changed the code to release the memory stored under the `new_path` variable a bit earlier.

However, at that point the variable `cp` still points to `new_path` (if non-`NULL`), and we _then_ pass that pointer to `reject_tree_in_index()`.

Let's make sure that the pointer still points to allocated memory by moving the original `free()` call back where it was, and adding another one in the early return code path.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 object-name.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Hmph. Without the "lets make sure we do not leak in the error code path", it would have been no brainer to avoid this bug in the original version. Of course the postimage of the above hunk is correct, but with extra free() sprinkled, it became ugly to the eye.

I wonder if the following is easier to follow.

Thanks.

 object-name.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 961b130d20c9 (branch: add --recurse-submodules option for branch creation, 2022-01-28), a funny pattern was introduced where first some struct is `xmalloc()`ed, then we resize an array whose element type is the same struct, and then the first struct's contents are copied into the last element of that array.

Crucially, the `xmalloc()`ed memory never gets released.

Let's avoid that memory leak and that memory allocation dance altogether by first reallocating the array, then using a pointer to the last array element to go forward.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 submodule-config.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Sigh. The original is butt ugly, with this strange pattern and structure assignments etc. I wonder how something like this slipped through our reviews.

I wonder if it would help for me to stop trusting reviews by less experienced reviewers too much, and instead give sanity checks to more patches myself from now on, but I certainly cannot afford the time and my mental health to do so for all the patches X-<.

Will queue.

Junio C Hamano <gitster@pobox.com> writes:

Gah, I have no excuses for bad code I've already written, I can only strive to write better code next time.

Thanks Johannes for spotting and fixing this.

I seem to recall that this was reviewed by fairly experienced folks. I'm guessing it slipped through due to reviewer fatigue, which might be a good argument for adding more tooling to lighten reviewer load/patch up the gaps.

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the `add_pack()` function, we forgot to initialize the field `next`, which could potentially lead to readin uninitialized memory later.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pack-redundant.c | 1 +
 1 file changed, 1 insertion(+)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

		return pack_list_insert(&altodb_packs, &l);

The pack_list_insert reads like so:

static inline struct pack_list * pack_list_insert(struct pack_list **pl,
					   struct pack_list *entry)
{
	struct pack_list *p = xmalloc(sizeof(struct pack_list));
	memcpy(p, entry, sizeof(struct pack_list));
	p->next = *pl;
	*pl = p;
	return p;
}

IOW, we prepare members of "l", pass it as "entry" to pack_list_insert(), but the helper function allocates a new piece of memory, copies "l" to it, and then sets the .next member of it to a reasonable value.

And after that, the "l" that was merely used as a template goes out of scope.

So I think this is a false positive. memcpy(p) does copy the uninitialized garbage that is held in entry->next to p->next, but it is overwritten with a reasonable address immediately after that.

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In c51f8f94e5b3 (submodule--helper: run update procedures from C, 2021-08-24), we added code that first obtains the default remote, and then adds that to a `strvec`.

However, we never released the default remote's memory.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/submodule--helper.c | 1 +
 1 file changed, 1 insertion(+)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Trivially correct.  Will queue.

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 998330ac2e7c (read-cache: look for shared index files next to the index, too, 2021-08-26), we added code that allocates memory to store the base path of a shared index, but we never released that memory.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 read-cache.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Hi,

I think there is another problem with this code: later on it refers to the 'base_path' variable, but I think it should refer to 'base_path2' if the first path did not exist.

How about the following patch instead (or in addition)?

Regards, Tom Levy

-- >8 --

Subject: [PATCH] read_index_from(): use second path consistently, avoid memory
 leak

In 998330ac2e7c (read-cache: look for shared index files next to the index, too, 2021-08-26), we added code that allocates memory to store the path of a second shared base index, but we never released that memory. Also, later code (e.g. the call to freshen_shared_index()) continued to use the first path even when the second path was selected.

Modify the code to store the second path in the original variable (after freeing the first path), so that the later code will use the second path (and also free it).

The memory leak was reported by Coverity.

Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Tom Levy <tomlevy93@gmail.com>
---
 read-cache.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 94cd775a6c52 (pack-mtimes: support reading .mtimes files, 2022-05-20), code was added to close the file descriptor corresponding to the mtimes file.

However, it is possible that opening that file failed, in which case we are closing a file descriptor with the value `-1`. Let's guard that `close()` call.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 pack-mtimes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On Wed, Jun 15, 2022 at 11:35:43PM +0000, Johannes Schindelin via GitGitGadget wrote:

Nice catch. I agree with your assessment and fix. Thanks for spotting and fixing!

Thanks, Taylor

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 63e95beb085c (submodule: port resolve_relative_url from shell to C, 2016-04-15), we added a loop over `url` where we are looking for `../` or `./` components.

The loop condition we used is the pointer `url` itself, which is clearly not what we wanted.

Pointed out by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

8/11, 9/11, and 10/11 are trivially correct.  Will queue.

Thanks.

On Wed, Jun 15 2022, Johannes Schindelin via GitGitGadget wrote:

Clearly, but having looked at this I think it's useful to note that coverity must be (I don't know what it actually emitted) be complaining about the "url" condition being useless, i.e. it's the same as a "while (1)", not that we run off the end of the string.

I.e. due to the way those start_with*() functions work we would abort early if we were running off th end of the string.

Which I tested with this:
	
	diff --git a/remote.c b/remote.c
	index 9b9bbfe80ec..e049bbb791c 100644
	--- a/remote.c
	+++ b/remote.c
	@@ -2846,14 +2846,17 @@ char *relative_url(const char *remote_url, const char *url,
	 	 * When the url starts with '../', remove that and the
	 	 * last directory in remoteurl.
	 	 */
	-	while (url) {
	+	while (1) {
	 		if (starts_with_dot_dot_slash_native(url)) {
	 			url += 3;
	 			colonsep |= chop_last_dir(&remoteurl, is_relative);
	-		} else if (starts_with_dot_slash_native(url))
	+		} else if (starts_with_dot_slash_native(url)) {
	 			url += 2;
	-		else
	-			break;
	+		} else if (!*url) {
	+			BUG("ran off the end of our url?");
	+		} else {
	+			break; 
	+		}
	 	}
	 	strbuf_reset(&sb);
	 	strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
	
Which will fail e.g. on this test in t3420-rebase-autostash.sh:

	+ git submodule add ./ sub
	BUG: remote.c:2856: ran off the end of our url?
	Aborted

I worried a bit about this since we released this with v2.9.0, so in all this time we haven't been infinitely looping on this case, or at least haven't had any reports about that.

So if we hadn't been catching this in starts_with_*() I wouldn't be confident that this was the correct fix, yes it's almost certainly not what not what was intended, but if we change it to that are we confident that the side-effects are going to be what we want?

But in this case I'm pretty sure that the behavior before/after will be the same, and that the only change will be to make coverity happier, and the code less confusing.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Sorry, but I am confused. I do not see the point of that test before the BUG.

I agree that this loop over url is "we want to loop forever, and stop immediately when url does not start with ../ or ./" infinite loop, and it is better written with "while(1)". If you do not make any other changes that confuses me, we'll get this:

-	while (*url) {
+	while (1) {
		if (starts_with_dot_dot_slash_native(url)) {
			url += 3;
			colonsep |= chop_last_dir(&remoteurl, is_relative);
		} else if (starts_with_dot_slash_native(url))
			url += 2;
		else
			break;
	}

We know url upon entry is not NULL. starts_with_dot_*() ends up calling dir.c:path_match_flags() and the first thing it does is to return if the byte is not '.'. So the difference is whether you leave the loop with the "while" condition, or you fail the two starts_with_dot_*() calls and hit the "break" at the end in the loop.

IOW, running of the end of the URL is not a BUG, is it?

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

		} else if (starts_with_dot_slash_native(url))
			url += 2;
		else
			break;
	}

Looking at the "problematic" code again, the original is bad in the sense that it is utterly misleading, but not quite "incorrect", I would think.

In other words, what the original wanted to write is an infinite loop that uses an explicit "break" to terminate the iteration, so we should have written "while (1)" in the first place.

We know that earlier part of the function already depends on url being not NULL, and nothing in the loop makes it NULL. *url that is NUL would make the two starts_with_frotz() tests fail and causes the loop to terminate by hitting "break", so while it is correct to check *url, it is wasting cycles.

But this patch is not wrong per-se, so I still have it in my tree ;-)

Thanks.

From: Johannes Schindelin <johannes.schindelin@gmx.de>

According to the manual:

	Each invocation of va_copy() must be matched by a corresponding
	invocation of va_end() in the  same function.

Note: There is another instance of `va_copy()` in `usage.c` that is
missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,
though, because that function either `exit()`s or `abort()`s, anyway.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 usage.c | 1 +
 1 file changed, 1 insertion(+)

On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote:

This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal bug() function to go with BUG(), 2022-06-02). But there's a much worse bug in the same function. The code introduced by that patch does:

  va_list ap, cp;
  [...]
  va_copy(cp, ap);
  va_start(ap, fmt);

So "cp" is copied from "ap" before we have actually initialized "ap". It's surprising that this works at all. The two lines should be flipped.

IMHO, since we're in the actual varargs function itself, it would be simpler to just bracket each use with start/end, rather than copying, like:

Jeff King <peff@peff.net> writes:

Yes, it is surprising.  Perhaps it is not working at all.

Thanks for an extra set of eyeballs.

On Wed, Jun 15 2022, Junio C Hamano wrote:

It's "working" now in the sense that we run this code, and even test that trace2 output specifically, see the tests in 0cc05b044fd (usage.c: add a non-fatal bug() function to go with BUG(), 2022-06-02).

But obviously that's a bad use of the varargs API, I just don't know how we've been getting away with it in practice, sorry about that.

The fix Peff's got here LGTM. I can (re)submit it with format-patch+send-email after giving it a commit message describing the issue if you'd like, but the change would be the same.

I'm also happy for you to pick up the upthread directly, but I don't see that you did that already in one of the integration branches (but perhaps I missed one).

Just let me know, but I didn't want to re-submit this without asking, in case I'd step on any toes (or submit something in duplicate).

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Exactly. We three all expressed our surprises on why it "works". Nobody offered an explanation, though, which leaves us in suspense ;-)

Yup, I think the code change there looks the most sensible.

Thanks, all.

On Thu, Jun 16, 2022 at 11:03:25AM -0700, Junio C Hamano wrote:

Being the curious sort, I ran it in a debugger. And indeed, "cp" is filled with uninitialized garbage. The reason it works is that the test calls bug() with a format string that does not contain any placeholders. And thus our eventual call to vsnprintf() does not ever look at "cp" at all!

I'll wrap it up with a commit message and modify the test to be more thorough.

-Peff

On Thu, Jun 16, 2022 at 03:20:08PM -0400, Jeff King wrote:

Here it is. This can replace Johannes's patch 11.

-- >8 --
Subject: bug_fl(): correctly initialize trace2 va_list

The code added 0cc05b044f (usage.c: add a non-fatal bug() function to go with BUG(), 2022-06-02) sets up two va_list variables: one to output to stderr, and one to trace2. But the order of initialization is wrong:

  va_list ap, cp;
  va_copy(cp, ap);
  va_start(ap, fmt);

We copy the contents of "ap" into "cp" before it is initialized, meaning it is full of garbage. The two should be swapped.

However, there's another bug, noticed by Johannes Schindelin: we forget to call va_end() for the copy. So instead of just fixing the copy's initialization, let's do two separate start/end pairs. This is allowed by the standard, and we don't need to use copy here since we have access to the original varargs. Matching the pairs with the calls makes it more obvious that everything is being done correctly.

Note that we do call bug_fl() in the tests, but it didn't trigger this problem because our format string doesn't have any placeholders. So even though we were passing a garbage va_list through the stack, nobody ever needed to look at it. We can easily adjust one of the trace2 tests to trigger this, both for bug() and for BUG(). The latter isn't broken, but it's nice to exercise both a bit more. Without the fix in this patch (but with the test change), the bug() case causes a segfault.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-trace2.c | 4 ++--
 usage.c                | 8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

Jeff King <peff@peff.net> writes:

;-)  Thanks.

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Well, it certainly is better than random shiny new features or refactoring for the sake of refactoring, but finding and fixing a bug that has been with us since forever without anybody noticing is not all that ultra-urgent, either ;-)

It is a good time to concentrate on finding and fixing regressions.

Thanks.