[PATCH 00/11] coverity: fix unchecked returns

[PATCH 00/11] coverity: fix unchecked returns

[Johannes Schindelin via GitGitGadget]

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

Johannes Schindelin (11):
  http: die on curl_easy_duphandle failure in get_active_slot
  config: propagate launch_editor() failure in show_editor()
  reftable/block: check deflateInit() return value
  reftable tests: check reftable_table_init_ref_iterator() return
  last-modified: handle repo_parse_commit() failures
  compat/pread: check initial lseek for errors
  transport-helper: check dup() return in get_exporter
  transport-helper: warn when export-marks file cannot be finalized
  bisect: check strbuf_getline_lf return when reading terms
  bisect: check get_terms return at all call sites
  bisect: handle dup() failure when redirecting stdout

 bisect.c                        |  6 ++++--
 builtin/bisect.c                | 27 +++++++++++++++++++++++++--
 builtin/config.c                |  5 ++++-
 builtin/last-modified.c         |  9 ++++++---
 compat/pread.c                  |  2 ++
 http.c                          |  2 ++
 reftable/block.c                |  3 ++-
 t/unit-tests/u-reftable-table.c |  6 ++++--
 transport-helper.c              |  6 +++++-
 9 files changed, 54 insertions(+), 12 deletions(-)


base-commit: 55526a18268bbc1ddaf8a6b7850c33d984eac9e9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2179%2Fdscho%2Fcoverity-fixes-unchecked-returns-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2179/dscho/coverity-fixes-unchecked-returns-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2179
-- 
gitgitgadget

[PATCH 01/11] http: die on curl_easy_duphandle failure in get_active_slot

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

get_active_slot() duplicates the default curl handle via
curl_easy_duphandle() to create a per-slot session handle. The
return value is stored directly in slot->curl without checking
for NULL. curl_easy_duphandle() can return NULL when memory
allocation fails internally, and the libcurl documentation
explicitly states this possibility.

When this happens, slot->curl is NULL and the very next operation
(curl_easy_setopt on line 1632 for CURLOPT_COOKIEFILE) passes
NULL as the curl handle, which is undefined behavior in libcurl
and typically crashes.

Every HTTP operation in git goes through get_active_slot(), so
this affects all remote-https, remote-http, and HTTP-based
operations (clone, fetch, push over HTTP, bundle-uri downloads).

Add a NULL check and die() with a clear message. There is no
reasonable recovery from a failed handle duplication: the process
is out of memory and cannot perform any HTTP operation.

Pointed out by Coverity.

Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 http.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/http.c b/http.c
index b4e7b8d00b..8f1d6d1f56 100644
--- a/http.c
+++ b/http.c
@@ -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++;
 	}
 
-- 
gitgitgadget

[PATCH 02/11] config: propagate launch_editor() failure in show_editor()

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

show_editor() calls launch_editor() to open the user's editor on
the configuration file, but discards the return value and
unconditionally returns 0 (success). When the editor fails to
launch (e.g., $EDITOR is not found, or the editor exits with a
nonzero status), the caller receives no indication that anything
went wrong.

This affects "git config edit" and "git config --edit": the
command silently succeeds even when the editor could not be
started. In contrast, other editor-launching paths in git (such
as "git commit" and "git rebase --edit-todo") properly propagate
editor failures and exit with an error.

Check the return value and propagate the failure by returning -1.
The two callers (cmd_config_edit at line 1315 and the legacy
cmd_config at line 1478) both propagate this return to
handle_builtin, which translates negative returns into an error
exit.

Pointed out by Coverity.

Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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;
+	}
 	free(config_file);
 
 	return 0;
-- 
gitgitgadget

Re: [PATCH 02/11] config: propagate launch_editor() failure in show_editor()

[Patrick Steinhardt]

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

[PATCH 03/11] reftable/block: check deflateInit() return value

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

block_writer_init() allocates a z_stream and calls deflateInit()
to prepare it for compressing log records. The return value of
deflateInit() is silently discarded. If zlib initialization fails
(e.g., Z_MEM_ERROR when the system is under memory pressure), the
z_stream is left in an undefined state.

