coverity: fix unchecked returns

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

From: Johannes Schindelin <[email protected]>

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.

---
 http.c | 2 ++
 1 file changed, 2 insertions(+)

index b4e7b8d00b..8f1d6d1f56 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 builtin/config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 8d8ec0beea..1307fdb0d6 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 reftable/block.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 920b3f4486..ec81fd0493 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 t/unit-tests/u-reftable-table.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index fae478ee04..6f444f8cf9 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 builtin/last-modified.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 5478182f2e..fe012b0c2e 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 compat/pread.c | 2 ++
 1 file changed, 2 insertions(+)

index 484e6d4c71..ac7d058cb8 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 transport-helper.c | 2 ++
 1 file changed, 2 insertions(+)

index 80f90eb7ba..31883b244e 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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

---
 transport-helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 31883b244e..ed0543f1ad 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 bisect.c         |  6 ++++--
 builtin/bisect.c | 10 ++++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

index 94c7028d2a..c2ef5da462 100644
@@ -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);
 	}

index 798e28f501..fe66d84382 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 builtin/bisect.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

index fe66d84382..15a2a30f89 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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

---
 builtin/bisect.c | 5 +++++
 1 file changed, 5 insertions(+)

index 15a2a30f89..801daf8c78 100644
@@ -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

"Johannes Schindelin via GitGitGadget" <[email protected]>

			goto cleanup;

"Johannes Schindelin via GitGitGadget" <[email protected]>

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

--- >8 ---
Subject: [PATCH] fixup! last-modified: handle repo_parse_commit() failures

https://lore.kernel.org/git/[email protected]/

'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'.

index fe012b0c2e..3846244dfc 100644
@@ -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;

--- >8 ---
Subject: [PATCH] fixup! bisect: check strbuf_getline_lf return when reading
terms

https://lore.kernel.org/git/[email protected]/

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.

index fe66d84382..69ab7ea248 100644
@@ -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);

Hi Junio,

Correct.

Thanks,
Johannes

Hi Patrick,

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

Hi Junio,

Hi Patrick,

As far as I can tell, no, the case where either term->bad or term->good are NULL is not permissible.

It's a good point that we should not look at term->bad and term->good, but at the return value of get_term() instead. That's incidentally what bisect_terms() does, and we should do the same here (including the same, already-translated error message).

Thanks,
Johannes

Hi Patrick,

True. I wonder why Coverity didn't complain... funny. I changed it to also check the return value of dup2().

Thank you for your review!
Johannes

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

Changes since v1:

* The last-modified patch is now more careful to clean up a commit slab when parsing the commit failed.
* When the "good" bisect term was read successfully, but not the "bad" one, the "good" one is now cleaned up.
* Instead of detecting failed get_terms() calls indirectly, the return value is now checked.
* Failures when bisect_run() calls dup2() are now handled properly, too.

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                | 42 +++++++++++++++++++++++----------
 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, 59 insertions(+), 22 deletions(-)

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

Range-diff vs v1:

  1:  e653255de1 =  1:  e653255de1 http: die on curl_easy_duphandle failure in get_active_slot
  2:  0692704d45 =  2:  0692704d45 config: propagate launch_editor() failure in show_editor()
  3:  9bf7e737c7 =  3:  9bf7e737c7 reftable/block: check deflateInit() return value
  4:  711671c3ab =  4:  711671c3ab reftable tests: check reftable_table_init_ref_iterator() return
  5:  f728be4dac !  5:  72a74c76be last-modified: handle repo_parse_commit() failures

@@ Commit message
Pointed out by Coverity.

+ Helped-by: Junio C Hamano <[email protected]>

## builtin/last-modified.c ##
@@ builtin/last-modified.c: static int last_modified_run(struct last_modified *lm)
*/
- repo_parse_commit(lm->rev.repo, c);
+ if (repo_parse_commit(lm->rev.repo, c))
-+ continue;
++ goto cleanup;

