Skip to content

coverity: fix unchecked returns #2179

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 11 commits into
base: master
Choose a base branch
from
Open

coverity: fix unchecked returns #2179

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e653255
http: die on curl_easy_duphandle failure in get_active_slot
dscho Apr 27, 2026
0692704
config: propagate launch_editor() failure in show_editor()
dscho Apr 27, 2026
9bf7e73
reftable/block: check deflateInit() return value
dscho Apr 27, 2026
711671c
reftable tests: check reftable_table_init_ref_iterator() return
dscho Apr 27, 2026
f728be4
last-modified: handle repo_parse_commit() failures
dscho Apr 27, 2026
b31e032
compat/pread: check initial lseek for errors
dscho May 5, 2026
1792042
transport-helper: check dup() return in get_exporter
dscho May 5, 2026
13ddcce
transport-helper: warn when export-marks file cannot be finalized
dscho Jun 27, 2026
17c382f
bisect: check strbuf_getline_lf return when reading terms
dscho May 5, 2026
c0827a7
bisect: check get_terms return at all call sites
dscho May 5, 2026
2da452e
bisect: handle dup() failure when redirecting stdout
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
6 changes: 4 additions & 2 deletions bisect.c
View file
Open in desktop
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);

@gitgitgadget gitgitgadget Bot Jul 15, 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:

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

@gitgitgadget gitgitgadget Bot Jul 20, 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):

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

}
} 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
22 changes: 20 additions & 2 deletions builtin/bisect.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
terms->term_good = strbuf_detach(&str, NULL);

finish:
Expand Down Expand Up @@ -1051,6 +1057,8 @@ static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
*word_end = '\0'; /* NUL-terminate the word */

@gitgitgadget gitgitgadget Bot Jul 15, 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 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


get_terms(terms);
if (!terms->term_bad || !terms->term_good)
return error(_("no terms defined"));
if (check_and_set_terms(terms, p))
return -1;

Expand Down Expand Up @@ -1377,6 +1385,8 @@ static int cmd_bisect__next(int argc, const char **argv UNUSED, const char *pref
return error(_("'%s' requires 0 arguments"),
"git bisect next");
get_terms(&terms);
if (!terms.term_bad || !terms.term_good)
return error(_("no terms defined"));
res = bisect_next(&terms, prefix);
free_terms(&terms);
return res;
Expand Down Expand Up @@ -1411,6 +1421,8 @@ static int cmd_bisect__skip(int argc, const char **argv, const char *prefix UNUS

set_terms(&terms, "bad", "good");
get_terms(&terms);
if (!terms.term_bad || !terms.term_good)
return error(_("no terms defined"));
res = bisect_skip(&terms, argc, argv);
free_terms(&terms);
return res;
Expand All @@ -1423,6 +1435,8 @@ static int cmd_bisect__visualize(int argc, const char **argv, const char *prefix
struct bisect_terms terms = { 0 };

get_terms(&terms);
if (!terms.term_bad || !terms.term_good)
return error(_("no terms defined"));
res = bisect_visualize(&terms, argc, argv);
free_terms(&terms);
return res;
Expand All @@ -1437,6 +1451,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 (!terms.term_bad || !terms.term_good)
return error(_("no terms defined"));
res = bisect_run(&terms, argc, argv);
free_terms(&terms);
return res;
Expand Down Expand Up @@ -1476,6 +1492,8 @@ int cmd_bisect(int argc,

set_terms(&terms, "bad", "good");
get_terms(&terms);
if (!terms.term_bad || !terms.term_good)
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
View file
Open in desktop
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)

@gitgitgadget gitgitgadget Bot Jul 15, 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 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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,8 @@ static void process_parent(struct last_modified *lm,
{

@gitgitgadget gitgitgadget Bot Jul 15, 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:

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

@gitgitgadget gitgitgadget Bot Jul 20, 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):

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;

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

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
View file
Open in desktop
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;

@gitgitgadget gitgitgadget Bot Jul 15, 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 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


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
View file
Open in desktop
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
View file
Open in desktop
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);
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
View file
Open in desktop
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
View file
Open in desktop
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

@gitgitgadget gitgitgadget Bot Jul 15, 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 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