Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dscho wants to merge 7 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ef08bba
wrapper: guard writev_in_full() against signed overflow
dscho Sep 15, 2026
3fc7774
gpg-interface: make signature-prefix matching length-aware
dscho Sep 15, 2026
1cf4e5d
midx: validate incremental MIDX pack IDs
dscho Sep 15, 2026
bd9e06e
rerere: do not record failed conflict resolution data
dscho Sep 15, 2026
bc67ad3
t/unit-tests: check reftable iterator initialization
dscho Sep 15, 2026
de28e72
oss-fuzz: handle reftable iterator initialization failures
dscho Sep 15, 2026
60599d2
test-read-midx: check midx_fill_entry() result
dscho Sep 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions gpg-interface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,20 +133,20 @@ static struct gpg_format *get_format_by_name(const char *str)
return NULL;

@gitgitgadget gitgitgadget Bot Sep 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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

All the existing callers of check_signature() pass a NUL-terminated
buffer which is <buf, len> pair of a strbuf.  Another approach that
may be simpler is to drop the slen parameter from check_signature().

@gitgitgadget gitgitgadget Bot Sep 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Johannes Schindelin wrote on the Git mailing list (how to reply to this email):

Hi Junio,

On Thu, 17 Sep 2026, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > @@ -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);

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.

> >  	if (!fmt)
> >  		die(_("bad/incompatible signature '%s'"), signature);
> 
> All the existing callers of check_signature() pass a NUL-terminated
> buffer which is <buf, len> pair of a strbuf.

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.

> Another approach that may be simpler is to drop the slen parameter from
> check_signature().

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/20240208214137.GB1090198@coredump.intra.peff.net/

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

@gitgitgadget gitgitgadget Bot Sep 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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.

> ... Applying the same "audit
> today's callers and assume it holds" reasoning to `check_signature()` now
> risks reproducing that failure mode a second time.

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

}

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";
}

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down
12 changes: 9 additions & 3 deletions midx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down
18 changes: 10 additions & 8 deletions oss-fuzz/fuzz-reftable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
30 changes: 26 additions & 4 deletions rerere.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,8 +476,11 @@ static int handle_file(struct index_state *istate,
unlink_or_warn(output);

@gitgitgadget gitgitgadget Bot Sep 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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

Good to see this one, which is the only unchecked call to the
handle_file() function, checked for an error.  Looking good.

Thanks.

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

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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))
Expand Down
6 changes: 5 additions & 1 deletion t/helper/test-read-midx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion t/unit-tests/u-reftable-table.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 4 additions & 0 deletions wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/*
Expand Down
Loading