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

[Taylor Blau <me@ttaylorr.com>]

On Wed, Nov 25, 2020 at 07:48:54PM -0500, Jeff King wrote:
> 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