{"thread":{"id":"65971","subject":"[PATCH] object-file: fix closing object stream twice","startedAt":"2026-07-10T14:54:26Z","lastAt":"2026-07-11T07:33:29Z","messageCount":2,"participants":["Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"547738","messageId":"20260710-pks-odb-stream-double-close-v1-1-d5fa233a37c7@pks.im","threadId":"65971","inReplyTo":null,"subject":"[PATCH] object-file: fix closing object stream twice","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-10T14:54:16Z","receivedAt":"2026-07-10T14:54:26Z","isPatch":true,"body":"In 10a6762719 (object-file: adapt `stream_object_signature()` to take a\nstream, 2026-02-23), we have refactored `stream_object_signature()` so\nthat it doesn't create the stream ad-hoc anymore. Instead, callers are\nexpected to pass in a stream, which allows them to construct the streams\nfrom different sources.\n\nWhile the stream was previously managed by `stream_object_signature()`,\nthe full lifecycle is now owned by the caller. Hence, it's the caller's\nresponsibility to close the stream, and the called function shouldn't do\nthat anymore.\n\nAnd while the mentioned commit did drop one call that closed the stream,\nthere's a second such call that was missed when reading from the stream\nfails. The consequence of this can be a double free of the stream.\n\nFix the bug by dropping that leftover call to `odb_read_stream_close()`.\n\nNote that it was originally discussed whether this should be treated as\na security vulnerability. But there are only two callers: once via\n`parse_object_with_flags()`, and once via `verify_packfile()`. Neither\nof these callers plays any role on the transport layer, so this issue is\nonly relevant for objects that are already available via the local\nobject database. Furthermore, a packfile that is corrupted in this way\nwould be detected when receiving the packfile, so it's not easy for an\nadversary to plant such a packfile, either. Consequently, we decided\nthat this is not covered as part of our threat model.\n\nReported-by: xuqing yang <rigelyoung@icloud.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nHi,\n\nthis patch fixes a double-free of object streams introduced via\n10a6762719 (object-file: adapt `stream_object_signature()` to take a\nstream, 2026-02-23). It was reported to the security mailing list, but\nbecause we couldn't find a way to abuse this issue remotely we decided\nthat the issue can be fixed in the open.\n\nThe fix is built on top of v2.54.0, which is where this issue was\nintroduced. It merges cleanly to \"master\".\n\nThanks!\n\nPatrick\n---\n object-file.c   |  5 +----\n t/t1450-fsck.sh | 17 +++++++++++++++++\n 2 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 2acc9522df..610faba5b6 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -150,11 +150,8 @@ int stream_object_signature(struct repository *r,\n \tfor (;;) {\n \t\tchar buf[1024 * 16];\n \t\tssize_t readlen = odb_read_stream_read(st, buf, sizeof(buf));\n-\n-\t\tif (readlen < 0) {\n-\t\t\todb_read_stream_close(st);\n+\t\tif (readlen < 0)\n \t\t\treturn -1;\n-\t\t}\n \t\tif (!readlen)\n \t\t\tbreak;\n \t\tgit_hash_update(&c, buf, readlen);\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 54e81c2636..bc326a78f6 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -538,6 +538,23 @@ test_expect_success 'rev-list --verify-objects with bad sha1' '\n \ttest_grep -q \"error: hash mismatch $(dirname $new)$(test_oid ff_2)\" out\n '\n \n+test_expect_success 'rev-list --verify-objects with truncated loose blob' '\n+\tgit init truncated-blob &&\n+\t(\n+\t\tcd truncated-blob &&\n+\t\tblob=$(test-tool genrandom one 5k | git hash-object -t blob -w --stdin) &&\n+\t\tobj=.git/objects/$(test_oid_to_path $blob) &&\n+\n+\t\t# Truncate the loose blob such that its header can still be\n+\t\t# parsed, but reading the object data fails mid-stream.\n+\t\ttest_copy_bytes 64 <\"$obj\" >obj.tmp &&\n+\t\tmv obj.tmp \"$obj\" &&\n+\n+\t\ttest_must_fail git rev-list --verify-objects \"$blob\" 2>err &&\n+\t\ttest_grep \"hash mismatch\" err\n+\t)\n+'\n+\n # An actual bit corruption is more likely than swapped commits, but\n # this provides an easy way to have commits which don't match their purported\n # hashes, but which aren't so broken we can't read them at all.\n\n---\nbase-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0\nchange-id: 20260710-pks-odb-stream-double-close-49c4b4a93f01\n\n"},{"id":"547812","messageId":"20260711073320.GA1457061@coredump.intra.peff.net","threadId":"65971","inReplyTo":"20260710-pks-odb-stream-double-close-v1-1-d5fa233a37c7@pks.im","subject":"Re: [PATCH] object-file: fix closing object stream twice","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-11T07:33:20Z","receivedAt":"2026-07-11T07:33:29Z","isPatch":true,"body":"On Fri, Jul 10, 2026 at 04:54:16PM +0200, Patrick Steinhardt wrote:\n\n> And while the mentioned commit did drop one call that closed the stream,\n> there's a second such call that was missed when reading from the stream\n> fails. The consequence of this can be a double free of the stream.\n> \n> Fix the bug by dropping that leftover call to `odb_read_stream_close()`.\n\nThanks, both the patch and the new test look good to me.\n\n> Note that it was originally discussed whether this should be treated as\n> a security vulnerability. But there are only two callers: once via\n> `parse_object_with_flags()`, and once via `verify_packfile()`. Neither\n> of these callers plays any role on the transport layer, so this issue is\n> only relevant for objects that are already available via the local\n> object database. Furthermore, a packfile that is corrupted in this way\n> would be detected when receiving the packfile, so it's not easy for an\n> adversary to plant such a packfile, either. Consequently, we decided\n> that this is not covered as part of our threat model.\n\nI think this case probably would violate our \"it is OK to clone from the\nlocal untrusted .git repo\" goal (since you could perhaps get to this\ncode path via upload-pack/pack-objects, though I didn't try it myself).\n\nBut the text in git(1)'s SECURITY section is pretty clear that it is\nmore goal than promise, and that this scenario carries extra risk\nexactly because of the increased attack surface. And that you can\nmitigate by serving from an untrusted user.\n\n-Peff\n"}]}