object-file: fix closing object stream twice

In 10a6762719 (object-file: adapt `stream_object_signature()` to take a
stream, 2026-02-23), we have refactored `stream_object_signature()` so
that it doesn't create the stream ad-hoc anymore. Instead, callers are
expected to pass in a stream, which allows them to construct the streams
from different sources.

While the stream was previously managed by `stream_object_signature()`,
the full lifecycle is now owned by the caller. Hence, it's the caller's
responsibility to close the stream, and the called function shouldn't do
that anymore.

And while the mentioned commit did drop one call that closed the stream,
there's a second such call that was missed when reading from the stream
fails. The consequence of this can be a double free of the stream.

Fix the bug by dropping that leftover call to `odb_read_stream_close()`.

Note that it was originally discussed whether this should be treated as
a security vulnerability. But there are only two callers: once via
`parse_object_with_flags()`, and once via `verify_packfile()`. Neither
of these callers plays any role on the transport layer, so this issue is
only relevant for objects that are already available via the local
object database. Furthermore, a packfile that is corrupted in this way
would be detected when receiving the packfile, so it's not easy for an
adversary to plant such a packfile, either. Consequently, we decided
that this is not covered as part of our threat model.

Reported-by: xuqing yang <rigelyoung@icloud.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/object-file.c b/object-file.c
index 2acc9522df2daa8c3b0155dc9ed410a722e4c855..610faba5b699bf5f32e4a2b6d7f184bbfdc9fb19 100644 (file)
--- a/object-file.c
+++ b/object-file.c
@@ -150,11 +150,8 @@ int stream_object_signature(struct repository *r,
        for (;;) {
                char buf[1024 * 16];
                ssize_t readlen = odb_read_stream_read(st, buf, sizeof(buf));
-
-               if (readlen < 0) {
-                       odb_read_stream_close(st);
+               if (readlen < 0)
                        return -1;
-               }
                if (!readlen)
                        break;
                git_hash_update(&c, buf, readlen);

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 54e81c263686de387a6cfc73ca562d4d1ebeb5ae..bc326a78f6f810b58ee57911e872f24f1e2e88c2 100755 (executable)
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -538,6 +538,23 @@ test_expect_success 'rev-list --verify-objects with bad sha1' '
        test_grep -q "error: hash mismatch $(dirname $new)$(test_oid ff_2)" out
 '
 
+test_expect_success 'rev-list --verify-objects with truncated loose blob' '
+       git init truncated-blob &&
+       (
+               cd truncated-blob &&
+               blob=$(test-tool genrandom one 5k | git hash-object -t blob -w --stdin) &&
+               obj=.git/objects/$(test_oid_to_path $blob) &&
+
+               # Truncate the loose blob such that its header can still be
+               # parsed, but reading the object data fails mid-stream.
+               test_copy_bytes 64 <"$obj" >obj.tmp &&
+               mv obj.tmp "$obj" &&
+
+               test_must_fail git rev-list --verify-objects "$blob" 2>err &&
+               test_grep "hash mismatch" err
+       )
+'
+
 # An actual bit corruption is more likely than swapped commits, but
 # this provides an easy way to have commits which don't match their purported
 # hashes, but which aren't so broken we can't read them at all.