coverity: fix unchecked returns

[dscho]

This is the next batch of fixes in response to issues reported by Coverity.

cc: Patrick Steinhardt ps@pks.im

[dscho]

/submit

[gitgitgadget]

Submitted as pull.2179.git.1784069325.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2179/dscho/coverity-fixes-unchecked-returns-v1

To fetch this version to local tag pr-2179/dscho/coverity-fixes-unchecked-returns-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2179/dscho/coverity-fixes-unchecked-returns-v1

[gitgitgadget]

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]

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]

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

[gitgitgadget]

User Patrick Steinhardt <ps@pks.im> has been added to the cc: list.

[gitgitgadget]

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]

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

[gitgitgadget]

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

[gitgitgadget]

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