# [PATCH 0/7] Fix issues pointed out in Git for Windows by Coverity after merging v2.56.0-rc0 12 messages from 2026-09-17 to 2026-09-18. Participants: Johannes Schindelin via GitGitGadget, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/66341 ## Johannes Schindelin via GitGitGadget, 2026-09-17 17:52 Subject: [PATCH 0/7] Fix issues pointed out in Git for Windows by Coverity after merging v2.56.0-rc0 Message-ID: ``` 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 ``` ## Johannes Schindelin via GitGitGadget, 2026-09-17 17:52 Subject: [PATCH 1/7] wrapper: guard writev_in_full() against signed overflow Message-ID: In-Reply-To: ``` From: Johannes Schindelin 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. Assisted-by: GPT-5.6 Luna Signed-off-by: Johannes Schindelin --- wrapper.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/wrapper.c b/wrapper.c index 561f9ee9c9..05a9cd369c 100644 --- a/wrapper.c +++ b/wrapper.c @@ -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 ``` ## Johannes Schindelin via GitGitGadget, 2026-09-17 17:52 Subject: [PATCH 2/7] gpg-interface: make signature-prefix matching length-aware Message-ID: <3fc7774ba867a10f35f7a74424baa4527f038232.1789667556.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Johannes Schindelin 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/20240208214137.GB1090198@coredump.intra.peff.net/: 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/20240307092638.GK2080210@coredump.intra.peff.net/, 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. Assisted-by: GPT-5.6 Luna Signed-off-by: Johannes Schindelin --- gpg-interface.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 95abf1ef4e..60c315fba9 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -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 ``` ## Johannes Schindelin via GitGitGadget, 2026-09-17 17:52 Subject: [PATCH 3/7] midx: validate incremental MIDX pack IDs Message-ID: <1cf4e5ddb5996423478b7543f7978e58cfcc4eb1.1789667556.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Johannes Schindelin 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. Assisted-by: GPT-5.6 Luna Signed-off-by: Johannes Schindelin --- midx.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/midx.c b/midx.c index 6d1c548e3d..6968fc1c00 100644 --- a/midx.c +++ b/midx.c @@ -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 ``` ## Johannes Schindelin via GitGitGadget, 2026-09-17 17:52 Subject: [PATCH 4/7] rerere: do not record failed conflict resolution data Message-ID: In-Reply-To: ``` From: Johannes Schindelin `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. Assisted-by: GPT-5.6 Luna Signed-off-by: Johannes Schindelin --- rerere.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/rerere.c b/rerere.c index 1c3745d9e3..45bbe6ab2c 100644 --- a/rerere.c +++ b/rerere.c @@ -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 ``` ## Johannes Schindelin via GitGitGadget, 2026-09-17 17:52 Subject: [PATCH 5/7] t/unit-tests: check reftable iterator initialization Message-ID: In-Reply-To: ``` From: Johannes Schindelin 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. Assisted-by: GPT-5.6 Luna Signed-off-by: Johannes Schindelin --- t/unit-tests/u-reftable-table.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/unit-tests/u-reftable-table.c b/t/unit-tests/u-reftable-table.c index bd04b477a3..1e4378b2eb 100644 --- a/t/unit-tests/u-reftable-table.c +++ b/t/unit-tests/u-reftable-table.c @@ -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 ``` ## Johannes Schindelin via GitGitGadget, 2026-09-17 17:52 Subject: [PATCH 6/7] oss-fuzz: handle reftable iterator initialization failures Message-ID: In-Reply-To: ``` From: Johannes Schindelin 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. Assisted-by: GPT-5.6 Luna Signed-off-by: Johannes Schindelin --- oss-fuzz/fuzz-reftable.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/oss-fuzz/fuzz-reftable.c b/oss-fuzz/fuzz-reftable.c index c46eac2c6b..75b8ad0c3d 100644 --- a/oss-fuzz/fuzz-reftable.c +++ b/oss-fuzz/fuzz-reftable.c @@ -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 ``` ## Johannes Schindelin via GitGitGadget, 2026-09-17 17:52 Subject: [PATCH 7/7] test-read-midx: check midx_fill_entry() result Message-ID: <60599d24542722b1393483a0b0ff6eff2325ad36.1789667556.git.gitgitgadget@gmail.com> In-Reply-To: ``` From: Johannes Schindelin 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. Assisted-by: GPT-5.6 Luna Signed-off-by: Johannes Schindelin --- t/helper/test-read-midx.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c index 83b07c6236..412089563f 100644 --- a/t/helper/test-read-midx.c +++ b/t/helper/test-read-midx.c @@ -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 ``` ## Junio C Hamano, 2026-09-17 19:32 Subject: Re: [PATCH 2/7] gpg-interface: make signature-prefix matching length-aware Message-ID: In-Reply-To: <3fc7774ba867a10f35f7a74424baa4527f038232.1789667556.git.gitgitgadget@gmail.com> ``` "Johannes Schindelin via GitGitGadget" 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 pair of a strbuf. Another approach that may be simpler is to drop the slen parameter from check_signature(). ``` ## Junio C Hamano, 2026-09-17 19:34 Subject: Re: [PATCH 4/7] rerere: do not record failed conflict resolution data Message-ID: In-Reply-To: ``` "Johannes Schindelin via GitGitGadget" 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. ``` ## Johannes Schindelin, 2026-09-18 07:12 Subject: Re: [PATCH 2/7] gpg-interface: make signature-prefix matching length-aware Message-ID: In-Reply-To: ``` Hi Junio, On Thu, 17 Sep 2026, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > 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 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 ``` ## Junio C Hamano, 2026-09-18 08:59 Subject: Re: [PATCH 2/7] gpg-interface: make signature-prefix matching length-aware Message-ID: In-Reply-To: ``` Johannes Schindelin 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." ```