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

coverity: fix unchecked returns #2179

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 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
c689148
reftable: handle block-writer initialization errors
dscho Aug 6, 2026
66953a6
reftable/block: check deflateInit() return value
dscho Apr 27, 2026
a49af20
reftable tests: check reftable_table_init_ref_iterator() return
dscho Apr 27, 2026
bf06239
last-modified: handle repo_parse_commit() failures
dscho Apr 27, 2026
6e2295b
compat/pread: check initial lseek for errors
dscho May 5, 2026
689bb48
transport-helper: check dup() return in get_exporter
dscho May 5, 2026
ad6ea19
transport-helper: warn when export-marks file cannot be finalized
dscho Jun 27, 2026
7db6ac2
bisect: check strbuf_getline_lf return when reading terms
dscho May 5, 2026
aefdbe2
bisect: check get_terms return at all call sites
dscho May 5, 2026
258dbb0
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;

@gitgitgadget gitgitgadget Bot Aug 5, 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 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
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

@gitgitgadget gitgitgadget Bot Aug 5, 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, 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
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
5 changes: 4 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,10 @@ int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,
REFTABLE_CALLOC_ARRAY(bw->zstream, 1);

@gitgitgadget gitgitgadget Bot Aug 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:

> 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) {
REFTABLE_FREE_AND_NULL(bw->zstream);
return REFTABLE_ZLIB_ERROR;
}
}

return 0;
Expand Down
8 changes: 7 additions & 1 deletion reftable/writer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ int reftable_writer_new(struct reftable_writer **out,
{
struct reftable_write_options opts = {0};
struct reftable_writer *wp;
int err;

if (_opts)
opts = *_opts;
Expand Down Expand Up @@ -177,7 +178,12 @@ int reftable_writer_new(struct reftable_writer **out,
wp->opts = opts;
wp->hash_id = hash_id;
wp->flush = flush_func;
writer_reinit_block_writer(wp, REFTABLE_BLOCK_TYPE_REF);
err = writer_reinit_block_writer(wp, REFTABLE_BLOCK_TYPE_REF);
if (err < 0) {
reftable_free(wp->block);
reftable_free(wp);
return err;
}

*out = wp;

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,

@gitgitgadget gitgitgadget Bot Aug 13, 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, Aug 12, 2026 at 08:03:17AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> When push_refs_with_export() finalizes a successful push, it writes
> the fast-export marks file to a .tmp sibling and rename()s it into
> place. The return value of rename() is currently ignored. If the
> rename fails (permission denied, full disk, or an antivirus product
> locking the destination on Windows), the .tmp file is left behind
> and the existing export_marks file remains stale; the next
> fast-export operation that resumes from it then silently operates on
> inconsistent bookkeeping.

One question here would be whether we should try to unlink the file
instead if renaming it into place failed. But not doing so potentially
gives the user the ability to fix that issue. So I'm not sure whether
that's really a sensible thing to do in the first place.

In any case, the post-image of this patch is a clear improvement as we
now enable the user to act on the warning in the first place, whereas
previously they wouldn't ever learn about it until the failed rename may
cause errors. So overall I think this is okay as-is.

Patrick

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