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