Skip to content

coverity: fix leaks and error paths #2163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dscho wants to merge 12 commits into
base: master
Choose a base branch
from
Open

coverity: fix leaks and error paths #2163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
80ae352
load_one_loose_object_map(): fix resource leak
dscho Dec 19, 2024
546a7c5
loose: avoid closing invalid fd on error path
dscho May 5, 2026
17c3b4c
download_https_uri_to_file(): do not leak fd upon failure
dscho Dec 19, 2024
0360016
run-command: avoid `close(-1)` in `start_command()` error paths
dscho Jun 27, 2026
8c623cc
line-log: avoid redundant copy that leaks in process_ranges
dscho Apr 19, 2026
8a8fe2d
dir: free allocations on parse-error paths in `read_one_dir()`
dscho Apr 19, 2026
5397ea7
submodule: fix cwd leak in `get_superproject_working_tree()`
dscho Apr 27, 2026
0048c0c
worktree: fix resource leaks when branch creation fails
dscho Apr 27, 2026
4048a22
imap-send: avoid leaking the IMAP upload buffer
dscho Jun 27, 2026
13ecebc
reftable/table: release filter on error path
dscho Jun 27, 2026
97049d7
fsmonitor: plug token-data leak on early daemon-startup failures
dscho Jun 28, 2026
a5a6c27
mingw: make `exit_process()` own the process handle on all paths
dscho Jun 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions builtin/fsmonitor--daemon.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
{
Expand Down
7 changes: 5 additions & 2 deletions builtin/worktree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion bundle-uri.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ static int download_https_uri_to_file(const char *file, const char *uri)
if (child_in)
fclose(child_in);
if (finish_command(&cp))
return 1;
result = 1;
if (child_out)
fclose(child_out);
return result;
Expand Down
4 changes: 1 addition & 3 deletions compat/mingw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions compat/win32/exit-process.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ static int exit_process(HANDLE process, int exit_code)
return terminate_process_tree(process, exit_code);
}

CloseHandle(process);
return 0;
}

Expand Down
9 changes: 7 additions & 2 deletions dir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3792,13 +3792,18 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
ALLOC_ARRAY(ud.untracked, ud.untracked_nr);

@gitgitgadget gitgitgadget Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Wed, Jul 01, 2026 at 07:04:25AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/dir.c b/dir.c
> index 32430090dc..23335b9f7a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -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));

Hm. Here we assign ownership to the caller, but this still feels quite
off to me as we also have two more early returns after this point that
seem to leak memory. Do the callers make sure to always free the data?

Patrick

@gitgitgadget gitgitgadget Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Johannes Schindelin wrote on the Git mailing list (how to reply to this email):

Hi Patrick,

On Wed, 1 Jul 2026, Patrick Steinhardt wrote:

> On Wed, Jul 01, 2026 at 07:04:25AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/dir.c b/dir.c
> > index 32430090dc..23335b9f7a 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -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));
> 
> Hm. Here we assign ownership to the caller, but this still feels quite
> off to me as we also have two more early returns after this point that
> seem to leak memory. Do the callers make sure to always free the data?

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


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));
Expand Down
1 change: 1 addition & 0 deletions imap-send.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 1 addition & 2 deletions line-log.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1141,8 +1141,7 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev, struct commit

@gitgitgadget gitgitgadget Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeff King wrote on the Git mailing list (how to reply to this email):

On Wed, Jul 01, 2026 at 07:04:24AM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

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

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);
Expand Down
12 changes: 6 additions & 6 deletions loose.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
{

@gitgitgadget gitgitgadget Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Wed, Jul 01, 2026 at 07:04:19AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/loose.c b/loose.c
> index 0b626c1b85..47b7f5ec38 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -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;
>  }

Makes sense. There's no `goto err` before we assign `fp`, and when the
call to `fopen()` fails we return via a different path. So the added
call to `fclose(fp)` is fine.

Patrick

@gitgitgadget gitgitgadget Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Pointed out by Coverity.
>
> While at it, reduce near-duplicate clean-up code at the end of the
> function.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  loose.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/loose.c b/loose.c
> index 0b626c1b85..47b7f5ec38 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -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;

Wow, this is bad bad bad.  We do not even know what is in errno as
we are supposed to have jumped to out-of-line err label in all error
cases.

> +	ret = 0;

Or we can do

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

if we want to be sure that we have caught all the errors.

