Re: [PATCH] object-file: avoid ODB transaction when not writing objects

[Junio C Hamano <gitster@pobox.com>]

Justin Tobler <jltobler@gmail.com> writes:

> This patch addresses a bug report[1] where performing git-diff(1) on
> files that exceed "core.bigFileThreshold" outside of a repository causes
> a segfault. Originally this patch was included in another series sent to
> the mailing list[2] as a preparatory refactor. Since it happens to fix
> the reported bug though, I've extracted it from that series with the
> hope of upstreaming more quickly.

OK, so the bug was introduced in fd13909e (Merge branch
'jt/odb-transaction', 2025-10-02) aka v2.52.0-rc0~91 and applying
the test part of your patch alone on v2.52 indeed fails t1517.  The
code change does not directly apply to the old codebase, so I
wiggled your patch to make it apply to v2.52 codebase.

> I wasn't entirely sure if this patch should be based on master or maint.
> I went with master, but am happy to resend if this is incorrect.

So if we wanted to fix the past releases, the attached may be where
we want to start, then adjusting backwards to the shape of the patch
you posted as we merge it up to v2.53 and v2.54 track.

I've applied this to 'maint-2.52' (done and tested), 'maint-2.53'
(done and tested), and to 'master', and made sure that the last one
matches the result of applying your patch directly on top of
'master'.

I'd appreciate it if you can give your eyeballs to the attached to
see if that is how you would fixed the bug in the original context
of v2.52 track.  If everything looks OK, then there is no need to
spend time backporting on your side.  We have everything necessary.

Thanks.

--- >8 ---
From: Justin Tobler <jltobler@gmail.com>
Date: Tue, 7 Apr 2026 15:17:30 -0500
Subject: [PATCH] object-file: avoid ODB transaction when not writing objects

In ce1661f9da (odb: add transaction interface, 2025-09-16), existing
ODB transaction logic is adapted to create a transaction interface
at the ODB layer. The intent here is for the ODB transaction
interface to eventually provide an object source agnostic means to
manage transactions.

An unintended consequence of this change though is that
`object-file.c:index_fd()` may enter the ODB transaction path even
when no object write is requested. In non-repository contexts, this
can result in a NULL dereference and segfault. One such case occurs
when running git-diff(1) outside of a repository with
"core.bigFileThreshold" forcing the streaming path in `index_fd()`:

        $ echo foo >foo
        $ echo bar >bar
        $ git -c core.bigFileThreshold=1 diff -- foo bar

In this scenario, the caller only needs to compute the object ID. Object
hashing does not require an ODB, so starting a transaction is both
unnecessary and invalid.

Fix the bug by avoiding the use of ODB transactions in `index_fd()` when
callers are only interested in computing the object hash.

Reported-by: Luca Stefani <luca.stefani.ge1@gmail.com>
Signed-off-by: Justin Tobler <jltobler@gmail.com>
[jc: adjusted to fd13909e (Merge branch 'jt/odb-transaction', 2025-10-02)]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 object-file.c           | 49 ++++++++++++++++++++++++++++++++++-------
 t/t1517-outside-repo.sh |  8 +++++++
 2 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/object-file.c b/object-file.c
index 17a236d2fe..0969e27f3d 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1599,6 +1599,34 @@ static int index_blob_packfile_transaction(struct odb_transaction *transaction,
 	return 0;
 }
 
+static int hash_blob_stream(const struct git_hash_algo *hash_algo,
+			    struct object_id *result_oid, int fd, size_t size)
+{
+	unsigned char buf[16384];
+	struct git_hash_ctx ctx;
+	unsigned header_len;
+
+	header_len = format_object_header((char *)buf, sizeof(buf),
+					  OBJ_BLOB, size);
+	hash_algo->init_fn(&ctx);
+	git_hash_update(&ctx, buf, header_len);
+
+	while (size) {
+		size_t rsize = size < sizeof(buf) ? size : sizeof(buf);
+		ssize_t read_result = read_in_full(fd, buf, rsize);
+
+		if ((read_result < 0) || ((size_t)read_result != rsize))
+			return -1;
+
+		git_hash_update(&ctx, buf, rsize);
+		size -= read_result;
+	}
+
+	git_hash_final_oid(result_oid, &ctx);
+
+	return 0;
+}
+
 int index_fd(struct index_state *istate, struct object_id *oid,
 	     int fd, struct stat *st,
 	     enum object_type type, const char *path, unsigned flags)
@@ -1620,14 +1648,19 @@ int index_fd(struct index_state *istate, struct object_id *oid,
 		ret = index_core(istate, oid, fd, xsize_t(st->st_size),
 				 type, path, flags);
 	} else {
-		struct odb_transaction *transaction;
-
-		transaction = odb_transaction_begin(the_repository->objects);
-		ret = index_blob_packfile_transaction(the_repository->objects->transaction,
-						      oid, fd,
-						      xsize_t(st->st_size),
-						      path, flags);
-		odb_transaction_commit(transaction);
+		if (flags & INDEX_WRITE_OBJECT) {
+			struct odb_transaction *transaction;
+
+			transaction = odb_transaction_begin(the_repository->objects);
+			ret = index_blob_packfile_transaction(the_repository->objects->transaction,
+							      oid, fd,
+							      xsize_t(st->st_size),
+							      path, flags);
+			odb_transaction_commit(transaction);
+		} else {
+			ret = hash_blob_stream(the_repository->hash_algo, oid,
+					       fd, xsize_t(st->st_size));
+		}
 	}
 
 	close(fd);
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index c824c1a25c..c1dbc6359a 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -93,6 +93,14 @@ test_expect_success 'diff outside repository' '
 	test_cmp expect actual
 '
 
+test_expect_success 'diff files exceeding bigFileThreshold outside repository' '
+	cd non-repo &&
+	echo foo >foo &&
+	echo bar >bar &&
+	test_must_fail git -c core.bigFileThreshold=1 diff -- foo bar >actual &&
+	test_grep "diff --git a/foo b/bar" actual
+'
+
 test_expect_success 'stripspace outside repository' '
 	nongit git stripspace -s </dev/null
 '
-- 
2.54.0-rc0-282-gaf2821fd49