Fix issues pointed out in Git for Windows by Coverity after merging v2.56.0-rc0

These Coverity reports are new as of this -rc cycle; Apart from the writev one, I don't think any of these are pressing, in most cases I am still puzzled why they were reported only now.

Johannes Schindelin (7):
wrapper: guard writev_in_full() against signed overflow
gpg-interface: make signature-prefix matching length-aware
midx: validate incremental MIDX pack IDs
rerere: do not record failed conflict resolution data t/unit-tests: check reftable iterator initialization
oss-fuzz: handle reftable iterator initialization failures
test-read-midx: check midx_fill_entry() result

 gpg-interface.c                 | 10 +++++-----
 midx.c                          | 12 +++++++++---
 oss-fuzz/fuzz-reftable.c        | 18 ++++++++++--------
 rerere.c                        | 30 ++++++++++++++++++++++++++----
 t/helper/test-read-midx.c       |  6 +++++-
 t/unit-tests/u-reftable-table.c |  2 +-
 wrapper.c                       |  4 ++++
 7 files changed, 60 insertions(+), 22 deletions(-)

base-commit: 12cb6293d6288865c1a133cf22accbaf99d13eb6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2231%2Fdscho%2Ffix-coverity-high-severity-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2231/dscho/fix-coverity-high-severity-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2231

gitgitgadget

From: Johannes Schindelin <[email protected]>

As Git for Windows' Coverity run after merging v2.56.0-rc0 reported, writev_in_full() keeps its cumulative successful output in an ssize_t. Although xwritev() limits each individual write to a syscall-sized amount, repeated successful writes can still exceed SSIZE_MAX. The unchecked accumulation was introduced by d70eb7f3600d (wrapper: introduce writev(3p) wrappers, 2026-08-07).

Treat an aggregate that would overflow the signed total as an I/O failure.

---
 wrapper.c | 4 ++++
 1 file changed, 4 insertions(+)

index 561f9ee9c9..05a9cd369c 100644
@@ -376,6 +376,10 @@ ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)
 			return -1;
 		}
 
