Opening multiple instance of the same MIDX can lead to problems like two
separate packed_git structures which represent the same pack being added
to the repository's object store.
The above scenario can happen because prepare_midx_pack() checks if
`m->packs[pack_int_id]` is NULL in order to determine if a pack has been
opened and installed in the repository before. But a caller can
construct two copies of the same MIDX by calling get_multi_pack_index()
and load_multi_pack_index() since the former manipulates the
object store directly but the latter is a lower-level routine which
allocates a new MIDX for each call.
So if prepare_midx_pack() is called on multiple MIDXs with the same
pack_int_id, then that pack will be installed twice in the object
store's packed_git pointer.
This can lead to problems in, for e.g., the pack-bitmap code, which does
something like the following (in pack-bitmap.c:open_pack_bitmap()):
struct bitmap_index *bitmap_git = ...;
for (p = get_all_packs(r); p; p = p->next) {
if (open_pack_bitmap_1(bitmap_git, p) == 0)
ret = 0;
}
which is a problem if two copies of the same pack exist in the
packed_git list because pack-bitmap.c:open_pack_bitmap_1() contains a
conditional like the following:
if (bitmap_git->pack || bitmap_git->midx) {
/* ignore extra bitmap file; we can only handle one */
warning("ignoring extra bitmap file: %s", packfile->pack_name);
close(fd);
return -1;
}
Avoid this scenario by not letting write_midx_internal() open a MIDX
that isn't also pointed at by the object store. So long as this is the
case, other routines should prefer to open MIDXs with
get_multi_pack_index() or reprepare_packed_git() instead of creating
instances on their own. Because get_multi_pack_index() returns
`r->object_store->multi_pack_index` if it is non-NULL, we'll only have
one instance of a MIDX open at one time, avoiding these problems.
To encourage this, drop the `struct multi_pack_index *` parameter from
`write_midx_internal()`, and rely instead on the `object_dir` to find
(or initialize) the correct MIDX instance.
Likewise, replace the call to `close_midx()` with
`close_object_store()`, since we're about to replace the MIDX with a new
one and should invalidate the object store's memory of any MIDX that
might have existed beforehand.
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
---
midx.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/midx.c b/midx.c
index 18e1949613..d67d7f383d 100644
--- a/midx.c
+++ b/midx.c
@@ -893,7 +893,7 @@ static int midx_checksum_valid(struct multi_pack_index *m)
return hashfile_checksum_valid(m->data, m->data_len);
}
-static int write_midx_internal(const char *object_dir, struct multi_pack_index *m,
+static int write_midx_internal(const char *object_dir,
struct string_list *packs_to_drop,
const char *preferred_pack_name,
unsigned flags)
@@ -904,6 +904,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
struct hashfile *f = NULL;
struct lock_file lk;
struct write_midx_context ctx = { 0 };
+ struct multi_pack_index *cur;
int pack_name_concat_len = 0;
int dropped_packs = 0;
int result = 0;
@@ -914,10 +915,14 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
die_errno(_("unable to create leading directories of %s"),
midx_name);
- if (m)
- ctx.m = m;
- else
- ctx.m = load_multi_pack_index(object_dir, 1);
+ for (cur = get_multi_pack_index(the_repository); cur; cur = cur->next) {
+ if (!strcmp(object_dir, cur->object_dir)) {
+ ctx.m = cur;
+ break;
+ }
+ }
+ if (!ctx.m)
+ ctx.m = get_local_multi_pack_index(the_repository);
if (ctx.m && !midx_checksum_valid(ctx.m)) {
warning(_("ignoring existing multi-pack-index; checksum mismatch"));
@@ -1182,8 +1187,7 @@ int write_midx_file(const char *object_dir,
const char *preferred_pack_name,
unsigned flags)
{
- return write_midx_internal(object_dir, NULL, NULL, preferred_pack_name,
- flags);
+ return write_midx_internal(object_dir, NULL, preferred_pack_name, flags);
}
struct clear_midx_data {
@@ -1460,8 +1464,10 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla
free(count);
- if (packs_to_drop.nr)
- result = write_midx_internal(object_dir, m, &packs_to_drop, NULL, flags);
+ if (packs_to_drop.nr) {
+ result = write_midx_internal(object_dir, &packs_to_drop, NULL, flags);
+ m = NULL;
+ }
string_list_clear(&packs_to_drop, 0);
return result;
@@ -1650,7 +1656,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
goto cleanup;
}
- result = write_midx_internal(object_dir, m, NULL, NULL, flags);
+ result = write_midx_internal(object_dir, NULL, NULL, flags);
m = NULL;
cleanup:
--
2.31.1.163.ga65ce7f831
