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
