Coverity 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 <[email protected]>

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 <[email protected]>

---
 run-command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

run-command.c +1 -1

index 14f17830f51..2ba38850b4d 100644
@@ -154,7 +154,7 @@ int is_executable(const char *name)
 		n = read(fd, buf, 2);
 		if (n == 2)
 			/* look for a she-bang */
-			if (!strcmp(buf, "#!"))
+			if (!memcmp(buf, "#!", 2))
 				st.st_mode |= S_IXUSR;
 		close(fd);
 	}

From: Johannes Schindelin <[email protected]>

Reported by Coverity.

Signed-off-by Johannes Schindelin <[email protected]>

---
 fsmonitor-settings.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

fsmonitor-settings.c +0 -0

index 658cb79da01..464424a1e92 100644

fsmonitor-settings.c +0 -0

 	case FSMONITOR_REASON_ERROR:
 		strbuf_addf(&msg,

From: Johannes Schindelin <[email protected]>

Reported by Coverity.

Signed-off-by Johannes Schindelin <[email protected]>

---
 builtin/submodule--helper.c | 2 ++
 1 file changed, 2 insertions(+)

builtin/submodule--helper.c +0 -0

index 5c77dfcffee..d7b8004b933 100644

builtin/submodule--helper.c +0 -0

 		next.prefix = NULL;
 		oidcpy(&next.oid, null_oid());
 		oidcpy(&next.suboid, null_oid());

From: Johannes Schindelin <[email protected]>

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.

submodule-config.c +4 -4

 		if (S_ISGITLINK(name_entry->mode) &&
 		    is_tree_submodule_active(r, root_tree, tree_path)) {
-			st_entry = xmalloc(sizeof(*st_entry));
+			ALLOC_GROW(out->entries, out->entry_nr + 1,
+				   out->entry_alloc);
+			st_entry = &out->entries[out->entry_nr++];
+
 			st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry));
 			*st_entry->name_entry = *name_entry;
 			st_entry->submodule =

@@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r,
 						root_tree))
 				FREE_AND_NULL(st_entry->repo);
 
-			ALLOC_GROW(out->entries, out->entry_nr + 1,
-				   out->entry_alloc);
-			out->entries[out->entry_nr++] = *st_entry;
 		} else if (S_ISDIR(name_entry->mode))
 			traverse_tree_submodules(r, root_tree, tree_path,
 						 &name_entry->oid, out);

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 builtin/pack-redundant.c | 1 +
 1 file changed, 1 insertion(+)

builtin/pack-redundant.c +0 -0

index ed9b9013a5f..1f7da1f68b6 100644

From: Johannes Schindelin <[email protected]>

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.

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 builtin/submodule--helper.c | 1 +
 1 file changed, 1 insertion(+)

builtin/submodule--helper.c +0 -0

index d7b8004b933..9a8248ffe6a 100644

builtin/submodule--helper.c +0 -0

 	}
 
 	return run_command(&cp);

Quoted 4 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

According to the manual:

Quoted 8 lines

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.

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:

usage.c +5 -3

index 79900d0287..56e29d6cd6 100644
@@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
 int bug_called_must_BUG;
 void bug_fl(const char *file, int line, const char *fmt, ...)
 {
-	va_list ap, cp;
+	va_list ap;
 
 	bug_called_must_BUG = 1;
 
-	va_copy(cp, ap);
 	va_start(ap, fmt);
 	BUG_vfl_common(file, line, fmt, ap);
 	va_end(ap);
-	trace2_cmd_error_va(fmt, cp);
+
+	va_start(ap, fmt);
+	trace2_cmd_error_va(fmt, ap);
+	va_end(ap);
 }
 
 #ifdef SUPPRESS_ANNOTATED_LEAKS

but I am happy with any solution that is correct. :)

-Peff

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

remote.c +1 -1

