Re: [BUG] git diff --no-index segfaults on large files (NULL object database)

[Justin Tobler <jltobler@gmail.com>]

On 26/04/04 07:09PM, Jeff King wrote:
> Alternatively, should the odb transaction system be more forgiving here,
> and act as a noop when there is no odb?
> 
> Bisecting the segfault yields ce1661f9da (odb: add transaction
> interface, 2025-09-16). Before then, we passed around the
> object_database itself, saw that its transaction field was NULL, and
> returned immediately. After that commit, we pass the object_databse to
> odb_transaction_begin(), which narrows it to odb->sources (which is
> NULL) while passing to object_file_transaction_begin(). And then that
> function looks at source->odb to go back to the object_database! But
> the source being NULL, it segfaults.
> 
> Immediately after that commit, the switch from taking an odb to a source
> is not helpful, though I think eventually it is used to set
> transaction->base.source. But should the whole thing check for a NULL
> source and return early? Or otherwise establish some kind of noop
> transaction?
> 
> I haven't thought about the implications (nor even really looked at odb
> transaction code before). But doing it that way would fix not only this
> bug, but also other potential bugs throughout the code base when callers
> start a noop transaction.
> 
> +cc Justin (author of ce1661f9da) for any thoughts.

Thanks Peff for the cc! :)

Ok so the segfault in question can be reproduced with the following:

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

And indeed the problematic code path is the `odb_transaction_begin()`
invoked via `object-file.c:index_fd()`. In a scenario where git-diff(1)
is being executed outside of a repository and involves files that exceed
"core.bigFileThreshold", `index_blob_packfile_transaction()` is invoked
without the `INDEX_WRITE_OBJECT` flag to stream the "large" object
object and generate its hash only. I unfortunately didn't consider this
use case.

IMO it is already questionable as to why we would want to start an ODB
transaction if it is already known that the object won't be written.
IOW, if we are only interested in streaming the object to get its hash
we shouldn't have to start a transaction. As part of a patch series I'm
currently working on [1], a separate `hash_blob_stream()` helper is
introduced to just hash the blob and is used to bypass the ODB
transaction entirely. Building/testing with this patch does appear to
fix the issue.

This series is only in "seen" right now, so I doubt it would make this
next release. I don't mind extracting this patch from its current series
and submitting it as a fix if this the route we want to go though.

The other option would be as Peff suggested and to make
`odb_transaction_begin()` just return early with NULL or some type of
no-op transaction if there is no ODB set up. I do think the former
approach would be preferable though as I'm not sure there is really a
good use case for supporting ODB transactions when there isn't an ODB
set up.

-Justin

[1]: https://lore.kernel.org/git/20260402213220.2651523-5-jltobler@gmail.com/