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

[dscho]

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.

[gitgitgadget]

There is an issue in commit 957966d:
test-read-midx: check midx_fill_entry() result

• Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
  Indented lines, and lines without whitespace, are exempt

[dscho]

/submit

[gitgitgadget]

Submitted as pull.2231.git.1789667556.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2231/dscho/fix-coverity-high-severity-v1

To fetch this version to local tag pr-2231/dscho/fix-coverity-high-severity-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2231/dscho/fix-coverity-high-severity-v1

[gitgitgadget]

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

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

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

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]

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

[gitgitgadget]

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.