Re: [PATCH 2/2] builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects

[Jonathan Tan <jonathantanmy@google.com>]

Taylor Blau <me@ttaylorr.com> writes:
> On Fri, May 13, 2022 at 04:06:39PM -0700, Jonathan Tan wrote:
> > (An alternative to the change in this patch may be to reset *found_pack
> > to NULL when it is found that the pack is invalid, but I haven't
> > investigated all the callers to see if they can tolerate *found_pack
> > moving changing non-NULL to NULL, so the change in this patch is
> > probably more practical.)
> 
> I haven't either, but I think that this points out a flaw in the patch I
> originally posted.
> 
> Consider this:
> 
>   - `want_object_in_pack()` calls `want_found_object()` with a pack that
>     has gone away and has zero open fds, and `want_found_object()`
>     returns -1
>   - `want_object_in_pack()` continues and calls
>     `want_object_in_pack_one()` later on, with some pack that is the
>     same as `*found_pack`
>   - `want_object_in_pack_one()` then _doesn't_ call `is_pack_valid()`
>     (since `p == *found_pack`), leaving us in the same situation as
>     before.
> 
> I think that would be sufficient to hit this race even after this patch.

Ah, yes, indeed this would be a problem.

> I'll take a look to see if `want_object_in_pack()` callers can handle
> `*found_pack` being set back to NULL. They should be able to, but I want
> to do a little more careful analysis to be sure.

Sounds good.

> Thanks for pointing this out, I am so glad for your review! :-)

Thanks for your kind words! Thanks for your explanations too.