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

From: Jeff King <peff@peff.net>
Date: Apr 5, 2026, 06:14 UTC
Message-ID: <20260405061430.GA1451922@coredump.intra.peff.net>
In-Reply-To: <7841c013-1f6e-49e3-9f47-f25044764a11@malon.dev>

On Sun, Apr 05, 2026 at 10:48:16AM +0800, Tian Yuchen wrote:

Show 14 quoted lines

> > 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?
> > 
> 
> In a nutshell, are you suggesting that odb_transaction_begin() should return
> silently when source is NULL?
> 
> I understand and agree with your intention, but in practical terms, if the
> odb has not been initialised correctly, data will be lost silently here and
> the caller will not receive an error message. I’m concerned that doing this
> might lead to more bugs in certain situations

The source of data loss is not the transaction begin/end, though, but the actual write operation inside it. If I do this:

  t = odb_transaction_begin(the_repository->objects);
  if (startup_info->have_repository)
	odb_write_object(the_repository->objects, ...);
  odb_transaction_commit(t);

should it be an error? We chose not to write because we knew we did not have an odb, so there is no data to be lost. Starting or ending a transaction without an odb _is_ a noop, because there is nothing that can or will be written. It is asking to write that is the error.

In the example above you could obviously push the transaction begin/end into the conditional. But that's harder when the "if" part is happening inside a helper function, which is exactly what's going on in index_fd() here. It passes the flags (which would omit INDEX_WRITE_OBJECT) down to index_blob_packfile_transaction(), which then goes through all its regular motions without touching the odb.

This used to work because begin_odb_transaction() did quietly return NULL, and index_fd() did not have to care about the flag at all.

> Would it make more sense to treat this as a bug instead, e.g. by
> triggering a BUG() when source is NULL?

I don't think so, if the point is to be kind to callers. We should still BUG() on an actual write when there is no odb or repo, of course.

I looked over the other callers of odb_transaction_begin() and I think this is probably the only affected caller. But I don't think it's purely academic. If you teach index_fd() to use index_core() when the INDEX_WRITE_OBJECT is not set (as proposed earlier in this thread), I think you'll fall afoul of the comment at the top of the function:

          /*
           * Call xsize_t() only when needed to avoid potentially unnecessary
           * die() for large files.
           */

E.g., on a 32-bit system we'd die() on a 4GB file now (because we can't fit its size in a size_t anymore), and probably even smaller (because we'd try to mmap() it and there wouldn't be a large enough contiguous chunk of the address space).

So we really do want to follow the streaming path, even if we're not writing out the object.

-Peff