Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions bisect.c
Original file line number Diff line number Diff line change
Expand Up @@ -1019,10 +1019,12 @@ void read_bisect_terms(char **read_bad, char **read_good)
die_errno(_("could not read file '%s'"), filename);

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:

> diff --git a/builtin/bisect.c b/builtin/bisect.c
> index 798e28f501..fe66d84382 100644
> --- a/builtin/bisect.c
> +++ b/builtin/bisect.c
> @@ -498,9 +498,15 @@ static int get_terms(struct bisect_terms *terms)
>  	}
>  
>  	free_terms(terms);
> -	strbuf_getline_lf(&str, fp);
> +	if (strbuf_getline_lf(&str, fp) == EOF) {
> +		res = -1;
> +		goto finish;
> +	}
>  	terms->term_bad = strbuf_detach(&str, NULL);
> -	strbuf_getline_lf(&str, fp);
> +	if (strbuf_getline_lf(&str, fp) == EOF) {
> +		res = -1;
> +		goto finish;
> +	}

We want to clean-up terms->term_bad when we fail to read the second
line after reading the first line successfully, no?

>  	terms->term_good = strbuf_detach(&str, NULL);
>  
>  finish:

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

Junio C Hamano <gitster@pobox.com> writes:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> ...
>> diff --git a/builtin/bisect.c b/builtin/bisect.c
>> index 798e28f501..fe66d84382 100644
>> --- a/builtin/bisect.c
>> +++ b/builtin/bisect.c
>> @@ -498,9 +498,15 @@ static int get_terms(struct bisect_terms *terms)
>>  	}
>>  
>>  	free_terms(terms);
>> -	strbuf_getline_lf(&str, fp);
>> +	if (strbuf_getline_lf(&str, fp) == EOF) {
>> +		res = -1;
>> +		goto finish;
>> +	}
>>  	terms->term_bad = strbuf_detach(&str, NULL);
>> -	strbuf_getline_lf(&str, fp);
>> +	if (strbuf_getline_lf(&str, fp) == EOF) {
>> +		res = -1;
>> +		goto finish;
>> +	}
>
> We want to clean-up terms->term_bad when we fail to read the second
> line after reading the first line successfully, no?
>
>>  	terms->term_good = strbuf_detach(&str, NULL);
>>  
>>  finish:

--- >8 ---
Subject: [PATCH] fixup! bisect: check strbuf_getline_lf return when reading
 terms

https://lore.kernel.org/git/xmqqh5m1qcfh.fsf@gitster.g/

This fixes the immediate leak introduced by

https://lore.kernel.org/git/17c382fdf46eada79ce03a7604dd7e0454d8bea4.1784069325.git.gitgitgadget@gmail.com/

but many callers of get_terms() should all be fixed to check for
return value.  If it fails to grab the replacement word for "bad",
both terms->term_bad and terms->term_good are left NULL, since the
function calls free_terms() early.

