coverity: fix leaks and error paths

I wanted to whittle down the many issues reported by Coverity in the Git for Windows project. Turns out: The vast majority of the issues are false positives. Most of the remaining issues are in core Git proper.

This effort was forced on pause while Coverity was down from May 16
[https://web.archive.org/web/20260516152422/https://scan.coverity.com/] to June 22
[https://web.archive.org/web/20260622182153/https://scan.coverity.com/]).

Here is a first batch of fixes for those issues.

Johannes Schindelin (13):
load_one_loose_object_map(): fix resource leak
loose: avoid closing invalid fd on error path download_https_uri_to_file(): do not leak fd upon failure
run-command: avoid close(-1) in start_command() error paths
run_diff_files: avoid memory leak
line-log: avoid redundant copy that leaks in process_ranges
dir: free allocations on parse-error paths in read_one_dir()
submodule: fix cwd leak in get_superproject_working_tree()
worktree: fix resource leaks when branch creation fails
imap-send: avoid leaking the IMAP upload buffer reftable/table: release filter on error path
fsmonitor: plug token-data leak on early daemon-startup failures
mingw: make exit_process() own the process handle on all paths

 builtin/fsmonitor--daemon.c |  2 ++
 builtin/worktree.c          |  7 +++++--
 bundle-uri.c                |  2 +-
 compat/mingw.c              |  4 +---
 compat/win32/exit-process.h |  1 +
 diff-lib.c                  |  3 ++-
 dir.c                       |  9 +++++++--
 imap-send.c                 |  1 +
 line-log.c                  |  3 +--
 loose.c                     | 11 ++++++-----
 reftable/table.c            |  4 ++++
 run-command.c               |  6 +++---
 submodule.c                 |  8 ++++++--
 13 files changed, 40 insertions(+), 21 deletions(-)

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2163%2Fdscho%2Fcoverity-fixes-leaks-and-error-paths-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2163/dscho/coverity-fixes-leaks-and-error-paths-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2163

gitgitgadget

From: Johannes Schindelin <[email protected]>

Pointed out by Coverity.

While at it, reduce near-duplicate clean-up code at the end of the function.

---
 loose.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 0b626c1b85..47b7f5ec38 100644
@@ -65,6 +65,7 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
 {
 	struct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;
 	FILE *fp;
+	int ret = -1;
 
 	if (!loose->map)
 		loose_object_map_init(&loose->map);

@@ -98,13 +99,12 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
 		insert_loose_map(loose, &oid, &compat_oid);
 	}
 
-	strbuf_release(&buf);
-	strbuf_release(&path);
-	return errno ? -1 : 0;
+	ret = 0;
 err:
+	fclose(fp);
 	strbuf_release(&buf);
 	strbuf_release(&path);
-	return -1;
+	return ret;
 }
 
 int repo_read_loose_object_map(struct repository *repo)

gitgitgadget

From: Johannes Schindelin <[email protected]>

write_one_object() opens a file at line 186 and jumps to the errout label on failure. The errout cleanup unconditionally calls close(fd), but when open() itself failed, fd is -1. Calling close(-1) is harmless on most platforms (returns EBADF) but is undefined behavior per POSIX and can confuse fd tracking in sanitizer builds.

Guard the close with fd >= 0.

Pointed out by Coverity.

---
 loose.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 47b7f5ec38..2c6db45245 100644
@@ -202,7 +202,8 @@ static int write_one_object(struct odb_source_loose *loose,
 	return 0;
 errout:
 	error_errno(_("failed to write loose object index %s"), path.buf);
-	close(fd);
+	if (fd >= 0)
+		close(fd);
 	rollback_lock_file(&lock);
 	strbuf_release(&buf);
 	strbuf_release(&path);

gitgitgadget

From: Johannes Schindelin <[email protected]>

When the git-remote-https command fails, we do not want to leak child_out.

Pointed out by Coverity.

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

index 3b2e347288..34fa452e76 100644
@@ -378,7 +378,7 @@ cleanup:
 	if (child_in)
 		fclose(child_in);
 	if (finish_command(&cp))
-		return 1;
+		result = 1;
 	if (child_out)
 		fclose(child_out);
 	return result;

gitgitgadget

From: Johannes Schindelin <[email protected]>

When start_command() fails to set up a pipe partway through, it rolls back by closing the pipe ends it has already opened. For descriptors supplied by the caller rather than allocated locally, that rollback tested if (cmd->in) / if (cmd->out) before calling close(). The CHILD_PROCESS_INIT default of -1 ("no descriptor") is non-zero and so passes the test, meaning a caller that sets cmd->no_stdin or cmd->no_stdout without supplying a real fd ends up triggering close(-1) on the error path.

The stdin-pipe failure branch a few lines above already uses the right idiom, if (cmd->out > 0), which rejects both the -1 sentinel and 0 (the parent's own standard streams). Apply it to the three remaining rollback sites.

Reported by Coverity as CID 1049722 ("Argument cannot be negative").

---
 run-command.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index e70a8a387b..ce84db8782 100644
@@ -706,7 +706,7 @@ int start_command(struct child_process *cmd)
 			failed_errno = errno;
 			if (need_in)
 				close_pair(fdin);
-			else if (cmd->in)
+			else if (cmd->in > 0)
 				close(cmd->in);
 			str = "standard output";
 			goto fail_pipe;

@@ -720,11 +720,11 @@ int start_command(struct child_process *cmd)
 			failed_errno = errno;
 			if (need_in)
 				close_pair(fdin);
-			else if (cmd->in)
+			else if (cmd->in > 0)
 				close(cmd->in);
 			if (need_out)
 				close_pair(fdout);
-			else if (cmd->out)
+			else if (cmd->out > 0)
 				close(cmd->out);
 			str = "standard error";
 fail_pipe:

gitgitgadget

From: Johannes Schindelin <[email protected]>

In 4fc970c4388 (diff --cc: fix display of symlink conflicts during a merge., 2007-02-25) a conditional block was introduced in run_diff_files() that skips the rest of the loop iteration and advances directly to the next iteration.

However, it missed that there was a similar conditional block that was last touched in b4b1550315c (Don't instantiate structures with FAMs., 2006-06-18) and which demonstrated that the dpath structure needed to be released.

Let's fix this.

Pointed out by Coverity.

---
 diff-lib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index ae91027a02..7ba839b4a8 100644
@@ -152,7 +152,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
 			continue;
 
 		if (ce_stage(ce)) {
-			struct combine_diff_path *dpath;
+			struct combine_diff_path *dpath = NULL;
 			struct diff_filepair *pair;
 			unsigned int wt_mode = 0;
 			int num_compare_stages = 0;

@@ -164,6 +164,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
 			else {
 				if (changed < 0) {
 					perror(ce->name);
+					free(dpath);
 					continue;
 				}
 				wt_mode = 0;

gitgitgadget

From: Johannes Schindelin <[email protected]>

When bloom_filter_check() indicates that a commit does not touch any of the tracked paths, line_log_process_ranges_arbitrary_commit() propagates the current ranges to the parent by calling line_log_data_copy() and passing the copy to add_line_range(). However, add_line_range() always makes its own copy internally (via line_log_data_copy or line_log_data_merge), so the caller's copy is never freed and leaks every time this path is taken.

Pass range directly to add_line_range() instead of making a redundant intermediate copy. The callee's internal copy handles ownership correctly.

Pointed out by Coverity.

---
 line-log.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 5fc75ae275..0179f138f7 100644
@@ -1141,8 +1141,7 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev, struct commit
 
 	if (range) {
 		if (commit->parents && !bloom_filter_check(rev, commit, range)) {
-			struct line_log_data *prange = line_log_data_copy(range);
-			add_line_range(rev, commit->parents->item, prange);
+			add_line_range(rev, commit->parents->item, range);
 			clear_commit_line_range(rev, commit);
 		} else if (commit->parents && commit->parents->next)
 			changed = process_ranges_merge_commit(rev, commit, range);

gitgitgadget

From: Johannes Schindelin <[email protected]>

When read_one_dir() encounters a parse error while reading the untracked cache from disk, it returns -1 immediately. Two allocations made earlier in the function can leak on these early-return paths: ud.untracked (allocated at line 3846 when untracked_nr > 0) and ud.dirs (allocated at line 3851).

Free both before returning on the two error paths between these allocations and the point where they are transferred into the final xmalloc'd struct at line 3857.

Pointed out by Coverity.

---
 dir.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index 32430090dc..23335b9f7a 100644
@@ -3792,13 +3792,18 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
 		ALLOC_ARRAY(ud.untracked, ud.untracked_nr);
 
 	ud.dirs_alloc = ud.dirs_nr = decode_varint(&data);
-	if (data > end)
+	if (data > end) {
+		free(ud.untracked);
 		return -1;
+	}
 	ALLOC_ARRAY(ud.dirs, ud.dirs_nr);
 
 	eos = memchr(data, '\0', end - data);
-	if (!eos || eos == end)
+	if (!eos || eos == end) {
+		free(ud.untracked);
+		free(ud.dirs);
 		return -1;
+	}
 
 	*untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));
 	memcpy(untracked, &ud, sizeof(ud));

gitgitgadget

From: Johannes Schindelin <[email protected]>

get_superproject_working_tree() allocates cwd via xgetcwd() at the top of the function, but two early-return paths (when not inside a work tree, and when strbuf_realpath for "../" fails) return 0 without freeing it.

Redirect these early returns through a cleanup label that frees cwd before returning.

Pointed out by Coverity.

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

index fd91201a92..8ddeebd8af 100644
@@ -2627,10 +2627,10 @@ int get_superproject_working_tree(struct strbuf *buf)
 		 * We might have a superproject, but it is harder
 		 * to determine.
 		 */
-		return 0;
+		goto out;
 
 	if (!strbuf_realpath(&one_up, "../", 0))
-		return 0;
+		goto out;
 
 	subpath = relative_path(cwd, one_up.buf, &sb);
 	strbuf_release(&one_up);

@@ -2693,6 +2693,10 @@ int get_superproject_working_tree(struct strbuf *buf)
 		die(_("ls-tree returned unexpected return code %d"), code);
 
 	return ret;
+
+out:
+	free(cwd);
+	return 0;
 }
 
 /*

gitgitgadget

From: Johannes Schindelin <[email protected]>

In the "add" subcommand, when run_command() fails while creating a new branch (line 948), the function returns -1 immediately without freeing the allocations made earlier: path (from prefix_filename at line 858), opt_track, branch_to_free, and new_branch_to_free.

Redirect the error return through the existing cleanup block at the end of the function so all four allocations are properly freed.

Pointed out by Coverity.

---
 builtin/worktree.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index d21c43fde3..4bc7b4f6e7 100644
@@ -945,14 +945,17 @@ static int add(int ac, const char **av, const char *prefix,
 		strvec_push(&cp.args, branch);
 		if (opt_track)
 			strvec_push(&cp.args, opt_track);
-		if (run_command(&cp))
-			return -1;
+		if (run_command(&cp)) {
+			ret = -1;
+			goto cleanup;
+		}
 		branch = new_branch;
 	} else if (opt_track) {
 		die(_("--[no-]track can only be used if a new branch is created"));
 	}
 
 	ret = add_worktree(path, branch, &opts);
+cleanup:
 	free(path);
 	free(opt_track);
 	free(branch_to_free);

gitgitgadget

From: Johannes Schindelin <[email protected]>

When uploading messages via libcurl, curl_append_msgs_to_imap() accumulates each one in a strbuf that grows across loop iterations but is never released before the function returns.

Release it alongside the existing libcurl cleanup.

Reported by Coverity as CID 1671507 ("Resource leak").

---
 imap-send.c | 1 +
 1 file changed, 1 insertion(+)

index cfd6a5120c..0d16d02029 100644
@@ -1750,6 +1750,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
 
 	curl_easy_cleanup(curl);
 	curl_global_cleanup();
+	strbuf_release(&msgbuf.buf);
 
 	if (cred.username) {
 		if (res == CURLE_OK)

gitgitgadget

From: Johannes Schindelin <[email protected]>

reftable_table_refs_for_unindexed() allocates a filtering_ref_iterator and then calls reftable_buf_add() to populate its oid buffer. On success ownership is transferred to the output iterator, but if reftable_buf_add() fails, the goto-out cleanup only frees the table iterator and walks away from both the filter allocation and the oid buffer that reftable_buf_add() may have grown.

Release filter->oid and free filter alongside the existing table iterator cleanup.

Reported by Coverity as CID 1671512 ("Resource leak").

---
 reftable/table.c | 4 ++++
 1 file changed, 4 insertions(+)

index 56362df0ed..d604ddebf4 100644
@@ -709,6 +709,10 @@ out:
 		if (ti)
 			table_iter_close(ti);
 		reftable_free(ti);
+		if (filter) {
+			reftable_buf_release(&filter->oid);
+			reftable_free(filter);
+		}
 	}
 	return err;
 }

gitgitgadget

From: Johannes Schindelin <[email protected]>

fsmonitor_run_daemon() allocates state.current_token_data before any subordinate setup step that may fail (alias resolution, listener/health constructors, asynchronous IPC server init). On the successful path the listener thread takes ownership and clears the field during its teardown, so the done: cleanup block sees a NULL pointer. On every early-error path, however, control jumps straight to done: with the freshly allocated token data still referenced, and it is never freed, as Coverity flagged.

Free it at the top of done: and clear the pointer. The success path is a no-op (the pointer is already NULL there); the error paths now drop the otherwise-leaked allocation. fsmonitor_free_token_data() is NULL-safe and asserts client_ref_count == 0, which holds trivially here because the IPC server has not yet begun accepting clients when these failures occur.

---
 builtin/fsmonitor--daemon.c | 2 ++
 1 file changed, 2 insertions(+)

index f920cf3a82..4161dd8282 100644
@@ -1418,6 +1418,8 @@ static int fsmonitor_run_daemon(void)
 	err = fsmonitor_run_daemon_1(&state);
 
 done:
+	fsmonitor_free_token_data(state.current_token_data);
+	state.current_token_data = NULL;
 	pthread_cond_destroy(&state.cookies_cond);
 	pthread_mutex_destroy(&state.main_lock);
 	{

gitgitgadget

From: Johannes Schindelin <[email protected]>

After "mingw: kill child processes in a gentler way", the ownership of the HANDLE passed to exit_process() and terminate_process_tree() is inconsistent. terminate_process_tree() always closes the handle; exit_process() closes it on success and on the terminate-tree fallback, but leaks it on the early return where GetExitCodeProcess() fails or reports the process is no longer STILL_ACTIVE.

mingw_kill() compensated by closing the handle on its own error path, which is a double-close on every error path that does not hit that one leaky branch -- the callee has already closed the handle by then. Coverity flagged the resulting use-after-free as CID 1437238.

Pin down the invariant that exit_process() and terminate_process_tree() own the handle from the call onward and close it on every return path; with that, the bogus close in mingw_kill() goes away.

---
 compat/mingw.c              | 4 +---
 compat/win32/exit-process.h | 1 +
 2 files changed, 2 insertions(+), 3 deletions(-)

index 41e055f7de..e2cb92a414 100644
@@ -2269,10 +2269,8 @@ int mingw_kill(pid_t pid, int sig)
 			}
 			ret = terminate_process_tree(h, 128 + sig);
 		}
-		if (ret) {
+		if (ret)
 			errno = err_win_to_posix(GetLastError());
-			CloseHandle(h);
-		}
 		return ret;
 	} else if (pid > 0 && sig == 0) {
 		HANDLE h = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);

index d53989884c..26004161bc 100644
@@ -159,6 +159,7 @@ static int exit_process(HANDLE process, int exit_code)
 		return terminate_process_tree(process, exit_code);
 	}
 
+	CloseHandle(process);
 	return 0;
 }
 

gitgitgadget

Heh, I just posted the identical patch (in my case found by running the test suite with GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1).

So yeah, looks good to me. :)

-Peff

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

	ret = ferror(fp) ? -1 : 0;

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

I read through the series and did not see anything jumping at me as wrong. Looking good. Will queue.

Thanks.

Hi Junio,

Agreed; that is what v2 will use.

To corroborate the diagnosis: strbuf_getline_lf() ultimately calls getdelim(), which returns -1 on both EOF and I/O error, so ferror(fp) on the underlying stream is the only reliable way to distinguish the two. That also makes the errno = 0; I had added at the top of the loop dead, so it goes away in v2.

Ciao,
Johannes

Hi Patrick,

You are right; the patch as posted plugs nothing.

The reason it looks pointless is that the leak it was written against was fixed independently in the meantime by 949bb8f74f4a (run_diff_files():
delay allocation of combine_diff_path, 2025-01-09), which moved the dpath = xmalloc(...) to after the check_removed() call. Before that reordering, the two continue statements did leak the just-allocated dpath (originally introduced by 4fc970c43884, 2007-02-25).

I had missed that when picking back up the work on addressing Coverity reports, sorry! I will drop this patch from v2.

Ciao,
Johannes

Hi Patrick,

Ownership transfers to the caller on the xmalloc/memcpy line: the memcpy copies the ud.untracked and ud.dirs pointers into the freshly xmalloc'd struct that becomes *untracked_. From there, any subsequent failure in the caller reaches free_untracked_cache() and then free_untracked(), which releases both arrays. So the two further early returns are correct as-are.

I will fold that reasoning into the v2 commit message so a future reader does not have to re-derive it.

Incidentally, and orthogonal to Coverity's leak report: on those same failure paths, individual slots of ->dirs and ->untracked remain uninitialised, so free_untracked() walks garbage pointers before it ever reaches the two free() calls above. That is a separate crash-on-cleanup bug and I would prefer to address it in a follow-up rather than widen the scope of this series.

Ciao,
Johannes

Hi Patrick,

Agreed. In v2 the function has a single exit path: all late returns fall through to the out: label, which additionally releases sb and one_up.

A side effect worth noting is that consolidation also closes a latent leak the original had on the strbuf_realpath(&one_up, "../", 0) failure path. strbuf_realpath_1() calls strbuf_reset(resolved) on error, which does not free the backing buffer, so one_up could carry a residual allocation that the previous shape never released.

Ciao,
Johannes

I wanted to whittle down the many issues reported by Coverity in the Git for Windows project. Turns out: The vast majority of the issues are false positives. Most of the remaining issues are in core Git proper.

This effort was forced on pause while Coverity was down from May 16
[https://web.archive.org/web/20260516152422/https://scan.coverity.com/] to June 22
[https://web.archive.org/web/20260622182153/https://scan.coverity.com/]).

Here is a first batch of fixes for those issues.

Changes since v1:

* Edited the commit messages to put function names in backticks, and reflowed the messages afterwards.
* Took Junio's suggestion to avoid (ab-)using errno to determine the return value of load_one_loose_object_map().
* Dropped the obsolete patch "run_diff_files: avoid memory leak".
* Rewrote the commit message of "dir: free allocations on parse-error paths in read_one_dir()" to clarify ownership of the allocated untracked/dirs buffers.
* Changed "submodule: fix cwd leak in get_superproject_working_tree()" to reduce the cognitive load on the reader (i.e. to make it a lot easier to reason about the correctness of the patch).

Johannes Schindelin (12):
load_one_loose_object_map(): fix resource leak
loose: avoid closing invalid fd on error path download_https_uri_to_file(): do not leak fd upon failure
run-command: avoid close(-1) in start_command() error paths
line-log: avoid redundant copy that leaks in process_ranges
dir: free allocations on parse-error paths in read_one_dir()
submodule: fix cwd leak in get_superproject_working_tree()
worktree: fix resource leaks when branch creation fails
imap-send: avoid leaking the IMAP upload buffer reftable/table: release filter on error path
fsmonitor: plug token-data leak on early daemon-startup failures
mingw: make exit_process() own the process handle on all paths

 builtin/fsmonitor--daemon.c |  2 ++
 builtin/worktree.c          |  7 +++++--
 bundle-uri.c                |  2 +-
 compat/mingw.c              |  4 +---
 compat/win32/exit-process.h |  1 +
 dir.c                       |  9 +++++++--
 imap-send.c                 |  1 +
 line-log.c                  |  3 +--
 loose.c                     | 12 ++++++------
 reftable/table.c            |  4 ++++
 run-command.c               |  6 +++---
 submodule.c                 | 19 ++++++++++---------
 12 files changed, 42 insertions(+), 28 deletions(-)

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2163%2Fdscho%2Fcoverity-fixes-leaks-and-error-paths-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2163/dscho/coverity-fixes-leaks-and-error-paths-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2163

Range-diff vs v1:

1: 17242c249f ! 1: 80ae35227d load_one_loose_object_map(): fix resource leak

     @@ loose.c: static int load_one_loose_object_map(struct repository *repo, struct od
       
       	if (!loose->map)
       		loose_object_map_init(&loose->map);
     +@@ loose.c: static int load_one_loose_object_map(struct repository *repo, struct odb_source_
     + 		return 0;
     + 	}
     + 
     +-	errno = 0;
     + 	if (strbuf_getwholeline(&buf, fp, '\n') || strcmp(buf.buf, loose_object_header))
     + 		goto err;
     + 	while (!strbuf_getline_lf(&buf, fp)) {
      @@ loose.c: static int load_one_loose_object_map(struct repository *repo, struct odb_source_
       		insert_loose_map(loose, &oid, &compat_oid);
       	}
     @@ loose.c: static int load_one_loose_object_map(struct repository *repo, struct od
      -	strbuf_release(&buf);
      -	strbuf_release(&path);
      -	return errno ? -1 : 0;
     -+	ret = 0;
     ++	ret = ferror(fp) ? -1 : 0;
       err:
      +	fclose(fp);
       	strbuf_release(&buf);

2: a1cd229e33 ! 2: 546a7c5d9f loose: avoid closing invalid fd on error path
@@ Metadata
## Commit message ##
loose: avoid closing invalid fd on error path

- write_one_object() opens a file at line 186 and jumps to the
- errout label on failure. The errout cleanup unconditionally calls
- close(fd), but when open() itself failed, fd is -1. Calling
- close(-1) is harmless on most platforms (returns EBADF) but is
- undefined behavior per POSIX and can confuse fd tracking in
- sanitizer builds.
+ write_one_object() opens a file at line 186 and jumps to the errout
+ label on failure. The errout cleanup unconditionally calls close(fd),
+ but when open() itself failed, fd is -1. Calling close(-1) is
+ harmless on most platforms (returns EBADF) but is undefined behavior per
+ POSIX and can confuse fd tracking in sanitizer builds.

Guard the close with fd >= 0.

  3:  a770d9708d =  3:  17c3b4ce4f download_https_uri_to_file(): do not leak fd upon failure
  4:  d7bcdda312 !  4:  0360016d91 run-command: avoid close(-1) in start_command() error paths

@@ Metadata
Author: Johannes Schindelin <[email protected]>

## Commit message ##
- run-command: avoid close(-1) in start_command() error paths
+ run-command: avoid close(-1) in start_command() error paths

- When start_command() fails to set up a pipe partway through, it
- rolls back by closing the pipe ends it has already opened. For
- descriptors supplied by the caller rather than allocated locally,
- that rollback tested if (cmd->in) / if (cmd->out) before calling
- close(). The CHILD_PROCESS_INIT default of -1 ("no descriptor") is
- non-zero and so passes the test, meaning a caller that sets
- cmd->no_stdin or cmd->no_stdout without supplying a real fd ends up
- triggering close(-1) on the error path.
+ When start_command() fails to set up a pipe partway through, it rolls
+ back by closing the pipe ends it has already opened. For descriptors
+ supplied by the caller rather than allocated locally, that rollback
+ tested if (cmd->in) / if (cmd->out) before calling close(). The
+ CHILD_PROCESS_INIT default of -1 ("no descriptor") is non-zero and so
+ passes the test, meaning a caller that sets cmd->no_stdin or
+ cmd->no_stdout without supplying a real fd ends up triggering close(-1)
+ on the error path.

- The stdin-pipe failure branch a few lines above already uses the
- right idiom, if (cmd->out > 0), which rejects both the -1 sentinel
- and 0 (the parent's own standard streams). Apply it to the three
- remaining rollback sites.
+ The stdin-pipe failure branch a few lines above already uses the right
+ idiom, if (cmd->out > 0), which rejects both the -1 sentinel and 0
+ (the parent's own standard streams). Apply it to the three remaining
+ rollback sites.

Reported by Coverity as CID 1049722 ("Argument cannot be negative").

  5:  860bc8f52d <  -:  ---------- run_diff_files: avoid memory leak
  6:  5a6b17f075 !  5:  8c623cc28f line-log: avoid redundant copy that leaks in process_ranges

@@ Metadata
## Commit message ##
line-log: avoid redundant copy that leaks in process_ranges

- When bloom_filter_check() indicates that a commit does not touch
- any of the tracked paths, line_log_process_ranges_arbitrary_commit()
+ When bloom_filter_check() indicates that a commit does not touch any
+ of the tracked paths, line_log_process_ranges_arbitrary_commit()
propagates the current ranges to the parent by calling
- line_log_data_copy() and passing the copy to add_line_range().
- However, add_line_range() always makes its own copy internally
- (via line_log_data_copy or line_log_data_merge), so the caller's
- copy is never freed and leaks every time this path is taken.
+ line_log_data_copy() and passing the copy to add_line_range().
+ However, add_line_range() always makes its own copy internally (via
+ line_log_data_copy or line_log_data_merge), so the caller's copy is
+ never freed and leaks every time this path is taken.

- Pass range directly to add_line_range() instead of making a
- redundant intermediate copy. The callee's internal copy handles
- ownership correctly.
+ Pass range directly to add_line_range() instead of making a redundant
+ intermediate copy. The callee's internal copy handles ownership
+ correctly.

Pointed out by Coverity.

7: 62ce03454a ! 6: 8a8fe2d3e3 dir: free allocations on parse-error paths in read_one_dir()
@@ Metadata
Author: Johannes Schindelin <[email protected]>

## Commit message ##
- dir: free allocations on parse-error paths in read_one_dir()
+ dir: free allocations on parse-error paths in read_one_dir()

- When read_one_dir() encounters a parse error while reading the
- untracked cache from disk, it returns -1 immediately. Two
- allocations made earlier in the function can leak on these
- early-return paths: ud.untracked (allocated at line 3846 when
- untracked_nr > 0) and ud.dirs (allocated at line 3851).
+ Two of read_one_dir()'s parse-error early returns leak ud.untracked
+ and ud.dirs. Plug them.

- Free both before returning on the two error paths between these
- allocations and the point where they are transferred into the
- final xmalloc'd struct at line 3857.
+ The other early returns in the same function are fine: they occur after
+ the xmalloc()+memcpy() that copies ud into *untracked_, at which
+ point ownership is transferred to the caller.
+ read_untracked_extension() then releases everything via
+ free_untracked_cache() on failure.

Pointed out by Coverity.

8: 6a43f95241 ! 7: 5397ea785c submodule: fix cwd leak in get_superproject_working_tree()
@@ Metadata
Author: Johannes Schindelin <[email protected]>

## Commit message ##
- submodule: fix cwd leak in get_superproject_working_tree()
+ submodule: fix cwd leak in get_superproject_working_tree()

- get_superproject_working_tree() allocates cwd via xgetcwd() at
- the top of the function, but two early-return paths (when not
- inside a work tree, and when strbuf_realpath for "../" fails)
- return 0 without freeing it.
+ get_superproject_working_tree() allocates cwd via xgetcwd() at the
+ top of the function, but two early-return paths (when not inside a work
+ tree, and when strbuf_realpath for "../" fails) return 0 without freeing
+ it.

- Redirect these early returns through a cleanup label that frees
- cwd before returning.
+ Redirect these early returns through a cleanup label that frees cwd
+ before returning.

Pointed out by Coverity.

@@ submodule.c: int get_superproject_working_tree(struct strbuf *buf)
+ goto out;

subpath = relative_path(cwd, one_up.buf, &sb);
- strbuf_release(&one_up);
+- strbuf_release(&one_up);
+
+ prepare_submodule_repo_env(&cp.env);
+ strvec_pop(&cp.env);
@@ submodule.c: int get_superproject_working_tree(struct strbuf *buf)
+ ret = 1;
+ free(super_wt);
+ }
+- free(cwd);
+- strbuf_release(&sb);
+
+ code = finish_command(&cp);
+
+ if (code == 128)
+ /* '../' is not a git repository */
+- return 0;
+- if (code == 0 && len == 0)
++ ret = 0;
++ else if (code == 0 && len == 0)
+ /* There is an unrelated git repository at '../' */
+- return 0;
+- if (code)
++ ret = 0;
++ else if (code)
die(_("ls-tree returned unexpected return code %d"), code);

- return ret;
-+
+out:
++ strbuf_release(&sb);
++ strbuf_release(&one_up);
+ free(cwd);
-+ return 0;
+ return ret;
}

- /*
9: e39e2f5aa4 ! 8: 0048c0ca27 worktree: fix resource leaks when branch creation fails
@@ Metadata
## Commit message ##
worktree: fix resource leaks when branch creation fails

- In the "add" subcommand, when run_command() fails while creating
- a new branch (line 948), the function returns -1 immediately
- without freeing the allocations made earlier: path (from
- prefix_filename at line 858), opt_track, branch_to_free, and
- new_branch_to_free.
+ In the "add" subcommand, when run_command() fails while creating a new
+ branch (line 948), the function returns -1 immediately without freeing
+ the allocations made earlier: path (from prefix_filename at line 858),
+ opt_track, branch_to_free, and new_branch_to_free.

- Redirect the error return through the existing cleanup block at
- the end of the function so all four allocations are properly
- freed.
+ Redirect the error return through the existing cleanup block at the end
+ of the function so all four allocations are properly freed.

Pointed out by Coverity.

10: cc19a300f5 ! 9: 4048a225a5 imap-send: avoid leaking the IMAP upload buffer
@@ Metadata
## Commit message ##
imap-send: avoid leaking the IMAP upload buffer

- When uploading messages via libcurl, curl_append_msgs_to_imap()
- accumulates each one in a strbuf that grows across loop iterations
- but is never released before the function returns.
+ When uploading messages via libcurl, curl_append_msgs_to_imap()
+ accumulates each one in a strbuf that grows across loop iterations but
+ is never released before the function returns.

Release it alongside the existing libcurl cleanup.

11: 198062addd ! 10: 13ecebcdee reftable/table: release filter on error path
@@ Metadata
## Commit message ##
reftable/table: release filter on error path

- reftable_table_refs_for_unindexed() allocates a filtering_ref_iterator
- and then calls reftable_buf_add() to populate its oid buffer. On
+ reftable_table_refs_for_unindexed() allocates a filtering_ref_iterator
+ and then calls reftable_buf_add() to populate its oid buffer. On
success ownership is transferred to the output iterator, but if
- reftable_buf_add() fails, the goto-out cleanup only frees the table
- iterator and walks away from both the filter allocation and the
- oid buffer that reftable_buf_add() may have grown.
+ reftable_buf_add() fails, the goto-out cleanup only frees the table
+ iterator and walks away from both the filter allocation and the oid
+ buffer that reftable_buf_add() may have grown.

Release filter->oid and free filter alongside the existing table iterator cleanup.

 12:  8ad6b220e9 = 11:  97049d7cc3 fsmonitor: plug token-data leak on early daemon-startup failures
 13:  23ab9864b2 ! 12:  a5a6c27184 mingw: make exit_process() own the process handle on all paths

@@ Metadata
Author: Johannes Schindelin <[email protected]>

## Commit message ##
- mingw: make exit_process() own the process handle on all paths
+ mingw: make exit_process() own the process handle on all paths

After "mingw: kill child processes in a gentler way", the ownership of
- the HANDLE passed to exit_process() and terminate_process_tree() is
- inconsistent. terminate_process_tree() always closes the handle;
- exit_process() closes it on success and on the terminate-tree
+ the HANDLE passed to exit_process() and terminate_process_tree() is
+ inconsistent. terminate_process_tree() always closes the handle;
+ exit_process() closes it on success and on the terminate-tree
fallback, but leaks it on the early return where GetExitCodeProcess() fails or reports the process is no longer STILL_ACTIVE.

- mingw_kill() compensated by closing the handle on its own error path,
- which is a double-close on every error path that does not hit that
- one leaky branch -- the callee has already closed the handle by then.
+ mingw_kill() compensated by closing the handle on its own error path,
+ which is a double-close on every error path that does not hit that one
+ leaky branch -- the callee has already closed the handle by then.
Coverity flagged the resulting use-after-free as CID 1437238.

- Pin down the invariant that exit_process() and
- terminate_process_tree() own the handle from the call onward and
- close it on every return path; with that, the bogus close in
- mingw_kill() goes away.
+ Pin down the invariant that exit_process() and
+ terminate_process_tree() own the handle from the call onward and close
+ it on every return path; with that, the bogus close in mingw_kill()
+ goes away.

gitgitgadget

From: Johannes Schindelin <[email protected]>

Pointed out by Coverity.

While at it, reduce near-duplicate clean-up code at the end of the function.

---
 loose.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index 0b626c1b85..940a9e0dfe 100644
@@ -65,6 +65,7 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
 {
 	struct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;
 	FILE *fp;
+	int ret = -1;
 
 	if (!loose->map)
 		loose_object_map_init(&loose->map);

@@ -84,7 +85,6 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
 		return 0;
 	}
 
-	errno = 0;
 	if (strbuf_getwholeline(&buf, fp, '\n') || strcmp(buf.buf, loose_object_header))
 		goto err;
 	while (!strbuf_getline_lf(&buf, fp)) {

@@ -98,13 +98,12 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
 		insert_loose_map(loose, &oid, &compat_oid);
 	}
 
-	strbuf_release(&buf);
-	strbuf_release(&path);
-	return errno ? -1 : 0;
+	ret = ferror(fp) ? -1 : 0;
 err:
+	fclose(fp);
 	strbuf_release(&buf);
 	strbuf_release(&path);
-	return -1;
+	return ret;
 }
 
 int repo_read_loose_object_map(struct repository *repo)

gitgitgadget

From: Johannes Schindelin <[email protected]>

write_one_object() opens a file at line 186 and jumps to the errout label on failure. The errout cleanup unconditionally calls close(fd), but when open() itself failed, fd is -1. Calling close(-1) is harmless on most platforms (returns EBADF) but is undefined behavior per POSIX and can confuse fd tracking in sanitizer builds.

Guard the close with fd >= 0.

Pointed out by Coverity.

---
 loose.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 940a9e0dfe..bf01d3e42d 100644
@@ -201,7 +201,8 @@ static int write_one_object(struct odb_source_loose *loose,
 	return 0;
 errout:
 	error_errno(_("failed to write loose object index %s"), path.buf);
-	close(fd);
+	if (fd >= 0)
+		close(fd);
 	rollback_lock_file(&lock);
 	strbuf_release(&buf);
 	strbuf_release(&path);

gitgitgadget

From: Johannes Schindelin <[email protected]>

When the git-remote-https command fails, we do not want to leak child_out.

Pointed out by Coverity.

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

index 3b2e347288..34fa452e76 100644
@@ -378,7 +378,7 @@ cleanup:
 	if (child_in)
 		fclose(child_in);
 	if (finish_command(&cp))
-		return 1;
+		result = 1;
 	if (child_out)
 		fclose(child_out);
 	return result;

gitgitgadget

From: Johannes Schindelin <[email protected]>

When start_command() fails to set up a pipe partway through, it rolls back by closing the pipe ends it has already opened. For descriptors supplied by the caller rather than allocated locally, that rollback tested if (cmd->in) / if (cmd->out) before calling close(). The CHILD_PROCESS_INIT default of -1 ("no descriptor") is non-zero and so passes the test, meaning a caller that sets cmd->no_stdin or cmd->no_stdout without supplying a real fd ends up triggering close(-1) on the error path.

The stdin-pipe failure branch a few lines above already uses the right idiom, if (cmd->out > 0), which rejects both the -1 sentinel and 0 (the parent's own standard streams). Apply it to the three remaining rollback sites.

Reported by Coverity as CID 1049722 ("Argument cannot be negative").

---
 run-command.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index e70a8a387b..ce84db8782 100644
@@ -706,7 +706,7 @@ int start_command(struct child_process *cmd)
 			failed_errno = errno;
 			if (need_in)
 				close_pair(fdin);
-			else if (cmd->in)
+			else if (cmd->in > 0)
 				close(cmd->in);
 			str = "standard output";
 			goto fail_pipe;

@@ -720,11 +720,11 @@ int start_command(struct child_process *cmd)
 			failed_errno = errno;
 			if (need_in)
 				close_pair(fdin);
-			else if (cmd->in)
+			else if (cmd->in > 0)
 				close(cmd->in);
 			if (need_out)
 				close_pair(fdout);
-			else if (cmd->out)
+			else if (cmd->out > 0)
 				close(cmd->out);
 			str = "standard error";
 fail_pipe:

gitgitgadget

From: Johannes Schindelin <[email protected]>

When bloom_filter_check() indicates that a commit does not touch any of the tracked paths, line_log_process_ranges_arbitrary_commit() propagates the current ranges to the parent by calling line_log_data_copy() and passing the copy to add_line_range(). However, add_line_range() always makes its own copy internally (via line_log_data_copy or line_log_data_merge), so the caller's copy is never freed and leaks every time this path is taken.

Pass range directly to add_line_range() instead of making a redundant intermediate copy. The callee's internal copy handles ownership correctly.

Pointed out by Coverity.

---
 line-log.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 5fc75ae275..0179f138f7 100644
@@ -1141,8 +1141,7 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev, struct commit
 
 	if (range) {
 		if (commit->parents && !bloom_filter_check(rev, commit, range)) {
-			struct line_log_data *prange = line_log_data_copy(range);
-			add_line_range(rev, commit->parents->item, prange);
+			add_line_range(rev, commit->parents->item, range);
 			clear_commit_line_range(rev, commit);
 		} else if (commit->parents && commit->parents->next)
 			changed = process_ranges_merge_commit(rev, commit, range);

gitgitgadget

From: Johannes Schindelin <[email protected]>

Two of read_one_dir()'s parse-error early returns leak ud.untracked and ud.dirs. Plug them.

The other early returns in the same function are fine: they occur after the xmalloc()+memcpy() that copies ud into *untracked_, at which point ownership is transferred to the caller. read_untracked_extension() then releases everything via free_untracked_cache() on failure.

Pointed out by Coverity.

---
 dir.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index 32430090dc..23335b9f7a 100644
@@ -3792,13 +3792,18 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
 		ALLOC_ARRAY(ud.untracked, ud.untracked_nr);
 
 	ud.dirs_alloc = ud.dirs_nr = decode_varint(&data);
-	if (data > end)
+	if (data > end) {
+		free(ud.untracked);
 		return -1;
+	}
 	ALLOC_ARRAY(ud.dirs, ud.dirs_nr);
 
 	eos = memchr(data, '\0', end - data);
-	if (!eos || eos == end)
+	if (!eos || eos == end) {
+		free(ud.untracked);
+		free(ud.dirs);
 		return -1;
+	}
 
 	*untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));
 	memcpy(untracked, &ud, sizeof(ud));

gitgitgadget

From: Johannes Schindelin <[email protected]>

get_superproject_working_tree() allocates cwd via xgetcwd() at the top of the function, but two early-return paths (when not inside a work tree, and when strbuf_realpath for "../" fails) return 0 without freeing it.

Redirect these early returns through a cleanup label that frees cwd before returning.

Pointed out by Coverity.

---
 submodule.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

index fd91201a92..92dfb0fc2d 100644
@@ -2627,13 +2627,12 @@ int get_superproject_working_tree(struct strbuf *buf)
 		 * We might have a superproject, but it is harder
 		 * to determine.
 		 */
-		return 0;
+		goto out;
 
 	if (!strbuf_realpath(&one_up, "../", 0))
-		return 0;
+		goto out;
 
 	subpath = relative_path(cwd, one_up.buf, &sb);
-	strbuf_release(&one_up);
 
 	prepare_submodule_repo_env(&cp.env);
 	strvec_pop(&cp.env);

@@ -2678,20 +2677,22 @@ int get_superproject_working_tree(struct strbuf *buf)
 		ret = 1;
 		free(super_wt);
 	}
-	free(cwd);
-	strbuf_release(&sb);
 
 	code = finish_command(&cp);
 
 	if (code == 128)
 		/* '../' is not a git repository */
-		return 0;
-	if (code == 0 && len == 0)
+		ret = 0;
+	else if (code == 0 && len == 0)
 		/* There is an unrelated git repository at '../' */
-		return 0;
-	if (code)
+		ret = 0;
+	else if (code)
 		die(_("ls-tree returned unexpected return code %d"), code);
 
+out:
+	strbuf_release(&sb);
+	strbuf_release(&one_up);
+	free(cwd);
 	return ret;
 }
 

gitgitgadget

From: Johannes Schindelin <[email protected]>

In the "add" subcommand, when run_command() fails while creating a new branch (line 948), the function returns -1 immediately without freeing the allocations made earlier: path (from prefix_filename at line 858), opt_track, branch_to_free, and new_branch_to_free.

Redirect the error return through the existing cleanup block at the end of the function so all four allocations are properly freed.

Pointed out by Coverity.

---
 builtin/worktree.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index d21c43fde3..4bc7b4f6e7 100644
@@ -945,14 +945,17 @@ static int add(int ac, const char **av, const char *prefix,
 		strvec_push(&cp.args, branch);
 		if (opt_track)
 			strvec_push(&cp.args, opt_track);
-		if (run_command(&cp))
-			return -1;
+		if (run_command(&cp)) {
+			ret = -1;
+			goto cleanup;
+		}
 		branch = new_branch;
 	} else if (opt_track) {
 		die(_("--[no-]track can only be used if a new branch is created"));
 	}
 
 	ret = add_worktree(path, branch, &opts);
+cleanup:
 	free(path);
 	free(opt_track);
 	free(branch_to_free);

gitgitgadget

From: Johannes Schindelin <[email protected]>

When uploading messages via libcurl, curl_append_msgs_to_imap() accumulates each one in a strbuf that grows across loop iterations but is never released before the function returns.

Release it alongside the existing libcurl cleanup.

Reported by Coverity as CID 1671507 ("Resource leak").

---
 imap-send.c | 1 +
 1 file changed, 1 insertion(+)

index cfd6a5120c..0d16d02029 100644
@@ -1750,6 +1750,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
 
 	curl_easy_cleanup(curl);
 	curl_global_cleanup();
+	strbuf_release(&msgbuf.buf);
 
 	if (cred.username) {
 		if (res == CURLE_OK)

gitgitgadget

From: Johannes Schindelin <[email protected]>

reftable_table_refs_for_unindexed() allocates a filtering_ref_iterator and then calls reftable_buf_add() to populate its oid buffer. On success ownership is transferred to the output iterator, but if reftable_buf_add() fails, the goto-out cleanup only frees the table iterator and walks away from both the filter allocation and the oid buffer that reftable_buf_add() may have grown.

Release filter->oid and free filter alongside the existing table iterator cleanup.

Reported by Coverity as CID 1671512 ("Resource leak").

---
 reftable/table.c | 4 ++++
 1 file changed, 4 insertions(+)

index 56362df0ed..d604ddebf4 100644
@@ -709,6 +709,10 @@ out:
 		if (ti)
 			table_iter_close(ti);
 		reftable_free(ti);
+		if (filter) {
+			reftable_buf_release(&filter->oid);
+			reftable_free(filter);
+		}
 	}
 	return err;
 }