+		if (signed_add_overflows(total_written, bytes_written)) {
+			errno = EOVERFLOW;
+			return -1;
+		}
 		total_written += bytes_written;
 
 		/*

gitgitgadget

From: Johannes Schindelin <[email protected]>

After merging v2.56.0-rc0 into Git for Windows, its Coverity run reported the following issue: The parse_signed_buffer() function accepts object buffers with an explicit size, while get_format_by_sig() uses starts_with(), i.e. it expects a NUL-terminated buffer. A tag object with a non-NUL-terminated payload ending in a partial signature prefix, such as a final '-' byte, could therefore cause an invalid read past the object buffer.

The observable consequences are limited to reading past the allocation. In practice it can crash Git if the read enters an unmapped page. It can also misplace the payload/signature split, corrupting the compat-hash object being written.

The older unbounded matcher predates this path, but c8762c30df5b (object-file-convert: convert tag objects when writing, 2023-10-01) exposed the defect by passing exact-sized converted tag buffers to parse_signed_buffer(). That commit first shipped in v2.45.0, so the defect has been latent in every release since.

This pattern was noticed on the mailing list in February 2024. Reviewing a patch for a very similar issue in commit.c's find_header_mem(), Jeff King observed in
https://lore.kernel.org/git/[email protected]/:

But more interestingly: even though we pass a buf/len pair to parse_signed_buffer(), it then calls get_format_by_sig() which takes only a NUL-terminated string. [...] That raises the question of whether parse_signed_buffer() has a similar walk-too-far problem. ;) The answer is no, because we feed it from a strbuf. But it's not a great pattern overall.

That reasoning surveyed the callers that existed at the time and missed c8762c30df5b (object-file-convert: convert tag objects when writing, 2023-10-01), which was four months old at that time, and does not feed from a strbuf; convert_tag_object() hands parse_signed_buffer() an exact-sized xmalloc() buffer, and the concern flagged and dismissed in that thread is exactly the defect Coverity now reports.

Jeff went on to add starts_with_mem() a month later, in
https://lore.kernel.org/git/[email protected]/,
precisely for "cases where the buffer is not NUL-terminated (and we instead have an explicit size or end pointer)", so the tool for this fix has been in the tree since v2.45.0.

Even though the issue had been latent, it most likely surfaced via Coverity because of 215d305f450f (odb: compute compat object ID in odb_write_object_ext(), 2026-07-17), which moved convert_object_file() out of the source->write_object function pointer into a direct call in odb_write_object_ext().

Preserve the existing NUL-terminated behavior for callers that provide strings while making signature-prefix matching honor the known buffer lengths, via the starts_with_mem() helper. This keeps reads within the object data without implying exploitability beyond the observed invalid read.

---
 gpg-interface.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 95abf1ef4e..60c315fba9 100644
@@ -133,20 +133,20 @@ static struct gpg_format *get_format_by_name(const char *str)
 	return NULL;
 }
 
-static struct gpg_format *get_format_by_sig(const char *sig)
+static struct gpg_format *get_format_by_sig(const char *sig, size_t len)
 {
 	int j;
 
 	for (size_t i = 0; i < ARRAY_SIZE(gpg_format); i++)
 		for (j = 0; gpg_format[i].sigs[j]; j++)
-			if (starts_with(sig, gpg_format[i].sigs[j]))
+			if (starts_with_mem(sig, len, gpg_format[i].sigs[j]))
 				return gpg_format + i;
 	return NULL;
 }
 
 const char *get_signature_format(const char *buf)
 {
-	struct gpg_format *format = get_format_by_sig(buf);
+	struct gpg_format *format = get_format_by_sig(buf, strlen(buf));
 	return format ? format->name : "unknown";
 }
 

@@ -669,7 +669,7 @@ int check_signature(struct signature_check *sigc,
 	sigc->result = 'N';
 	sigc->trust_level = TRUST_UNDEFINED;
 
-	fmt = get_format_by_sig(signature);
+	fmt = get_format_by_sig(signature, slen);
 	if (!fmt)
 		die(_("bad/incompatible signature '%s'"), signature);
 

@@ -706,7 +706,7 @@ size_t parse_signed_buffer(const char *buf, size_t size)
 	while (len < size) {
 		const char *eol;
 
-		if (get_format_by_sig(buf + len))
+		if (get_format_by_sig(buf + len, size - len))
 			match = len;
 
 		eol = memchr(buf + len, '\n', size - len);

gitgitgadget

From: Johannes Schindelin <[email protected]>

Incremental MIDX support made object-offset pack IDs local to each layer and then converted them to chain-global IDs by adding num_packs_in_base. The conversion was introduced by 19419821bac5 (midx: teach nth_midxed_pack_int_id() about incremental MIDXs, 2024-08-06). Chain-aware pack preparation followed in 1820bd878c62 (midx: teach prepare_midx_pack() about incremental MIDXs, 2024-08-06), but the final midx_fill_entry() lookup remained tied to the original layer. Only with 8f909ff4e9e8 (packfile: recover when a multi-pack-index names a removed pack, 2026-08-29) did Coverity point out this issue: a local ID such as UINT32_MAX could wrap when the base-pack count was added, producing a plausible but incorrect global ID. After prepare_midx_pack() resolved the chain, midx_fill_entry() could then underflow or address the wrong layer while indexing the current layer's pack array, causing an invalid memory access and crashing Git.

Validate each local pack ID against its layer's pack count before adding the base count, and obtain the final pack through nth_midxed_pack(), which resolves the correct MIDX layer. This prevents an invalid local ID from wrapping during conversion and ensures that the lookup uses the layer identified by the resolved chain-global ID.

---
 midx.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

index 6d1c548e3d..6968fc1c00 100644
@@ -583,10 +583,16 @@ off_t nth_midxed_offset(struct multi_pack_index *m, uint32_t pos)
 
 uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)
 {
+	uint32_t pack_int_id;
+
 	pos = midx_for_object(&m, pos);
+	pack_int_id = get_be32(m->chunk_object_offsets +
+			       (off_t)pos * MIDX_CHUNK_OFFSET_WIDTH);
+	if (pack_int_id >= m->num_packs)
+		die(_("bad pack-int-id: %"PRIu32" (%"PRIu32" total packs)"),
+		    pack_int_id, m->num_packs);
 
-	return m->num_packs_in_base + get_be32(m->chunk_object_offsets +
-					       (off_t)pos * MIDX_CHUNK_OFFSET_WIDTH);
+	return m->num_packs_in_base + pack_int_id;
 }
 
 enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,

@@ -606,7 +612,7 @@ enum midx_fill_result midx_fill_entry(struct multi_pack_index *m,
 
 	if (prepare_midx_pack(m, pack_int_id))
 		return MIDX_FILL_OWNER_UNAVAILABLE;
-	p = m->packs[pack_int_id - m->num_packs_in_base];
+	p = nth_midxed_pack(m, pack_int_id);
 
 	/*
 	* We are about to tell the caller where they can locate the

gitgitgadget

From: Johannes Schindelin <[email protected]>

rerere can mark a conflict variant as resolved even when writing its preimage or postimage fails. A later invocation may then replay incomplete data from the cache, turning a local filesystem failure into an incorrect working-tree change.

629716d256a7 (rerere: do use multiple variants, 2015-07-30) introduced the code paths without checks for those I/O results. Treat such failures as failures, report them, and leave the rerere status unchanged unless the corresponding data was recorded successfully.

The defect has been latent since 2015. Git for Windows' Coverity run only reported it after merging v2.56.0-rc0, for reasons that could not be figured out in a reasonable amount of time.

---
 rerere.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

index 1c3745d9e3..45bbe6ab2c 100644
@@ -476,8 +476,11 @@ static int handle_file(struct index_state *istate,
 			unlink_or_warn(output);
 		return error(_("could not parse conflict hunks in '%s'"), path);
 	}
-	if (io.io.wrerror)
+	if (io.io.wrerror) {
+		if (output)
+			unlink_or_warn(output);
 		return -1;
+	}
 	return has_conflicts;
 }
 

@@ -729,8 +732,25 @@ static void do_rerere_one_path(struct index_state *istate,
 
 	/* Has the user resolved it already? */
 	if (variant >= 0) {
-		if (!handle_file(istate, path, NULL, NULL)) {
-			copy_file(the_repository, rerere_path(&buf, id, "postimage"), path, 0666);
+		int ret = handle_file(istate, path, NULL, NULL);
+
+		if (ret < 0)
+			goto out;
+		if (!ret) {
+			const int had_postimage =
+				id->collection->status[variant] & RR_HAS_POSTIMAGE;
+			const char *postimage =
+				rerere_path(&buf, id, "postimage");
+
+			if (copy_file(the_repository,
+				      postimage,
+				      path, 0666)) {
+				if (!had_postimage)
+					unlink_or_warn(postimage);
+				error_errno(_("could not copy resolution for '%s'"),
+					    path);
+				goto out;
+			}
 			id->collection->status[variant] |= RR_HAS_POSTIMAGE;
 			fprintf_ln(stderr, _("Recorded resolution for '%s'."), path);
 			free_rerere_id(rr_item);

@@ -778,7 +798,9 @@ static void do_rerere_one_path(struct index_state *istate,
 	assign_variant(id);
 
 	variant = id->variant;
-	handle_file(istate, path, NULL, rerere_path(&buf, id, "preimage"));
+	if (handle_file(istate, path, NULL,
+			rerere_path(&buf, id, "preimage")) < 0)
+		goto out;
 	if (id->collection->status[variant] & RR_HAS_POSTIMAGE) {
 		const char *path = rerere_path(&buf, id, "postimage");
 		if (unlink(path))

gitgitgadget

From: Johannes Schindelin <[email protected]>

Coverity pointed out that the test_reftable_table__seek_invalid_log_offset() test, which was introduced by a1c085df8dcb (reftable/table: fix NULL pointer access when seeking to bogus offsets, 2026-07-03), ignores the result of reftable_table_init_log_iterator() and proceeds to reftable_iterator_seek_log(), although initialization can return REFTABLE_OUT_OF_MEMORY_ERROR without installing an ops table. Under allocation failure, the test then dereferences a NULL function table.

Assert successful iterator initialization before seeking.

---
 t/unit-tests/u-reftable-table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index bd04b477a3..1e4378b2eb 100644
@@ -257,7 +257,7 @@ void test_reftable_table__seek_invalid_log_offset(void)
 	 * know that the table is corrupt, so the seek must report a format
 	 * error instead of pretending that the section is empty.
 	 */
-	reftable_table_init_log_iterator(table, &it);
+	cl_assert_equal_i(reftable_table_init_log_iterator(table, &it), 0);
 	cl_assert_equal_i(reftable_iterator_seek_log(&it, ""),
 			  REFTABLE_FORMAT_ERROR);
 

gitgitgadget

From: Johannes Schindelin <[email protected]>

The reftable fuzzer introduced by adf45165e65b (oss-fuzz: add fuzzer for parsing reftables, 2026-07-03) ignored failures from reftable_table_init_ref_iterator() and reftable_table_init_log_iterator(). Coverity reported that under allocation failure, either constructor can return REFTABLE_OUT_OF_MEMORY_ERROR without installing an ops table, allowing a subsequent seek to dereference NULL.

Treat iterator initialization failure as a reason to skip the corresponding seek and iteration while retaining safe destruction for an uninitialized iterator.

---
 oss-fuzz/fuzz-reftable.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

index c46eac2c6b..75b8ad0c3d 100644
@@ -33,10 +33,11 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
 		struct reftable_ref_record ref = { 0 };
 		struct reftable_iterator it = { 0 };
 
-		reftable_table_init_ref_iterator(table, &it);
-		if (!reftable_iterator_seek_ref(&it, ""))
-			while (!reftable_iterator_next_ref(&it, &ref))
-				;
+		if (!reftable_table_init_ref_iterator(table, &it)) {
+			if (!reftable_iterator_seek_ref(&it, ""))
+				while (!reftable_iterator_next_ref(&it, &ref))
+					;
+		}
 
 		reftable_ref_record_release(&ref);
 		reftable_iterator_destroy(&it);

@@ -46,10 +47,11 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
 		struct reftable_log_record log = { 0 };
 		struct reftable_iterator it = { 0 };
 
-		reftable_table_init_log_iterator(table, &it);
-		if (!reftable_iterator_seek_log(&it, ""))
-			while (!reftable_iterator_next_log(&it, &log))
-				;
+		if (!reftable_table_init_log_iterator(table, &it)) {
+			if (!reftable_iterator_seek_log(&it, ""))
+				while (!reftable_iterator_next_log(&it, &log))
+					;
+		}
 
 		reftable_log_record_release(&log);
 		reftable_iterator_destroy(&it);

gitgitgadget

From: Johannes Schindelin <[email protected]>

The --show-objects mode of read_midx_file() uses the output of midx_fill_entry() without checking whether the lookup succeeded. A failed lookup or unavailable pack can leave that output unusable, allowing malformed or concurrently changed MIDX data to make this test helper crash instead of reporting a controlled error.

Reject the entry unless midx_fill_entry() returns MIDX_FILL_HIT. The unchecked call was introduced by 86d174b7246b (t/helper/test-read-midx.c: add '--show-objects', 2021-03-30); later incremental-MIDX changes expanded the possible failure modes, but this remains a test-helper robustness issue, not a production Git attack surface or an arbitrary-code-execution vulnerability.

It is unclear why Coverity reports this issue in Git for Windows only after merging v2.56.0-rc0; The issue was not reported before.

---
 t/helper/test-read-midx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index 83b07c6236..412089563f 100644
@@ -90,7 +90,11 @@ static int read_midx_file(const char *object_dir, const char *checksum,
 		for (i = 0; i < m->num_objects; i++) {
 			nth_midxed_object_oid(&oid, m,
 					      i + m->num_objects_in_base);
-			midx_fill_entry(m, &oid, &e, NULL);
+			if (midx_fill_entry(m, &oid, &e, NULL) !=
+			    MIDX_FILL_HIT) {
+				ret = error(_("failed to load pack entry"));
+				goto out;
+			}
 
 			printf("%s %"PRIu64"\t%s\n",
 			       oid_to_hex(&oid), e.offset, e.p->pack_name);

gitgitgadget

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

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

Hi Junio,

This hunk is a direct consequence of get_format_by_sig() gaining a length parameter earlier in the same patch: it now has three callers, get_signature_format(), check_signature() here, and the loop inside parse_signed_buffer() (the actual site of the bug this series fixes, Coverity issue with CID 1678690 if you want to double-check).

Once the function takes a (sig, len) pair uniformly, every one of them has to pass a length, so this call site is not optional scaffolding; it is what makes all three callers correct by construction instead of leaving two of them trusting NUL-termination and one bounds-checked.

That holds for six of the seven call sites: commit.c, tag.c, builtin/fast-import.c, fmt-merge-msg.c, and log-tree.c (twice). builtin/receive-pack.c is a minor wrinkle worth flagging: it passes push_cert.buf + bogs ("bogs" = "beginning_of_gpg_sig") and push_cert.len - bogs, an offset sub-buffer of push_cert, not that strbuf's own buf/len pair verbatim. It still ends on push_cert's own terminating NUL, so the observation holds in spirit, but strictly the pattern is "ends at some strbuf's own NUL", which is more a matter of code-review convention across call sites than something check_signature()'s own signature guarantees.

I would rather keep it right where it is, for (at least 😊) two reasons.

First, slen isn't new here: check_signature() has taken a (sigc, signature, slen) signature since 02769437e142 (ssh signing: use sigc struct to pass payload, 2021-12-09), three years before this series, so dropping it now would fold an unrelated API change into a bug fix.

Second, and this is the one that actually worries me: slen is used twice inside check_signature(), not once. Besides the get_format_by_sig() call above, the pre-existing fmt->verify_signed_buffer(sigc, fmt, signature, slen) a few lines down depends on it too (there it is named signature_size). Both concrete implementations of that vtable member, verify_gpg_signed_buffer() and verify_ssh_signed_buffer(), use signature_size to decide exactly how many bytes to write_in_full() into the temporary file that then gets handed to gpg/ssh-keygen as the detached signature to verify. That is the authoritative byte count of the blob being verified, not a defensive nicety. If we dropped slen and let check_signature() fall back on strlen(signature), a signature blob with an embedded NUL before its logical end would get truncated before it ever reaches the external verifier: a correctness regression in the actual cryptographic verification path, not merely in the prefix-matching helper this series fixes. check_signature() has no doc comment promising signature is free of embedded NULs, so dropping slen would trade an explicit length for an implicit assumption.

As the commit message notes, we have been down this road with this exact function chain before. In February 2024, Peff concluded there was no walk-too-far problem in parse_signed_buffer() "because we feed it from a strbuf":
https://lore.kernel.org/git/[email protected]/

But that conclusion was already four months stale: c8762c30df5b (object-file-convert: convert tag objects when writing, 2023-10-01) had already added convert_tag_object() as a caller that does _not_ feed from a strbuf: the same gap this series closes. Applying the same "audit today's callers and assume it holds" reasoning to check_signature() now risks reproducing that failure mode a second time.

So I would like to keep slen and the get_format_by_sig(signature, slen) call as in the patch.

Ciao,
Johannes

Yes but the audit was "is slen our callers pass redundant?", and not "does everybody pass strbuf and we are better off passing a pionter to a strbuf?". And the answer to the former question is "yes".

And I do not quite understand or agree with the logic here.

What I was saying was to force all current *and* *future* callers to pass NUL-terminated string by removing slen.

Having said all that, I think this falls into "once the code is written (and more importantly, once it is reviewed, as that is a lot more costly part of the development process for machine written code), it is not worth going back and change it, as the difference is not large enough either way."