index 9b9bbfe80ec..bded6acbfe8 100644
@@ -2846,7 +2846,7 @@ 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 (*url) {
 		if (starts_with_dot_dot_slash_native(url)) {
 			url += 3;
 			colonsep |= chop_last_dir(&remoteurl, is_relative);

Quoted 24 lines · Jeff King

Quoted 4 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

According to the manual:

Quoted 8 lines

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.

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

Quoted 2 lines

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.

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

Thanks for an extra set of eyeballs.

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

remote.c +0 -0

index 9b9bbfe80ec..bded6acbfe8 100644

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

remote.c +0 -0

index 9b9bbfe80ec..bded6acbfe8 100644

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 16 lines · writes:

From: Johannes Schindelin <[email protected]>

Reported by Coverity.

Signed-off-by Johannes Schindelin <[email protected]>

---
 builtin/submodule--helper.c | 2 ++
 1 file changed, 2 insertions(+)

builtin/submodule--helper.c +0 -0

index 5c77dfcffee..d7b8004b933 100644
@@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data)
 

builtin/submodule--helper.c static int update_submodule(struct update_data *update_data) · line 2516

Quoted 4 lines

builtin/submodule--helper.c +2 -0

 		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
 								  update_data->prefix);
+		free(prefixed_path);
+

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.

Quoted 3 lines

builtin/submodule--helper.c +0 -0

 		next.prefix = NULL;
 		oidcpy(&next.oid, null_oid());
 		oidcpy(&next.suboid, null_oid());

remote.c char *relative_url(const char *remote_url, const char *url, · line 2852

Quoted 5 lines

remote.c +1 -1

-	while (url) {
+	while (*url) {
 		if (starts_with_dot_dot_slash_native(url)) {
 			url += 3;
 			colonsep |= chop_last_dir(&remoteurl, is_relative);

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

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.

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 7 lines · writes:

From: Johannes Schindelin <[email protected]>

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.

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.

Quoted 20 lines

submodule-config.c +4 -4

 		if (S_ISGITLINK(name_entry->mode) &&
 		    is_tree_submodule_active(r, root_tree, tree_path)) {
-			st_entry = xmalloc(sizeof(*st_entry));
+			ALLOC_GROW(out->entries, out->entry_nr + 1,
+				   out->entry_alloc);
+			st_entry = &out->entries[out->entry_nr++];
+
 			st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry));
 			*st_entry->name_entry = *name_entry;
 			st_entry->submodule =

@@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r,
 						root_tree))
 				FREE_AND_NULL(st_entry->repo);
 
-			ALLOC_GROW(out->entries, out->entry_nr + 1,
-				   out->entry_alloc);
-			out->entries[out->entry_nr++] = *st_entry;
 		} else if (S_ISDIR(name_entry->mode))
 			traverse_tree_submodules(r, root_tree, tree_path,
 						 &name_entry->oid, out);

Quoted 2 lines · Ævar Arnfjörð Bjarmason

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.

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

Quoted 3 lines

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.

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

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 run-command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

run-command.c +1 -1

index 14f17830f51..2ba38850b4d 100644
@@ -154,7 +154,7 @@ int is_executable(const char *name)
 		n = read(fd, buf, 2);
 		if (n == 2)
 			/* look for a she-bang */
-			if (!strcmp(buf, "#!"))
+			if (!memcmp(buf, "#!", 2))
 				st.st_mode |= S_IXUSR;
 		close(fd);
 	}

Quoted 5 lines

Quoted 3 lines

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.

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

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

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!

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

Quoted 6 lines

From: Johannes Schindelin <[email protected]>

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.

Hmm, but buf _is_ initialized fully? Line 149:

        char buf[3] = { 0 };

Quoted 5 lines

run-command.c +0 -0

index 14f17830f51..2ba38850b4d 100644
@@ -154,7 +154,7 @@ int is_executable(const char *name)

run-command.c int is_executable(const char *name) · line 160

Quoted 8 lines

run-command.c +1 -1

 		n = read(fd, buf, 2);
 		if (n == 2)
 			/* look for a she-bang */
-			if (!strcmp(buf, "#!"))
+			if (!memcmp(buf, "#!", 2))
 				st.st_mode |= S_IXUSR;
 		close(fd);
 	}

Quoted 3 lines

Hmm, but buf _is_ initialized fully? Line 149:

        char buf[3] = { 0 };

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

From: Johannes Schindelin <[email protected]>

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.