diff --git a/builtin/bisect.c b/builtin/bisect.c
index fe66d84382..69ab7ea248 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -505,6 +505,7 @@ static int get_terms(struct bisect_terms *terms)
 	terms->term_bad = strbuf_detach(&str, NULL);
 	if (strbuf_getline_lf(&str, fp) == EOF) {
 		res = -1;
+		FREE_AND_NULL(terms->term_bad);
 		goto finish;
 	}
 	terms->term_good = strbuf_detach(&str, NULL);

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 Sun, 19 Jul 2026, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> > ...
> >> diff --git a/builtin/bisect.c b/builtin/bisect.c
> >> index 798e28f501..fe66d84382 100644
> >> --- a/builtin/bisect.c
> >> +++ b/builtin/bisect.c
> >> @@ -498,9 +498,15 @@ static int get_terms(struct bisect_terms *terms)
> >>  	}
> >>  
> >>  	free_terms(terms);
> >> -	strbuf_getline_lf(&str, fp);
> >> +	if (strbuf_getline_lf(&str, fp) == EOF) {
> >> +		res = -1;
> >> +		goto finish;
> >> +	}
> >>  	terms->term_bad = strbuf_detach(&str, NULL);
> >> -	strbuf_getline_lf(&str, fp);
> >> +	if (strbuf_getline_lf(&str, fp) == EOF) {
> >> +		res = -1;
> >> +		goto finish;
> >> +	}
> >
> > We want to clean-up terms->term_bad when we fail to read the second
> > line after reading the first line successfully, no?
> >
> >>  	terms->term_good = strbuf_detach(&str, NULL);
> >>  
> >>  finish:
> 
> --- >8 ---
> Subject: [PATCH] fixup! bisect: check strbuf_getline_lf return when reading
>  terms
> 
> https://lore.kernel.org/git/xmqqh5m1qcfh.fsf@gitster.g/
> 
> This fixes the immediate leak introduced by
> 
> https://lore.kernel.org/git/17c382fdf46eada79ce03a7604dd7e0454d8bea4.1784069325.git.gitgitgadget@gmail.com/
> 
> but many callers of get_terms() should all be fixed to check for
> return value.  If it fails to grab the replacement word for "bad",
> both terms->term_bad and terms->term_good are left NULL, since the
> function calls free_terms() early.
> 
> diff --git a/builtin/bisect.c b/builtin/bisect.c
> index fe66d84382..69ab7ea248 100644
> --- a/builtin/bisect.c
> +++ b/builtin/bisect.c
> @@ -505,6 +505,7 @@ static int get_terms(struct bisect_terms *terms)
>  	terms->term_bad = strbuf_detach(&str, NULL);
>  	if (strbuf_getline_lf(&str, fp) == EOF) {
>  		res = -1;
> +		FREE_AND_NULL(terms->term_bad);

Good catch!

Thank you,
Johannes

>  		goto finish;
>  	}
>  	terms->term_good = strbuf_detach(&str, NULL);
> 

}
} else {
strbuf_getline_lf(&str, fp);
if (strbuf_getline_lf(&str, fp) == EOF)
die(_("could not read bad term from file '%s'"), filename);
free(*read_bad);
*read_bad = strbuf_detach(&str, NULL);
strbuf_getline_lf(&str, fp);
if (strbuf_getline_lf(&str, fp) == EOF)
die(_("could not read good term from file '%s'"), filename);
free(*read_good);
*read_good = strbuf_detach(&str, NULL);
}
Expand Down
42 changes: 30 additions & 12 deletions builtin/bisect.c
Original file line number Diff line number Diff line change
Expand Up @@ -485,22 +485,29 @@ static int bisect_next_check(const struct bisect_terms *terms,
return decide_next(terms, current_term, !state.nr_good, !state.nr_bad);

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:

> There is one slight complication here: One caller _needs_ the return
> value to indicate an error when the `BISECT_TERMS` file is absent, all
> the other call sites are totally okay with a "missing" `BISECT_TERMS`
> file. To address that, extend the function signature of `get_terms()` to
> indicate which behavior the caller wants.

> -static int get_terms(struct bisect_terms *terms)
> +static int get_terms(struct bisect_terms *terms, int file_missing_is_ok)
>  {
>  	struct strbuf str = STRBUF_INIT;
>  	FILE *fp = NULL;
> @@ -493,7 +493,7 @@ static int get_terms(struct bisect_terms *terms)
>  
>  	fp = fopen(git_path_bisect_terms(), "r");
>  	if (!fp) {
> -		res = -1;
> +		res = file_missing_is_ok ? 0 : -1;
>  		goto finish;
>  	}

Hmph.  So, depending on the caller, a missing file error may have to
be treated as OK or as an error, while all other kinds of anomalies
are treated by all callers as errors.

As all the existing callsites of this function need to be adjusted
for this change anyway, I would have thought a more typical way to
handle a situation like this would be to define different error
codes for this function and have the callers deal with them.  But it
seems that almost all callers, except for one, pass "missing is OK."

So, instead of adjusting the majority of callers with something like:

        -       if (get_terms(...))
        +       if (get_terms(...) == BISECT_TERMS_ERROR)
                        oops we got an error

and keeping only the single oddball caller to barf on any non-zero
return, 

        -       if (get_terms(...))
        +       switch (get_terms(...)) {
	+	case BISECT_TERMS_ERROR:
                        oops we got an error
	+		break;
	+	case BISECT_TERMS_MISSING_FILE:
	+		deal with the missing file error
	+		break;
	+	default:
	+		break; /* ok */
	+	}

it may be simpler to change:

        -       if (get_terms(...))
        +       if (get_terms(..., 1))
                        oops we got an error

for the majority of them.  The one oddball caller then becomes:

        -       if (get_terms(...))
        +       if (get_terms(..., 0))
                        oops we got an error

to treat a missing file as an error as well.

I guess I can buy that.

If get_terms() were a public function that had many more callers,
my preference would probably be very different.  But this is local
to a single file, so the meaning of the mysterious 0/1 parameter
will quickly become evident to those who have to work with this
part of the system anyway.

Thanks.

}

static int get_terms(struct bisect_terms *terms)
static int get_terms(struct bisect_terms *terms, int file_missing_is_ok)
{
struct strbuf str = STRBUF_INIT;
FILE *fp = NULL;
int res = 0;

fp = fopen(git_path_bisect_terms(), "r");
if (!fp) {
res = -1;
res = file_missing_is_ok ? 0 : -1;
goto finish;
}

free_terms(terms);
strbuf_getline_lf(&str, fp);
if (strbuf_getline_lf(&str, fp) == EOF) {
res = -1;
goto finish;
}
terms->term_bad = strbuf_detach(&str, NULL);
strbuf_getline_lf(&str, fp);
if (strbuf_getline_lf(&str, fp) == EOF) {
res = -1;
FREE_AND_NULL(terms->term_bad);
goto finish;
}
terms->term_good = strbuf_detach(&str, NULL);

finish:
Expand All @@ -512,7 +519,7 @@ static int get_terms(struct bisect_terms *terms)

static int bisect_terms(struct bisect_terms *terms, const char *option)
{
if (get_terms(terms))
if (get_terms(terms, 0))
return error(_("no terms defined"));

if (!option) {
Expand Down Expand Up @@ -1050,7 +1057,8 @@ static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
rev = word_end + strspn(word_end, " \t");
*word_end = '\0'; /* NUL-terminate the word */

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 Tue, Jul 14, 2026 at 10:48:43PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Six callers of get_terms() silently discard its return value. When
> get_terms fails (missing or truncated BISECT_TERMS file), the term
> strings remain NULL or empty, causing confusing downstream
> behavior: commands like "bisect next" or "bisect run" proceed with
> empty term strings, producing nonsensical ref names (refs/bisect/
> with no suffix) and misleading error messages.
> 
> Add checks at each call site so that a failed get_terms produces a
> clear "no terms defined" error, matching the pattern already used
> in bisect_terms() at line 512. The check tests the term pointers
> rather than the return value because some callers (bisect skip,
> legacy bad/good) call set_terms before get_terms, and the
> set_terms values should survive a get_terms failure.

Hm. Are there any callers that accept the case where either `term->bad`
or `term->good` are `NULL`? If not, should we maybe adapt the function
itself to return an error if so and then have all callers only ever
check for the return value of `get_term()` instead of also having to
check the result? That might also allow us to deduplicate the error
messages.

Patrick

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, 15 Jul 2026, Patrick Steinhardt wrote:

> On Tue, Jul 14, 2026 at 10:48:43PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > Six callers of get_terms() silently discard its return value. When
> > get_terms fails (missing or truncated BISECT_TERMS file), the term
> > strings remain NULL or empty, causing confusing downstream
> > behavior: commands like "bisect next" or "bisect run" proceed with
> > empty term strings, producing nonsensical ref names (refs/bisect/
> > with no suffix) and misleading error messages.
> > 
> > Add checks at each call site so that a failed get_terms produces a
> > clear "no terms defined" error, matching the pattern already used
> > in bisect_terms() at line 512. The check tests the term pointers
> > rather than the return value because some callers (bisect skip,
> > legacy bad/good) call set_terms before get_terms, and the
> > set_terms values should survive a get_terms failure.
> 
> Hm. Are there any callers that accept the case where either `term->bad`
> or `term->good` are `NULL`?

As far as I can tell, no, the case where either `term->bad` or
`term->good` are `NULL` is not permissible.

> If not, should we maybe adapt the function itself to return an error if
> so and then have all callers only ever check for the return value of
> `get_term()` instead of also having to check the result? That might also
> allow us to deduplicate the error messages.

It's a good point that we should not look at `term->bad` and `term->good`,
but at the return value of `get_term()` instead. That's incidentally what
`bisect_terms()` does, and we should do the same here (including the same,
already-translated error message).

Thanks,
Johannes


get_terms(terms);
if (get_terms(terms, 1))
return error(_("no terms defined"));
if (check_and_set_terms(terms, p))
return -1;

Expand Down Expand Up @@ -1300,7 +1308,12 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)

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 Tue, Jul 14, 2026 at 10:48:44PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/builtin/bisect.c b/builtin/bisect.c
> index 15a2a30f89..801daf8c78 100644
> --- a/builtin/bisect.c
> +++ b/builtin/bisect.c
> @@ -1308,6 +1308,11 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
>  
>  		fflush(stdout);
>  		saved_stdout = dup(1);
> +		if (saved_stdout < 0) {
> +			res = error_errno(_("could not duplicate stdout"));
> +			close(temporary_stdout_fd);
> +			break;
> +		}
>  		dup2(temporary_stdout_fd, 1);

Shouldn't we also verify the return value of `dup2()` while at it?

Patrick

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, 15 Jul 2026, Patrick Steinhardt wrote:

> On Tue, Jul 14, 2026 at 10:48:44PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/builtin/bisect.c b/builtin/bisect.c
> > index 15a2a30f89..801daf8c78 100644
> > --- a/builtin/bisect.c
> > +++ b/builtin/bisect.c
> > @@ -1308,6 +1308,11 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
> >  
> >  		fflush(stdout);
> >  		saved_stdout = dup(1);
> > +		if (saved_stdout < 0) {
> > +			res = error_errno(_("could not duplicate stdout"));
> > +			close(temporary_stdout_fd);
> > +			break;
> > +		}
> >  		dup2(temporary_stdout_fd, 1);
> 
> Shouldn't we also verify the return value of `dup2()` while at it?

True. I wonder why Coverity didn't complain... funny. I changed it to also
check the return value of `dup2()`.

Thank you for your review!
Johannes

fflush(stdout);
saved_stdout = dup(1);
dup2(temporary_stdout_fd, 1);
if (saved_stdout < 0 ||
dup2(temporary_stdout_fd, 1) < 0) {
res = error_errno(_("could not duplicate stdout"));
close(temporary_stdout_fd);
break;
}

res = bisect_state(terms, 1, &new_state);

Expand Down Expand Up @@ -1376,7 +1389,8 @@ static int cmd_bisect__next(int argc, const char **argv UNUSED, const char *pref
if (argc)
return error(_("'%s' requires 0 arguments"),
"git bisect next");
get_terms(&terms);
if (get_terms(&terms, 1))
return error(_("no terms defined"));
res = bisect_next(&terms, prefix);
free_terms(&terms);
return res;
Expand Down Expand Up @@ -1410,7 +1424,8 @@ static int cmd_bisect__skip(int argc, const char **argv, const char *prefix UNUS
struct bisect_terms terms = { 0 };

set_terms(&terms, "bad", "good");
get_terms(&terms);
if (get_terms(&terms, 1))
return error(_("no terms defined"));
res = bisect_skip(&terms, argc, argv);
free_terms(&terms);
return res;
Expand All @@ -1422,7 +1437,8 @@ static int cmd_bisect__visualize(int argc, const char **argv, const char *prefix
int res;
struct bisect_terms terms = { 0 };

get_terms(&terms);
if (get_terms(&terms, 1))
return error(_("no terms defined"));
res = bisect_visualize(&terms, argc, argv);
free_terms(&terms);
return res;
Expand All @@ -1436,7 +1452,8 @@ static int cmd_bisect__run(int argc, const char **argv, const char *prefix UNUSE

if (!argc)
return error(_("'%s' failed: no command provided."), "git bisect run");
get_terms(&terms);
if (get_terms(&terms, 1))
return error(_("no terms defined"));
res = bisect_run(&terms, argc, argv);
free_terms(&terms);
return res;
Expand Down Expand Up @@ -1475,7 +1492,8 @@ int cmd_bisect(int argc,
usage_with_options(git_bisect_usage, options);

set_terms(&terms, "bad", "good");
get_terms(&terms);
if (get_terms(&terms, 1))
return error(_("no terms defined"));
if (check_and_set_terms(&terms, argv[0]) ||
!one_of(argv[0], terms.term_good, terms.term_bad, NULL))
usage_msg_optf(_("unknown command: '%s'"), git_bisect_usage,
Expand Down
5 changes: 4 additions & 1 deletion builtin/config.c
Original file line number Diff line number Diff line change
Expand Up @@ -1313,7 +1313,10 @@ static int show_editor(struct config_location_options *opts)
else if (errno != EEXIST)

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 Tue, Jul 14, 2026 at 10:48:35PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/builtin/config.c b/builtin/config.c
> index 8d8ec0beea..1307fdb0d6 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -1313,7 +1313,10 @@ static int show_editor(struct config_location_options *opts)
>  		else if (errno != EEXIST)
>  			die_errno(_("cannot create configuration file %s"), config_file);
>  	}
> -	launch_editor(config_file, NULL, NULL);
> +	if (launch_editor(config_file, NULL, NULL)) {
> +		free(config_file);
> +		return -1;
> +	}

All error paths in `launch_editor()` already print an error message, so
we indeed don't have to do anything but bubble up the error here.

Patrick

die_errno(_("cannot create configuration file %s"), config_file);
}
launch_editor(config_file, NULL, NULL);
if (launch_editor(config_file, NULL, NULL)) {
free(config_file);
return -1;
}
free(config_file);

return 0;
Expand Down
9 changes: 6 additions & 3 deletions builtin/last-modified.c
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,8 @@ static void process_parent(struct last_modified *lm,
{

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:

> Skip unparsable commits by checking the return value and
> continuing to the next iteration (or returning early in
> process_parent). This matches the defensive pattern used in other
> revision walkers such as limit_list() and get_revision_internal().
> ...
> @@ -414,12 +415,14 @@ static int last_modified_run(struct last_modified *lm)
>  		 * Otherwise, make sure that 'c' isn't reachable from anything
>  		 * in the '--not' queue.
>  		 */
> -		repo_parse_commit(lm->rev.repo, c);
> +		if (repo_parse_commit(lm->rev.repo, c))
> +			continue;

Shouldn't this be

			goto cleanup;

instead?  'n' pulled out of not_queue may be unparseable and when we
ignore it, don't we still want to clean up the active_paths slab for
commit 'c'?

>  		while ((n = prio_queue_get(&not_queue))) {
>  			struct commit_list *np;
>  
> -			repo_parse_commit(lm->rev.repo, n);
> +			if (repo_parse_commit(lm->rev.repo, n))
> +				continue;
>  
>  			for (np = n->parents; np; np = np->next) {
>  				if (!(np->item->object.flags & PARENT2)) {

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

Junio C Hamano <gitster@pobox.com> writes:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> ...
>> -		repo_parse_commit(lm->rev.repo, c);
>> +		if (repo_parse_commit(lm->rev.repo, c))
>> +			continue;
>
> Shouldn't this be
>
> 			goto cleanup;
>
> instead?  'n' pulled out of not_queue may be unparseable and when we
> ignore it, don't we still want to clean up the active_paths slab for
> commit 'c'?

--- >8 ---
Subject: [PATCH] fixup! last-modified: handle repo_parse_commit() failures

https://lore.kernel.org/git/xmqqldbdqciy.fsf@gitster.g/

'n' pulled out of not_queue may be unparseable and when we ignore
it, we still want to clean up the active_paths slab for commit 'c'.

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index fe012b0c2e..3846244dfc 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -416,7 +416,7 @@ static int last_modified_run(struct last_modified *lm)
 		 * in the '--not' queue.
 		 */
 		if (repo_parse_commit(lm->rev.repo, c))
-			continue;
+			goto cleanup;
 
 		while ((n = prio_queue_get(&not_queue))) {
 			struct commit_list *np;

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 Sun, 19 Jul 2026, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> > ...
> >> -		repo_parse_commit(lm->rev.repo, c);
> >> +		if (repo_parse_commit(lm->rev.repo, c))
> >> +			continue;
> >
> > Shouldn't this be
> >
> > 			goto cleanup;
> >
> > instead?  'n' pulled out of not_queue may be unparseable and when we
> > ignore it, don't we still want to clean up the active_paths slab for
> > commit 'c'?

Correct.

Thanks,
Johannes

> 
> --- >8 ---
> Subject: [PATCH] fixup! last-modified: handle repo_parse_commit() failures
> 
> https://lore.kernel.org/git/xmqqldbdqciy.fsf@gitster.g/
> 
> 'n' pulled out of not_queue may be unparseable and when we ignore
> it, we still want to clean up the active_paths slab for commit 'c'.
> 
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index fe012b0c2e..3846244dfc 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -416,7 +416,7 @@ static int last_modified_run(struct last_modified *lm)
>  		 * in the '--not' queue.
>  		 */
>  		if (repo_parse_commit(lm->rev.repo, c))
> -			continue;
> +			goto cleanup;
>  
>  		while ((n = prio_queue_get(&not_queue))) {
>  			struct commit_list *np;
> 

struct bitmap *active_p;

repo_parse_commit(lm->rev.repo, parent);
if (repo_parse_commit(lm->rev.repo, parent))
return;
active_p = active_paths_for(lm, parent);

/*
Expand Down Expand Up @@ -414,12 +415,14 @@ static int last_modified_run(struct last_modified *lm)
* Otherwise, make sure that 'c' isn't reachable from anything
* in the '--not' queue.
*/
repo_parse_commit(lm->rev.repo, c);
if (repo_parse_commit(lm->rev.repo, c))
goto cleanup;

while ((n = prio_queue_get(&not_queue))) {
struct commit_list *np;

repo_parse_commit(lm->rev.repo, n);
if (repo_parse_commit(lm->rev.repo, n))
continue;

for (np = n->parents; np; np = np->next) {
if (!(np->item->object.flags & PARENT2)) {
Expand Down
2 changes: 2 additions & 0 deletions compat/pread.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ ssize_t git_pread(int fd, void *buf, size_t count, off_t offset)
ssize_t rc;

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 Tue, Jul 14, 2026 at 10:48:39PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/compat/pread.c b/compat/pread.c
> index 484e6d4c71..ac7d058cb8 100644
> --- a/compat/pread.c
> +++ b/compat/pread.c
> @@ -7,6 +7,8 @@ ssize_t git_pread(int fd, void *buf, size_t count, off_t offset)
>          ssize_t rc;
>  
>          current_offset = lseek(fd, 0, SEEK_CUR);
> +	if (current_offset < 0)
> +		return -1;
>  
>          if (lseek(fd, offset, SEEK_SET) < 0)
>                  return -1;

Heh, funny. I wanted to complain about misindentation here, but your new
code is actually indented correctly. It's everything else in this file
that is indented with spaces.

Patrick

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, 15 Jul 2026, Patrick Steinhardt wrote:

> On Tue, Jul 14, 2026 at 10:48:39PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/compat/pread.c b/compat/pread.c
> > index 484e6d4c71..ac7d058cb8 100644
> > --- a/compat/pread.c
> > +++ b/compat/pread.c
> > @@ -7,6 +7,8 @@ ssize_t git_pread(int fd, void *buf, size_t count, off_t offset)
> >          ssize_t rc;
> >  
> >          current_offset = lseek(fd, 0, SEEK_CUR);
> > +	if (current_offset < 0)
> > +		return -1;
> >  
> >          if (lseek(fd, offset, SEEK_SET) < 0)
> >                  return -1;
> 
> Heh, funny. I wanted to complain about misindentation here, but your new
> code is actually indented correctly. It's everything else in this file
> that is indented with spaces.

Heh. I did notice something odd going on, thinking that Opus ignored my
clear instructions about tab-indentation once again when I replaced the
spaces by tabs...

Ciao,
Johannes


current_offset = lseek(fd, 0, SEEK_CUR);
if (current_offset < 0)
return -1;

if (lseek(fd, offset, SEEK_SET) < 0)
return -1;
Expand Down
2 changes: 2 additions & 0 deletions http.c
Original file line number Diff line number Diff line change
Expand Up @@ -1608,6 +1608,8 @@ struct active_request_slot *get_active_slot(void)

if (!slot->curl) {
slot->curl = curl_easy_duphandle(curl_default);
if (!slot->curl)
die("curl_easy_duphandle failed");
curl_session_count++;
}

Expand Down
3 changes: 2 additions & 1 deletion reftable/block.c
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,
REFTABLE_CALLOC_ARRAY(bw->zstream, 1);

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:

> The function already uses REFTABLE_ZLIB_ERROR for deflate()
> failures later in the code path (lines 171, 199), so returning
> the same error code for deflateInit() failure is consistent.
>
> Pointed out by Coverity.
>
> Assisted-by: Claude Opus 4.6
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  reftable/block.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/reftable/block.c b/reftable/block.c
> index 920b3f4486..ec81fd0493 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -87,7 +87,8 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,
>  		REFTABLE_CALLOC_ARRAY(bw->zstream, 1);
>  		if (!bw->zstream)
>  			return REFTABLE_OUT_OF_MEMORY_ERROR;
> -		deflateInit(bw->zstream, 9);
> +		if (deflateInit(bw->zstream, 9) != Z_OK)
> +			return REFTABLE_ZLIB_ERROR;
>  	}

Presumably bw->zstream occupies some memory allocated on the heap.
Does a failing deflateInit() release it?  If not, do we leak memory
here?    Or do we need

		if (deflateInit(bw->zstream, 9) !+ Z_OK) {
			REFTABLE_FREE_AND_NULL(bw->zstream);
			return REFTABLE_ZLIB_ERROR;
		}

here?

Noticing and returning an error is a good first step.  The only
caller of it is reftable/writer.c:writer_reinit_block_writer(), and
it checks and relays the error code from here to its callers, but
not all callers of it check the error condition.  The most blatant
offender being reftable_writer_new() that happily keeps going.  I do
not know if we end up calling zlib on bw->zstream for such a broken
block_writer(), as I didn't trace the call graph fully myself.

Stepping back a bit, if REFTABLE_CALLOC_ARRAY() fails, bw->zstream
would be NULL, and a caller that does not check the return value of
writer_reinit_block_writer() would be holding a block writer whose
zstream is NULL.  If the block writer is eventually passed to the
block_writer_release() function, we would call deflateEnd() on it.

if (!bw->zstream)
return REFTABLE_OUT_OF_MEMORY_ERROR;
deflateInit(bw->zstream, 9);
if (deflateInit(bw->zstream, 9) != Z_OK)
return REFTABLE_ZLIB_ERROR;
}

return 0;
Expand Down
6 changes: 4 additions & 2 deletions t/unit-tests/u-reftable-table.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ void test_reftable_table__seek_once(void)
ret = reftable_table_new(&table, &source, "name");
cl_assert(!ret);

reftable_table_init_ref_iterator(table, &it);
ret = reftable_table_init_ref_iterator(table, &it);
cl_assert_equal_i(ret, 0);
ret = reftable_iterator_seek_ref(&it, "");
cl_assert(!ret);
ret = reftable_iterator_next_ref(&it, &ref);
Expand Down Expand Up @@ -71,7 +72,8 @@ void test_reftable_table__reseek(void)
ret = reftable_table_new(&table, &source, "name");
cl_assert(!ret);

reftable_table_init_ref_iterator(table, &it);
ret = reftable_table_init_ref_iterator(table, &it);
cl_assert_equal_i(ret, 0);

for (size_t i = 0; i < 5; i++) {
ret = reftable_iterator_seek_ref(&it, "");
Expand Down
6 changes: 5 additions & 1 deletion transport-helper.c
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,8 @@ static int get_exporter(struct transport *transport,
/* we need to duplicate helper->in because we want to use it after

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 Tue, Jul 14, 2026 at 10:48:40PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/transport-helper.c b/transport-helper.c
> index 80f90eb7ba..31883b244e 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -487,6 +487,8 @@ static int get_exporter(struct transport *transport,
>  	/* we need to duplicate helper->in because we want to use it after
>  	 * fastexport is done with it. */
>  	fastexport->out = dup(helper->in);
> +	if (fastexport->out < 0)
> +		return error_errno(_("could not dup helper output fd"));
>  	strvec_push(&fastexport->args, "fast-export");
>  	strvec_push(&fastexport->args, "--use-done-feature");
>  	strvec_push(&fastexport->args, data->signed_tags ?

Makes sense. The only caller already knows to die in case it sees a
non-zero return value.

Patrick

* fastexport is done with it. */
fastexport->out = dup(helper->in);
if (fastexport->out < 0)
return error_errno(_("could not dup helper output fd"));
strvec_push(&fastexport->args, "fast-export");
strvec_push(&fastexport->args, "--use-done-feature");
strvec_push(&fastexport->args, data->signed_tags ?
Expand Down Expand Up @@ -1182,7 +1184,9 @@ static int push_refs_with_export(struct transport *transport,

if (data->export_marks) {
strbuf_addf(&buf, "%s.tmp", data->export_marks);
rename(buf.buf, data->export_marks);
if (rename(buf.buf, data->export_marks))
warning_errno(_("could not rename '%s' to '%s'"),
buf.buf, data->export_marks);
strbuf_release(&buf);
}

Expand Down
Loading