while ((n = prio_queue_get(&not_queue))) {
struct commit_list *np;

  6:  b31e0326e7 =  6:  f0b1e13979 compat/pread: check initial lseek for errors
  7:  1792042098 =  7:  0facb9e8ca transport-helper: check dup() return in get_exporter
  8:  13ddcce053 =  8:  2b0e4f32fd transport-helper: warn when export-marks file cannot be finalized
  9:  17c382fdf4 !  9:  7f2b963103 bisect: check strbuf_getline_lf return when reading terms
     @@ Commit message
          Pointed out by Coverity.
      
          Assisted-by: Claude Opus 4.6
     +    Helped-by: Junio C Hamano <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## bisect.c ##
     @@ builtin/bisect.c: static int get_terms(struct bisect_terms *terms)
      -	strbuf_getline_lf(&str, fp);
      +	if (strbuf_getline_lf(&str, fp) == EOF) {
      +		res = -1;
     ++		FREE_AND_NULL(terms->term_bad);
      +		goto finish;
      +	}
       	terms->term_good = strbuf_detach(&str, NULL);
 10:  c0827a7947 ! 10:  9a9103096a bisect: check get_terms return at all call sites
     @@ Commit message
          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.
     +    Let's not discard the return value, but handle an error with the same
     +    message `bisect_terms()` already uses when reading the terms failed.
      
          Pointed out by Coverity.
      
     +    There is one slight complication here: One caller _needs_ the return
     +    value to indicate an error when the `BISECT_TERMS` file is absent, all
     +    the other call sites are totally okay with a "missing" `BISECT_TERMS`
     +    file. To address that, extend the function signature of `get_terms()` to
     +    indicate which behavior the caller wants.
     +
          Assisted-by: Claude Opus 4.6
     +    Helped-by: Patrick Steinhardt <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## builtin/bisect.c ##
     +@@ builtin/bisect.c: static int bisect_next_check(const struct bisect_terms *terms,
     + 	return decide_next(terms, current_term, !state.nr_good, !state.nr_bad);
     + }
     + 
     +-static int get_terms(struct bisect_terms *terms)
     ++static int get_terms(struct bisect_terms *terms, int file_missing_is_ok)
     + {
     + 	struct strbuf str = STRBUF_INIT;
     + 	FILE *fp = NULL;
     +@@ builtin/bisect.c: static int get_terms(struct bisect_terms *terms)
     + 
     + 	fp = fopen(git_path_bisect_terms(), "r");
     + 	if (!fp) {
     +-		res = -1;
     ++		res = file_missing_is_ok ? 0 : -1;
     + 		goto finish;
     + 	}
     + 
     +@@ builtin/bisect.c: finish:
     + 
     + static int bisect_terms(struct bisect_terms *terms, const char *option)
     + {
     +-	if (get_terms(terms))
     ++	if (get_terms(terms, 0))
     + 		return error(_("no terms defined"));
     + 
     + 	if (!option) {
      @@ builtin/bisect.c: static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
     + 	rev = word_end + strspn(word_end, " \t");
       	*word_end = '\0'; /* NUL-terminate the word */
       
     - 	get_terms(terms);
     -+	if (!terms->term_bad || !terms->term_good)
     +-	get_terms(terms);
     ++	if (get_terms(terms, 1))
      +		return error(_("no terms defined"));
       	if (check_and_set_terms(terms, p))
       		return -1;
       
      @@ builtin/bisect.c: static int cmd_bisect__next(int argc, const char **argv UNUSED, const char *pref
     + 	if (argc)
       		return error(_("'%s' requires 0 arguments"),
       			     "git bisect next");
     - 	get_terms(&terms);
     -+	if (!terms.term_bad || !terms.term_good)
     +-	get_terms(&terms);
     ++	if (get_terms(&terms, 1))
      +		return error(_("no terms defined"));
       	res = bisect_next(&terms, prefix);
       	free_terms(&terms);
       	return res;
      @@ builtin/bisect.c: static int cmd_bisect__skip(int argc, const char **argv, const char *prefix UNUS
     + 	struct bisect_terms terms = { 0 };
       
       	set_terms(&terms, "bad", "good");
     - 	get_terms(&terms);
     -+	if (!terms.term_bad || !terms.term_good)
     +-	get_terms(&terms);
     ++	if (get_terms(&terms, 1))
      +		return error(_("no terms defined"));
       	res = bisect_skip(&terms, argc, argv);
       	free_terms(&terms);
       	return res;
      @@ builtin/bisect.c: static int cmd_bisect__visualize(int argc, const char **argv, const char *prefix
     + 	int res;
       	struct bisect_terms terms = { 0 };
       
     - 	get_terms(&terms);
     -+	if (!terms.term_bad || !terms.term_good)
     +-	get_terms(&terms);
     ++	if (get_terms(&terms, 1))
      +		return error(_("no terms defined"));
       	res = bisect_visualize(&terms, argc, argv);
       	free_terms(&terms);
       	return res;
      @@ builtin/bisect.c: 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)
     +-	get_terms(&terms);
     ++	if (get_terms(&terms, 1))
      +		return error(_("no terms defined"));
       	res = bisect_run(&terms, argc, argv);
       	free_terms(&terms);
       	return res;
      @@ builtin/bisect.c: int cmd_bisect(int argc,
     + 			usage_with_options(git_bisect_usage, options);
       
       		set_terms(&terms, "bad", "good");
     - 		get_terms(&terms);
     -+		if (!terms.term_bad || !terms.term_good)
     +-		get_terms(&terms);
     ++		if (get_terms(&terms, 1))
      +			return error(_("no terms defined"));
       		if (check_and_set_terms(&terms, argv[0]) ||
       		    !one_of(argv[0], terms.term_good, terms.term_bad, NULL))

11: 2da452e39c ! 11: 829cd82177 bisect: handle dup() failure when redirecting stdout
@@ Commit message
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.
+ Treat a failed dup(1) or dup2(..., 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").

+ Helped-by: Patrick Steinhardt <[email protected]>

## builtin/bisect.c ##
@@ builtin/bisect.c: static int bisect_run(struct bisect_terms *terms, int argc, co

fflush(stdout);
saved_stdout = dup(1);
-+ if (saved_stdout < 0) {
+- dup2(temporary_stdout_fd, 1);
++ if (saved_stdout < 0 ||
++ dup2(temporary_stdout_fd, 1) < 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

From: Johannes Schindelin <[email protected]>

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.

---
 http.c | 2 ++
 1 file changed, 2 insertions(+)

index b4e7b8d00b..8f1d6d1f56 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 builtin/config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 8d8ec0beea..1307fdb0d6 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 reftable/block.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 920b3f4486..ec81fd0493 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 t/unit-tests/u-reftable-table.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index fae478ee04..6f444f8cf9 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 builtin/last-modified.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 5478182f2e..3846244dfc 100644
@@ -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))
+			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)) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

---
 compat/pread.c | 2 ++
 1 file changed, 2 insertions(+)

index 484e6d4c71..ac7d058cb8 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 transport-helper.c | 2 ++
 1 file changed, 2 insertions(+)

index 80f90eb7ba..31883b244e 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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

---
 transport-helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 31883b244e..ed0543f1ad 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 bisect.c         |  6 ++++--
 builtin/bisect.c | 11 +++++++++--
 2 files changed, 13 insertions(+), 4 deletions(-)

index 94c7028d2a..c2ef5da462 100644
@@ -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);
 	}

index 798e28f501..69ab7ea248 100644
@@ -498,9 +498,16 @@ 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;
+		FREE_AND_NULL(terms->term_bad);
+		goto finish;
+	}
 	terms->term_good = strbuf_detach(&str, NULL);
 
 finish:

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

Let's not discard the return value, but handle an error with the same message bisect_terms() already uses when reading the terms failed.

Pointed out by Coverity.

There is one slight complication here: One caller _needs_ the return value to indicate an error when the BISECT_TERMS file is absent, all the other call sites are totally okay with a "missing" BISECT_TERMS file. To address that, extend the function signature of get_terms() to indicate which behavior the caller wants.

---
 builtin/bisect.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

index 69ab7ea248..ceb60b0626 100644
@@ -485,7 +485,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
 	return decide_next(terms, current_term, !state.nr_good, !state.nr_bad);
 }
 
-static int get_terms(struct bisect_terms *terms)
+static int get_terms(struct bisect_terms *terms, int file_missing_is_ok)
 {
 	struct strbuf str = STRBUF_INIT;
 	FILE *fp = NULL;

@@ -493,7 +493,7 @@ static int get_terms(struct bisect_terms *terms)
 
 	fp = fopen(git_path_bisect_terms(), "r");
 	if (!fp) {
-		res = -1;
+		res = file_missing_is_ok ? 0 : -1;
 		goto finish;
 	}
 

@@ -519,7 +519,7 @@ finish:
 
 static int bisect_terms(struct bisect_terms *terms, const char *option)
 {
-	if (get_terms(terms))
+	if (get_terms(terms, 0))
 		return error(_("no terms defined"));
 
 	if (!option) {

@@ -1057,7 +1057,8 @@ static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
 	rev = word_end + strspn(word_end, " \t");
 	*word_end = '\0'; /* NUL-terminate the word */
 
-	get_terms(terms);
+	if (get_terms(terms, 1))
+		return error(_("no terms defined"));
 	if (check_and_set_terms(terms, p))
 		return -1;
 

@@ -1383,7 +1384,8 @@ static int cmd_bisect__next(int argc, const char **argv UNUSED, const char *pref
 	if (argc)
 		return error(_("'%s' requires 0 arguments"),
 			     "git bisect next");
-	get_terms(&terms);
+	if (get_terms(&terms, 1))
+		return error(_("no terms defined"));
 	res = bisect_next(&terms, prefix);
 	free_terms(&terms);
 	return res;

@@ -1417,7 +1419,8 @@ static int cmd_bisect__skip(int argc, const char **argv, const char *prefix UNUS
 	struct bisect_terms terms = { 0 };
 
 	set_terms(&terms, "bad", "good");
-	get_terms(&terms);
+	if (get_terms(&terms, 1))
+		return error(_("no terms defined"));
 	res = bisect_skip(&terms, argc, argv);
 	free_terms(&terms);
 	return res;

@@ -1429,7 +1432,8 @@ static int cmd_bisect__visualize(int argc, const char **argv, const char *prefix
 	int res;
 	struct bisect_terms terms = { 0 };
 
-	get_terms(&terms);
+	if (get_terms(&terms, 1))
+		return error(_("no terms defined"));
 	res = bisect_visualize(&terms, argc, argv);
 	free_terms(&terms);
 	return res;

@@ -1443,7 +1447,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 (get_terms(&terms, 1))
+		return error(_("no terms defined"));
 	res = bisect_run(&terms, argc, argv);
 	free_terms(&terms);
 	return res;

@@ -1482,7 +1487,8 @@ int cmd_bisect(int argc,
 			usage_with_options(git_bisect_usage, options);
 
 		set_terms(&terms, "bad", "good");
-		get_terms(&terms);
+		if (get_terms(&terms, 1))
+			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

From: Johannes Schindelin <[email protected]>

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) or dup2(..., 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").

---
 builtin/bisect.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index ceb60b0626..733d28d377 100644
@@ -1308,7 +1308,12 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
 
 		fflush(stdout);
 		saved_stdout = dup(1);
-		dup2(temporary_stdout_fd, 1);
+		if (saved_stdout < 0 ||
+		    dup2(temporary_stdout_fd, 1) < 0) {
+			res = error_errno(_("could not duplicate stdout"));
+			close(temporary_stdout_fd);
+			break;
+		}
 
 		res = bisect_state(terms, 1, &new_state);
 

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

"Johannes Schindelin via GitGitGadget" <[email protected]>

		if (deflateInit(bw->zstream, 9) !+ Z_OK) {
			REFTABLE_FREE_AND_NULL(bw->zstream);
			return REFTABLE_ZLIB_ERROR;
		}

I didn't notice it while I was looking at this part, and wondering if we can (and should) do anything if close() failed there.

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

Changes since v2:

* Added a new commit to handle block-writer initialization errors (instead of ignoring them).
* The bw->zstream attribute is now also deinitialized in the error case, as suggested by Junio.
* The commit message of "reftable/block: check deflateInit() return value" was rephrased to stop suggesting that silent corruption by zlib would be possible before that patch: This turned out to be provably incorrect.
* When aborting the bisect because dup2() failed, a left-over saved_stdout is now also cleaned up.

Changes since v1:

* The last-modified patch is now more careful to clean up a commit slab when parsing the commit failed.
* When the "good" bisect term was read successfully, but not the "bad" one, the "good" one is now cleaned up.
* Instead of detecting failed get_terms() calls indirectly, the return value is now checked.
* Failures when bisect_run() calls dup2() are now handled properly, too.

Johannes Schindelin (12):
http: die on curl_easy_duphandle failure in get_active_slot
config: propagate launch_editor() failure in show_editor()
reftable: handle block-writer initialization errors 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                | 44 ++++++++++++++++++++++++---------
 builtin/config.c                |  5 +++-
 builtin/last-modified.c         |  9 ++++---
 compat/pread.c                  |  2 ++
 http.c                          |  2 ++
 reftable/block.c                |  5 +++-
 reftable/writer.c               |  8 +++++-
 t/unit-tests/u-reftable-table.c |  6 +++--
 transport-helper.c              |  6 ++++-
 10 files changed, 70 insertions(+), 23 deletions(-)

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

Range-diff vs v2:

  1:  e653255de1 =  1:  e653255de1 http: die on curl_easy_duphandle failure in get_active_slot
  2:  0692704d45 =  2:  0692704d45 config: propagate launch_editor() failure in show_editor()
  -:  ---------- >  3:  c689148aef reftable: handle block-writer initialization errors
  3:  9bf7e737c7 !  4:  66953a65d0 reftable/block: check deflateInit() return value

@@ Commit message
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.
+ on this uninitialized stream. Current zlib/zlib-ng versions handle
+ such a stream gracefully, by returning Z_STREAM_ERROR, so in
+ practice it would likely not result in catastrophic error.

- 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.
+ The function already uses REFTABLE_ZLIB_ERROR for deflate() failures
+ later in the code path, so returning the same error code for
+ deflateInit() failure is consistent.

Pointed out by Coverity.

+ Helped-by: Junio C Hamano <[email protected]>

## reftable/block.c ##
@@ reftable/block.c: int block_writer_init(struct block_writer *bw, uint8_t typ, ui
if (!bw->zstream)
return REFTABLE_OUT_OF_MEMORY_ERROR;
- deflateInit(bw->zstream, 9);
-+ if (deflateInit(bw->zstream, 9) != Z_OK)
++ if (deflateInit(bw->zstream, 9) != Z_OK) {
++ REFTABLE_FREE_AND_NULL(bw->zstream);
+ return REFTABLE_ZLIB_ERROR;
++ }
}

return 0;

  4:  711671c3ab =  5:  a49af20d30 reftable tests: check reftable_table_init_ref_iterator() return
  5:  72a74c76be =  6:  bf06239732 last-modified: handle repo_parse_commit() failures
  6:  f0b1e13979 =  7:  6e2295b8f0 compat/pread: check initial lseek for errors
  7:  0facb9e8ca =  8:  689bb48fe5 transport-helper: check dup() return in get_exporter
  8:  2b0e4f32fd =  9:  ad6ea19737 transport-helper: warn when export-marks file cannot be finalized
  9:  7f2b963103 = 10:  7db6ac2ab0 bisect: check strbuf_getline_lf return when reading terms
 10:  9a9103096a = 11:  aefdbe2bdf bisect: check get_terms return at all call sites
 11:  829cd82177 ! 12:  258dbb0fbd bisect: handle dup() failure when redirecting stdout
     @@ builtin/bisect.c: static int bisect_run(struct bisect_terms *terms, int argc, co
      +		if (saved_stdout < 0 ||
      +		    dup2(temporary_stdout_fd, 1) < 0) {
      +			res = error_errno(_("could not duplicate stdout"));
     ++			if (saved_stdout >= 0)
     ++				close(saved_stdout);
      +			close(temporary_stdout_fd);
      +			break;
      +		}

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

---
 http.c | 2 ++
 1 file changed, 2 insertions(+)

index b4e7b8d00b..8f1d6d1f56 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 builtin/config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 8d8ec0beea..1307fdb0d6 100644
@@ -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

From: Johannes Schindelin <[email protected]>

2d5dbb37b284 (reftable/block: handle allocation failures, 2024-10-02) taught writer_reinit_block_writer() to report initialization failures and updated its callers, but reftable_writer_new() continued to ignore the return value.

Consequently, the constructor could report success after block-writer initialization had failed. Propagate the error and release the constructor's allocations instead of returning an unusable writer.

Pointed out by GPT-5.6 Sol and Claude Opus 4.8.

---
 reftable/writer.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index d969a6a021..073b9bbd89 100644
@@ -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;

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

gitgitgadget

From: Johannes Schindelin <[email protected]>

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. Current zlib/zlib-ng versions handle such a stream gracefully, by returning Z_STREAM_ERROR, so in practice it would likely not result in catastrophic error.

The function already uses REFTABLE_ZLIB_ERROR for deflate() failures later in the code path, so returning the same error code for deflateInit() failure is consistent.

Pointed out by Coverity.

---
 reftable/block.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 920b3f4486..c12fedc5a2 100644
@@ -87,7 +87,10 @@ 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) {
+			REFTABLE_FREE_AND_NULL(bw->zstream);
+			return REFTABLE_ZLIB_ERROR;
+		}
 	}
 
 	return 0;

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

---
 t/unit-tests/u-reftable-table.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index fae478ee04..6f444f8cf9 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 builtin/last-modified.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 5478182f2e..3846244dfc 100644
@@ -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))
+			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)) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

---
 compat/pread.c | 2 ++
 1 file changed, 2 insertions(+)

index 484e6d4c71..ac7d058cb8 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 transport-helper.c | 2 ++
 1 file changed, 2 insertions(+)

index 80f90eb7ba..31883b244e 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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

---
 transport-helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 31883b244e..ed0543f1ad 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 bisect.c         |  6 ++++--
 builtin/bisect.c | 11 +++++++++--
 2 files changed, 13 insertions(+), 4 deletions(-)

index 94c7028d2a..c2ef5da462 100644
@@ -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);
 	}

index 798e28f501..69ab7ea248 100644
@@ -498,9 +498,16 @@ 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;
+		FREE_AND_NULL(terms->term_bad);
+		goto finish;
+	}
 	terms->term_good = strbuf_detach(&str, NULL);
 
 finish:

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

Let's not discard the return value, but handle an error with the same message bisect_terms() already uses when reading the terms failed.

Pointed out by Coverity.

There is one slight complication here: One caller _needs_ the return value to indicate an error when the BISECT_TERMS file is absent, all the other call sites are totally okay with a "missing" BISECT_TERMS file. To address that, extend the function signature of get_terms() to indicate which behavior the caller wants.

---
 builtin/bisect.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

index 69ab7ea248..ceb60b0626 100644
@@ -485,7 +485,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
 	return decide_next(terms, current_term, !state.nr_good, !state.nr_bad);
 }
 
-static int get_terms(struct bisect_terms *terms)
+static int get_terms(struct bisect_terms *terms, int file_missing_is_ok)
 {
 	struct strbuf str = STRBUF_INIT;
 	FILE *fp = NULL;

@@ -493,7 +493,7 @@ static int get_terms(struct bisect_terms *terms)
 
 	fp = fopen(git_path_bisect_terms(), "r");
 	if (!fp) {
-		res = -1;
+		res = file_missing_is_ok ? 0 : -1;
 		goto finish;
 	}
 

@@ -519,7 +519,7 @@ finish:
 
 static int bisect_terms(struct bisect_terms *terms, const char *option)
 {
-	if (get_terms(terms))
+	if (get_terms(terms, 0))
 		return error(_("no terms defined"));
 
 	if (!option) {

@@ -1057,7 +1057,8 @@ static int process_replay_line(struct bisect_terms *terms, struct strbuf *line)
 	rev = word_end + strspn(word_end, " \t");
 	*word_end = '\0'; /* NUL-terminate the word */
 
-	get_terms(terms);
+	if (get_terms(terms, 1))
+		return error(_("no terms defined"));
 	if (check_and_set_terms(terms, p))
 		return -1;
 

@@ -1383,7 +1384,8 @@ static int cmd_bisect__next(int argc, const char **argv UNUSED, const char *pref
 	if (argc)
 		return error(_("'%s' requires 0 arguments"),
 			     "git bisect next");
-	get_terms(&terms);
+	if (get_terms(&terms, 1))
+		return error(_("no terms defined"));
 	res = bisect_next(&terms, prefix);
 	free_terms(&terms);
 	return res;

@@ -1417,7 +1419,8 @@ static int cmd_bisect__skip(int argc, const char **argv, const char *prefix UNUS
 	struct bisect_terms terms = { 0 };
 
 	set_terms(&terms, "bad", "good");
-	get_terms(&terms);
+	if (get_terms(&terms, 1))
+		return error(_("no terms defined"));
 	res = bisect_skip(&terms, argc, argv);
 	free_terms(&terms);
 	return res;

@@ -1429,7 +1432,8 @@ static int cmd_bisect__visualize(int argc, const char **argv, const char *prefix
 	int res;
 	struct bisect_terms terms = { 0 };
 
-	get_terms(&terms);
+	if (get_terms(&terms, 1))
+		return error(_("no terms defined"));
 	res = bisect_visualize(&terms, argc, argv);
 	free_terms(&terms);
 	return res;

@@ -1443,7 +1447,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 (get_terms(&terms, 1))
+		return error(_("no terms defined"));
 	res = bisect_run(&terms, argc, argv);
 	free_terms(&terms);
 	return res;

@@ -1482,7 +1487,8 @@ int cmd_bisect(int argc,
 			usage_with_options(git_bisect_usage, options);
 
 		set_terms(&terms, "bad", "good");
-		get_terms(&terms);
+		if (get_terms(&terms, 1))
+			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

From: Johannes Schindelin <[email protected]>

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) or dup2(..., 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").

---
 builtin/bisect.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index ceb60b0626..be42468af6 100644
@@ -1308,7 +1308,14 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
 
 		fflush(stdout);
 		saved_stdout = dup(1);
-		dup2(temporary_stdout_fd, 1);
+		if (saved_stdout < 0 ||
+		    dup2(temporary_stdout_fd, 1) < 0) {
+			res = error_errno(_("could not duplicate stdout"));
+			if (saved_stdout >= 0)
+				close(saved_stdout);
+			close(temporary_stdout_fd);
+			break;
+		}
 
 		res = bisect_state(terms, 1, &new_state);
 

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Everything looks sensible. I am fine with declaring victory, but does anyone want to second it?

I cannot claim to have read all of the patches carefully, but this version addressed the sole concern I raised, and in the few other patches I glanced over I didn't see anything to complain about. So maybe consider that a weak second. :)

-Peff

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

I didn't spot anything that needs to change, either. Thanks!

Patrick