gitgitgadget

From: Johannes Schindelin <[email protected]>

fsmonitor_run_daemon() allocates state.current_token_data before any subordinate setup step that may fail (alias resolution, listener/health constructors, asynchronous IPC server init). On the successful path the listener thread takes ownership and clears the field during its teardown, so the done: cleanup block sees a NULL pointer. On every early-error path, however, control jumps straight to done: with the freshly allocated token data still referenced, and it is never freed, as Coverity flagged.

Free it at the top of done: and clear the pointer. The success path is a no-op (the pointer is already NULL there); the error paths now drop the otherwise-leaked allocation. fsmonitor_free_token_data() is NULL-safe and asserts client_ref_count == 0, which holds trivially here because the IPC server has not yet begun accepting clients when these failures occur.

---
 builtin/fsmonitor--daemon.c | 2 ++
 1 file changed, 2 insertions(+)

index f920cf3a82..4161dd8282 100644
@@ -1418,6 +1418,8 @@ static int fsmonitor_run_daemon(void)
 	err = fsmonitor_run_daemon_1(&state);
 
 done:
+	fsmonitor_free_token_data(state.current_token_data);
+	state.current_token_data = NULL;
 	pthread_cond_destroy(&state.cookies_cond);
 	pthread_mutex_destroy(&state.main_lock);
 	{

gitgitgadget

From: Johannes Schindelin <[email protected]>

After "mingw: kill child processes in a gentler way", the ownership of the HANDLE passed to exit_process() and terminate_process_tree() is inconsistent. terminate_process_tree() always closes the handle; exit_process() closes it on success and on the terminate-tree fallback, but leaks it on the early return where GetExitCodeProcess() fails or reports the process is no longer STILL_ACTIVE.

mingw_kill() compensated by closing the handle on its own error path, which is a double-close on every error path that does not hit that one leaky branch -- the callee has already closed the handle by then. Coverity flagged the resulting use-after-free as CID 1437238.

Pin down the invariant that exit_process() and terminate_process_tree() own the handle from the call onward and close it on every return path; with that, the bogus close in mingw_kill() goes away.

---
 compat/mingw.c              | 4 +---
 compat/win32/exit-process.h | 1 +
 2 files changed, 2 insertions(+), 3 deletions(-)

index 41e055f7de..e2cb92a414 100644
@@ -2269,10 +2269,8 @@ int mingw_kill(pid_t pid, int sig)
 			}
 			ret = terminate_process_tree(h, 128 + sig);
 		}
-		if (ret) {
+		if (ret)
 			errno = err_win_to_posix(GetLastError());
-			CloseHandle(h);
-		}
 		return ret;
 	} else if (pid > 0 && sig == 0) {
 		HANDLE h = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);

index d53989884c..26004161bc 100644
@@ -159,6 +159,7 @@ static int exit_process(HANDLE process, int exit_code)
 		return terminate_process_tree(process, exit_code);
 	}
 
+	CloseHandle(process);
 	return 0;
 }
 

gitgitgadget

Thanks. The reflow of the commit messages made the range-diff somewhat hard to read, but from all I could see the changes all make sense.

Patrick

Yup, this round looks good to me, too. Thanks, both.