From: Omar Polo <op@omarpolo.com>
Subject: Re: plug leak in got_fileindex_read error path
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Fri, 07 Apr 2023 13:19:11 +0200

On 2023/04/07 12:40:28 +0200, Omar Polo <op@omarpolo.com> wrote:
> Was playing with Otto' malloc diff to find leaks and spotted this.
> It's not what the malloc diff complains about (the recallocarray()
> call in lib/fileindex.c:/^read_fileindex_path around line 591 that I
> just can't see -- maybe it's an error in how malloc marks leaks.)

Unsurprisingly, turns out I was too fast in my judgment and that we
really leak some memory because some cmd_*() functions don't call
got_repo_close().  In my testing I was lucky to blindly hit one that
had the leak (cmd_status), but there are a few more.

These are not important leaks; after all once a cmd_*() terminates
we're gonna exit anyway, but since most functions are closing the
struct repository I think it's better to do so consistently.

While here I'm also catching got_repo_close() failures in
wrap_not_worktree_error().

Regress is happy and malloc reports too[1], ok?

[1]: well, mostly.  running "got status" in a loop sometimes still
     still show one leak in memory allocated at lib/fileindex.c:591,
     but not every time.

diff /home/op/w/got
commit - 6be067cef84c15f7e8623ec8fccaf955d98d006b
path + /home/op/w/got
blob - 8ff7e074110ce8eb022c3fee11169b99a0e583fe
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -968,6 +968,11 @@ done:
 		if (error == NULL)
 			error = pack_err;
 	}
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (preserve_logmsg) {
 		fprintf(stderr, "%s: log message preserved in %s\n",
 		    getprogname(), logmsg_path);
@@ -3214,6 +3219,11 @@ done:
 		got_ref_close(head_ref);
 	if (ref)
 		got_ref_close(ref);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	got_pathlist_free(&paths, GOT_PATHLIST_FREE_NONE);
 	free(commit_id_str);
 	free(commit_id);
@@ -3454,7 +3464,11 @@ wrap_not_worktree_error(const struct got_error *orig_e
 	    "'got checkout'.\n"
 	    "The got(1) manual page contains more information.", cmdname);
 	err = got_error_msg(GOT_ERR_NOT_WORKTREE, msg);
-	got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (close_err == NULL)
+			err = close_err;
+	}
 	if (pack_fds) {
 		const struct got_error *pack_err =
 		    got_repo_pack_fds_close(pack_fds);
@@ -3647,6 +3661,11 @@ done:
 		if (error == NULL)
 			error = pack_err;
 	}
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	free(worktree_path);
 	got_pathlist_free(&paths, GOT_PATHLIST_FREE_PATH);
 	free(commit_id);
@@ -6390,6 +6409,11 @@ done:
 		if (error == NULL)
 			error = pack_err;
 	}
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 
 	got_pathlist_free(&paths, GOT_PATHLIST_FREE_PATH);
 	free(cwd);