Subsequent deflate() calls in block_writer_finish() then operate
on this uninitialized stream. Depending on the zlib
implementation, this can produce silently corrupted compressed
data (which would be written to the reftable file and discovered
only when a later reader fails to inflate) or crash outright.

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;
 	}
 
 	return 0;
-- 
gitgitgadget

[PATCH 04/11] reftable tests: check reftable_table_init_ref_iterator() return

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

test_reftable_table__seek_once() and test_reftable_table__reseek()
both call reftable_table_init_ref_iterator() without checking its
return value. This function returns an int error code (0 on
success, negative on failure). Every other reftable function call
in these same tests checks the return via cl_assert_equal_i() or
cl_assert(), making this omission inconsistent.

If the iterator initialization ever fails (e.g., due to a memory
allocation failure in the reftable internals), the test would
proceed to seek and read with an uninitialized iterator, producing
misleading test results or crashes rather than a clear assertion
failure.

Check the return value via cl_assert_equal_i(ret, 0), consistent
with the surrounding code.

Pointed out by Coverity.

Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/unit-tests/u-reftable-table.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/unit-tests/u-reftable-table.c b/t/unit-tests/u-reftable-table.c
index fae478ee04..6f444f8cf9 100644
--- a/t/unit-tests/u-reftable-table.c
+++ b/t/unit-tests/u-reftable-table.c
@@ -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);
@@ -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, "");
-- 
gitgitgadget

[PATCH 05/11] last-modified: handle repo_parse_commit() failures

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

last_modified_run() and process_parent() call repo_parse_commit()
without checking the return value at three sites. When a commit
object is corrupt or unavailable (e.g., a shallow clone boundary
or a missing object in a partial clone), the parse fails and the
commit's internal fields (parents, tree, date) are not populated.

The consequences depend on which call site fails:

At line 417 (the main walk loop), c->parents stays NULL after a
failed parse. The parent-walking loop at line 440 simply does not
execute, silently treating the unparsable commit as a root commit.
This produces incorrect "last modified" results: paths changed in
ancestors beyond the corrupt commit are attributed to the wrong
commit or not reported at all.

At line 423 (the --not exclusion walk), n->parents stays NULL,
causing the exclusion walk to stop prematurely. Commits that
should be excluded from the output may be incorrectly included.

At line 293 (process_parent), the parent's tree and parents are
unavailable, so diff operations against it produce wrong results
and the parent's own ancestors are never enqueued for walking.

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().

