Re: [PATCH 1/2] pack-bitmap: check preferred pack validity when opening MIDX bitmap

> Taylor Blau <me@ttaylorr.com> writes:
>
> > The pack bitmap code does this, too, since prior to bab919bd44
> > (pack-bitmap: check pack validity when opening bitmap, 2015-03-26) it
> > was vulnerable to the same race.
>
> That might be a GitHub internal reference to some other commit?
> dc1daacd (pack-bitmap: check pack validity when opening bitmap,
> 2021-07-23) is what I found.

> > Similar to bab919bd44, we could technically just add this check in
>
> ... here.  But the solution in dc1daacd is quite different from what
> we see here in the posted patch, so perhaps you are referring to
> something different.  I dunno.

> The call graph around the functions involved is
>
>   prepare_midx_bitmap_git()
>    -> open_midx_bitmap_1()
>       * opens, mmaps and closes bitmap file
>       -> load_midx_revindex()
>    -> load_bitmap()
>       -> load_reverse_index()
>          -> prepare_midx_pack()
>          -> load_pack_revindex()
>
> And prepare_midx_pack() for these packs is moved from
> load_reverse_index() to open_midx_bitmap_1() in this patch.
>
> In addition, after doing so, we call is_pack_valid() on the single
> preferred pack and return failure.
>
> Because load_bitmap() or load_reverse_index() cannot be done before
> you do open_midx_bitmap_1(), calling prepare_midx_pack() early will
> end up calling add_packed_git() on underlying packs, allowing us to
> access them even when somebody else removed them from the disk?  Is
> that the idea?

> > reuse_partial_packfile_from_bitmap(), since it's technically possible to
> > use a MIDX .bitmap without needing to open any of its packs. But it's
> > simpler to do the check as early as possible, covering all direct uses
> > of the preferred pack. Note that doing this check early requires us to
> > call prepare_midx_pack() early, too, so move the relevant part of that
> > loop from load_reverse_index() into open_midx_bitmap_1().
>
> OK.  That matches my observation above, I guess.  I do not quite get
> why it is sufficient to check only the preferred one, though.

> > @@ -353,6 +355,21 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
> >  		warning(_("multi-pack bitmap is missing required reverse index"));
> >  		goto cleanup;
> >  	}
> > +
> > +	for (i = 0; i < bitmap_git->midx->num_packs; i++) {
> > +		if (prepare_midx_pack(the_repository, bitmap_git->midx, i))
> > +			die(_("could not open pack %s"),
> > +			    bitmap_git->midx->pack_names[i]);
> > +	}
> > +
> > +	preferred = bitmap_git->midx->packs[midx_preferred_pack(bitmap_git)];
> > +	if (!is_pack_valid(preferred)) {
> > +		close(fd);
>
> This close() does not look correct.  After calling xmmap() to map
> the bitmap file to bitmap_git->map, we do not need the underlying
> file descriptor in order to use the contents of the file.  We have
> closed it already at this point.