From: Stefan Sperling <stsp@stsp.name>
Subject: Re: plug leak in got_fileindex_read error path
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Fri, 7 Apr 2023 15:32:20 +0200

On Fri, Apr 07, 2023 at 01:19:11PM +0200, Omar Polo wrote:
> 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.

I think this issue of forgetting to close the repo in some
places came up before. Let's fix it. ok for the diff.

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