Pointed out by Coverity.

Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/last-modified.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 5478182f2e..fe012b0c2e 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -290,7 +290,8 @@ static void process_parent(struct last_modified *lm,
 {
 	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);
 
 	/*
@@ -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)) {
-- 
gitgitgadget

Re: [PATCH 05/11] last-modified: handle repo_parse_commit() failures

[Junio C Hamano]

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

[PATCH 06/11] compat/pread: check initial lseek for errors

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

git_pread() saves the current file offset via lseek(fd, 0,
SEEK_CUR) and later restores it. If the initial lseek fails
(e.g., the fd is a pipe or otherwise non-seekable),
current_offset is -1. This negative value is later passed to
lseek(fd, -1, SEEK_SET) at line 16, which sets the file position
to an unintended location (or fails with EINVAL on some
platforms).

Check the initial lseek return value and return -1 immediately
if it fails, consistent with the error handling for the other
lseek calls in the same function.

Pointed out by Coverity.

Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/pread.c | 2 ++
 1 file changed, 2 insertions(+)

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

Re: [PATCH 06/11] compat/pread: check initial lseek for errors

[Patrick Steinhardt]

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

[PATCH 07/11] transport-helper: check dup() return in get_exporter

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

get_exporter() duplicates helper->in via dup() and stores the
result in fastexport->out. If dup() fails (fd exhaustion), it
returns -1. The child_process machinery interprets out = -1 as
"create a pipe for stdout", which would silently change the
fast-export process's output wiring: instead of sending data
back through the helper's input fd, it would write to a new pipe
that nobody reads from.

Check the return value and report the error before proceeding.

Pointed out by Coverity.

Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 transport-helper.c | 2 ++
 1 file changed, 2 insertions(+)

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 ?
-- 
gitgitgadget

Re: [PATCH 07/11] transport-helper: check dup() return in get_exporter

[Patrick Steinhardt]

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

[PATCH 08/11] transport-helper: warn when export-marks file cannot be finalized

[Johannes Schindelin via GitGitGadget]

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.

The push itself succeeded by that point, so promoting this to a
fatal error would be inappropriate. Emit warning_errno() naming both
paths so the user can recover manually, and keep returning 0.

Flagged by Coverity as CID 1427723 ("Unchecked return value").

Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 transport-helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index 31883b244e..ed0543f1ad 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1184,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);
 	}
 
-- 
gitgitgadget

[PATCH 09/11] bisect: check strbuf_getline_lf return when reading terms

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

get_terms() in builtin/bisect.c and read_bisect_terms() in
bisect.c both read the BISECT_TERMS file but do not check the
strbuf_getline_lf() return values. If the file is truncated
(e.g., a partial write from a crash or disk-full condition),
strbuf_getline_lf returns EOF and the strbuf remains empty.
strbuf_detach then returns an empty string, and the term names
silently become "" instead of the expected "bad"/"good" or
custom terms.

In get_terms(), check for EOF and return -1 on truncation,
matching the existing -1 return for a missing file.

In read_bisect_terms(), die with a descriptive message when a
line cannot be read, consistent with the die_errno for a
non-ENOENT open failure in the same function. Unlike get_terms(),
read_bisect_terms() returns void and uses die() for all error
paths, so the die is the appropriate error handling here.

Pointed out by Coverity.

Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 bisect.c         |  6 ++++--
 builtin/bisect.c | 10 ++++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index 94c7028d2a..c2ef5da462 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1019,10 +1019,12 @@ void read_bisect_terms(char **read_bad, char **read_good)
 			die_errno(_("could not read file '%s'"), filename);
 		}
 	} 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);
 	}
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;
+	}
 	terms->term_good = strbuf_detach(&str, NULL);
 
 finish:
-- 
gitgitgadget

Re: [PATCH 09/11] bisect: check strbuf_getline_lf return when reading terms

[Junio C Hamano]

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

[PATCH 10/11] bisect: check get_terms return at all call sites

[Johannes Schindelin via GitGitGadget]

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.

Pointed out by Coverity.

Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/bisect.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index fe66d84382..15a2a30f89 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -1057,6 +1057,8 @@ static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
 	*word_end = '\0'; /* NUL-terminate the word */
 
 	get_terms(terms);
+	if (!terms->term_bad || !terms->term_good)
+		return error(_("no terms defined"));
 	if (check_and_set_terms(terms, p))
 		return -1;
 
@@ -1383,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;
@@ -1417,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;
@@ -1429,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;
@@ -1443,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;
@@ -1482,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,
-- 
gitgitgadget

Re: [PATCH 10/11] bisect: check get_terms return at all call sites

[Patrick Steinhardt]

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

[PATCH 11/11] bisect: handle dup() failure when redirecting stdout

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

To capture the output of each verdict command, bisect_run()
temporarily redirects stdout to a temporary file via the classic
dup(1) / dup2() pair, restoring it afterwards. The return value of
dup(1) is not checked, however. When it fails, the saved descriptor
is -1, which is then passed to close() (the issue Coverity flags),
and the matching dup2() that is meant to restore stdout also fails,
leaving the process with stdout still pointing at the temporary file
for the remainder of the run.

Treat a failed dup(1) as a fatal error for this bisect step: close
the temporary file descriptor, report the error via error_errno(),
and break out of the loop so the existing cleanup path handles the
rest, just as on other failure paths in this function.

Reported by Coverity as CID 1508242 ("Improper use of negative
value").

Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/bisect.c | 5 +++++
 1 file changed, 5 insertions(+)

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);
 
 		res = bisect_state(terms, 1, &new_state);
-- 
gitgitgadget

Re: [PATCH 11/11] bisect: handle dup() failure when redirecting stdout

[Patrick Steinhardt]

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