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

From: Jeff King <peff@peff.net>
Date: Sep 28, 2021, 23:49 UTC
Message-ID: <YVOqJTU5HmnXlC5P@coredump.intra.peff.net>
In-Reply-To: <YVOn3hDsb5pnxR53@coredump.intra.peff.net>

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

Show 13 quoted lines

> 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/

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