>  err:
> +	fclose(fp);
>  	strbuf_release(&buf);
>  	strbuf_release(&path);
> -	return -1;
> +	return ret;
>  }
>  
>  int repo_read_loose_object_map(struct repository *repo)

@gitgitgadget gitgitgadget Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Johannes Schindelin wrote on the Git mailing list (how to reply to this email):

Hi Junio,

On Wed, 1 Jul 2026, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > +	ret = 0;
> 
> Or we can do
> 
> 	ret = ferror(fp) ? -1 : 0;
> 
> if we want to be sure that we have caught all the errors.

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

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Sun, Jul 05, 2026 at 08:24:18AM +0000, Johannes Schindelin via GitGitGadget wrote:
> @@ -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;

Nit: it might've made sense to explain the switch to ferror(3p) in the
commit message, but that alone isn't worth a reroll.

Patrick

struct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;
FILE *fp;
int ret = -1;

if (!loose->map)
loose_object_map_init(&loose->map);
Expand All @@ -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)) {
Expand All @@ -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)
Expand Down Expand Up @@ -202,7 +201,8 @@ static int write_one_object(struct odb_source_loose *loose,
return 0;

@gitgitgadget gitgitgadget Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Wed, Jul 01, 2026 at 07:04:20AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/loose.c b/loose.c
> index 47b7f5ec38..2c6db45245 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -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);

Makes sense. At the time we hit the first `goto errout` we have already
assigned `fd = open(...)`, so we know it should be either negative or a
positive file descriptor.

There's also a second call to `close(fd)`, but if that call is
successful then we would not use the `errout` path. If it fails we may
try to close the file descriptor a second time, but that's probably a
non-issue.

Patrick

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);
Expand Down
4 changes: 4 additions & 0 deletions reftable/table.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,10 @@ static int reftable_table_refs_for_unindexed(struct reftable_table *t,
if (ti)
table_iter_close(ti);
reftable_free(ti);
if (filter) {
reftable_buf_release(&filter->oid);
reftable_free(filter);
}
}
return err;
}
Expand Down
6 changes: 3 additions & 3 deletions run-command.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ int start_command(struct child_process *cmd)
failed_errno = errno;

@gitgitgadget gitgitgadget Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Wed, Jul 01, 2026 at 07:04:22AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/run-command.c b/run-command.c
> index e70a8a387b..ce84db8782 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -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:

Right. There's a fourth site that does `close(cmd->out)`, but that site
already guards with `if (cmd->out > 0)`.

Patrick

if (need_in)
close_pair(fdin);
else if (cmd->in)
else if (cmd->in > 0)
close(cmd->in);
str = "standard output";
goto fail_pipe;
Expand All @@ -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:
Expand Down
19 changes: 10 additions & 9 deletions submodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2627,13 +2627,12 @@ int get_superproject_working_tree(struct strbuf *buf)
* We might have a superproject, but it is harder

@gitgitgadget gitgitgadget Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Wed, Jul 01, 2026 at 07:04:26AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/submodule.c b/submodule.c
> index fd91201a92..8ddeebd8af 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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;
>  }

Okay. This is fine, but it feels a bit fragile as we also have a call to
`free(cwd)` a bit further up. So if somebody were to add a `goto out`
after that call we'd have a double free. Makes me wonder whether we want
to have a single exit path for the complete function and then drop the
other call to free(3p).

Patrick

@gitgitgadget gitgitgadget Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Johannes Schindelin wrote on the Git mailing list (how to reply to this email):

Hi Patrick,

On Wed, 1 Jul 2026, Patrick Steinhardt wrote:

> On Wed, Jul 01, 2026 at 07:04:26AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/submodule.c b/submodule.c
> > index fd91201a92..8ddeebd8af 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -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;
> >  }
> 
> Okay. This is fine, but it feels a bit fragile as we also have a call to
> `free(cwd)` a bit further up. So if somebody were to add a `goto out`
> after that call we'd have a double free. Makes me wonder whether we want
> to have a single exit path for the complete function and then drop the
> other call to free(3p).

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

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Sun, Jul 05, 2026 at 08:24:24AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/submodule.c b/submodule.c
> index fd91201a92..92dfb0fc2d 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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);

Right. `ret` is already zero-initialized at the beginning of the
function, so it's fine to just `goto out` here.

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

The diff is a bit hard to read as we also convert this to use `else if`,
but overall the end result is easier to reason about.

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

All of these variables are always initialized, so this change looks good
to me.

Thanks!

Patrick

* 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);
Expand Down Expand Up @@ -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;
}

Expand Down
Loading