Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)

On Tue, Sep 28, 2021 at 07:40:14PM -0400, Jeff King wrote:

but then fsck shows that our cache is corrupted:

  Checking object directories: 100% (256/256), done.
  error: refs/notes/textconv/foo: invalid sha1 pointer 1e9b3ecffca73411001043fe914a7ec0e7291043
  error: refs/notes/textconv/foo: invalid reflog entry 1e9b3ecffca73411001043fe914a7ec0e7291043
  dangling tree 569b6e414203d2f50bdaafc1585eae11e28afc37

Now I'll admit the textconv-cache is a pretty seldom-used feature. And
that there _probably_ aren't a lot of other ways that the diff code
would try to write objects or references. But I think it speaks to the
kind of subtle interactions we should worry about (and certainly
corrupting the repository is a pretty bad outcome, though at least in
this case it's "just" a cache and we could blow away that ref manually).

I haven't looked at the way the bulk-fsync code uses tmp_objdir at all
yet, but this is the same kind of thing to be watching out for. I've
added Neeraj to the cc. The full reproduction showing the problem with
Elijah's patch is in the replied-to mail:

  https://lore.kernel.org/git/YVOn3hDsb5pnxR53@coredump.intra.peff.net/ (local)

The batch-fsync stuff might be OK in that the goal there is presumably
not to throw away the result, but to eventually migrate it into the odb.
So the unintended interaction to look out for there might be more like:

  - some code foo() wants to write a bunch of objects; it opens up a
    tmp_objdir to batch them

  - some other code bar() deep in the call-stack wants to write an
    object for whatever reason (maybe it's a caching textconv). Its
    object ends up in the tmp_objdir, too.

  - bar() writes a ref pointing to the object's oid, thinking it has
    fully written the object.

  - foo() encounters an error and aborts, rolling back the tmp_objdir,
    including the object written by bar(). Now our ref is corrupt.

As I said, I haven't looked at how the bulk-fsync stuff uses
tmp_objdir. But if it's doing the same "globally all object writes go to
the tmpdir" then it's at risk of this sort of thing.

-Peff