[PATCH] object-file: fix closing object stream twice

[PATCH] object-file: fix closing object stream twice

[Patrick Steinhardt]

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>
---
Hi,

this patch fixes a double-free of object streams introduced via
10a6762719 (object-file: adapt `stream_object_signature()` to take a
stream, 2026-02-23). It was reported to the security mailing list, but
because we couldn't find a way to abuse this issue remotely we decided
that the issue can be fixed in the open.

The fix is built on top of v2.54.0, which is where this issue was
introduced. It merges cleanly to "master".

Thanks!

Patrick
---
 object-file.c   |  5 +----
 t/t1450-fsck.sh | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/object-file.c b/object-file.c
index 2acc9522df..610faba5b6 100644
--- 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 54e81c2636..bc326a78f6 100755
--- 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.

---
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
change-id: 20260710-pks-odb-stream-double-close-49c4b4a93f01

Re: [PATCH] object-file: fix closing object stream twice

[Jeff King]

On Fri, Jul 10, 2026 at 04:54:16PM +0200, Patrick Steinhardt wrote:

> 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()`.

Thanks, both the patch and the new test look good to me.

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

I think this case probably would violate our "it is OK to clone from the
local untrusted .git repo" goal (since you could perhaps get to this
code path via upload-pack/pack-objects, though I didn't try it myself).

But the text in git(1)'s SECURITY section is pretty clear that it is
more goal than promise, and that this scenario carries extra risk
exactly because of the increased attack surface. And that you can
mitigate by serving from an untrusted user.

-Peff