Re: [PATCH 0/2] midx: prevent against racily disappearing packs

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 26, 2020, 01:04 UTC
Message-ID: <X77/LEwWlz9fjbYM@nand.local>
In-Reply-To: <X777dl3pqioME7uM@coredump.intra.peff.net>

On Wed, Nov 25, 2020 at 07:48:54PM -0500, Jeff King wrote:

Show 19 quoted lines

> Yeah, the race reproduction in the second commit message can actually
> reproduce the segfault as well (it depends on the exact timing which
> error you get). So the segfault is in the reader, who is not checking
> the result of find_revindex_entry().
>
> Arguably every call there should be checking for NULL, but in practice
> I think it would always be a bug:
>
>   - we were somehow unable to open the index in order to generate the
>     revindex (which is what happened here). But I think we are better
>     off making sure that we can always do so, which is what this series
>     does.
>
>   - the caller asked about an object at a position beyond the number of
>     objects in the packfile. This is a bug in the caller.
>
> So we could perhaps BUG() in find_revindex_entry() instead of returning
> NULL. A quick segfault accomplishes mostly the same thing, though the
> BUG() could distinguish the two cases more clearly.

Yeah, a find_revindex_entry() that returns NULL means that the caller is probably dead in the water.

FWIW, this function gets touched by a series that I'm working on here: [1]. There, I think "returning NULL" is equivalent to "returning -1", and the problem exists there, too.

We could return a different negative number, call BUG(), or do nothing other than what's written. I don't have any strong feelings, though.

> -Peff

Thanks, Taylor

[1]: https://github.com/ttaylorr/git/blob/tb/on-disk-revindex-part-one/pack-revindex.c#L177-L201