From 0ce4ff942125eabed3df694dc27922bec8177624 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 8 Oct 2018 08:17:03 -0700 Subject: [PATCH 1/4] midx: fix broken free() in close_midx() When closing a multi-pack-index, we intend to close each pack-file and free the struct packed_git that represents it. However, this line was previously freeing the array of pointers, not the pointer itself. This leads to a double-free issue. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- midx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/midx.c b/midx.c index f3e8dbc108..999717b96f 100644 --- a/midx.c +++ b/midx.c @@ -190,7 +190,7 @@ static void close_midx(struct multi_pack_index *m) for (i = 0; i < m->num_packs; i++) { if (m->packs[i]) { close_pack(m->packs[i]); - free(m->packs); + free(m->packs[i]); } } FREE_AND_NULL(m->packs); From 1dcd9f2043a38f0c9684d47c71b9e383942660ac Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 12 Oct 2018 10:34:19 -0700 Subject: [PATCH 2/4] midx: close multi-pack-index on repack When repacking, we may remove pack-files. This invalidates the multi-pack-index (if it exists). Previously, we removed the multi-pack-index file before removing any pack-file. In some cases, the repack command may load the multi-pack-index into memory. This may lead to later in-memory references to the non-existent pack- files. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/repack.c | 3 +-- midx.c | 15 ++++++++++++--- midx.h | 4 +++- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index c6a7943d5c..44965cbaa3 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -431,8 +431,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) char *fname, *fname_old; if (!midx_cleared) { - /* if we move a packfile, it will invalidated the midx */ - clear_midx_file(get_object_directory()); + clear_midx_file(the_repository); midx_cleared = 1; } diff --git a/midx.c b/midx.c index 999717b96f..2243c912fc 100644 --- a/midx.c +++ b/midx.c @@ -180,9 +180,13 @@ cleanup_fail: return NULL; } -static void close_midx(struct multi_pack_index *m) +void close_midx(struct multi_pack_index *m) { uint32_t i; + + if (!m) + return; + munmap((unsigned char *)m->data, m->data_len); close(m->fd); m->fd = -1; @@ -917,9 +921,14 @@ cleanup: return 0; } -void clear_midx_file(const char *object_dir) +void clear_midx_file(struct repository *r) { - char *midx = get_midx_filename(object_dir); + char *midx = get_midx_filename(r->objects->objectdir); + + if (r->objects && r->objects->multi_pack_index) { + close_midx(r->objects->multi_pack_index); + r->objects->multi_pack_index = NULL; + } if (remove_path(midx)) { UNLEAK(midx); diff --git a/midx.h b/midx.h index a210f1af2a..7e60907596 100644 --- a/midx.h +++ b/midx.h @@ -42,6 +42,8 @@ int midx_contains_pack(struct multi_pack_index *m, const char *idx_name); int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local); int write_midx_file(const char *object_dir); -void clear_midx_file(const char *object_dir); +void clear_midx_file(struct repository *r); + +void close_midx(struct multi_pack_index *m); #endif From 0465a50506023df0932fe0534fe6ac6712c0d854 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 12 Oct 2018 10:34:20 -0700 Subject: [PATCH 3/4] multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX The multi-pack-index feature is tested in isolation by t5319-multi-pack-index.sh, but there are many more interesting scenarios in the test suite surrounding pack-file data shapes and interactions. Since the multi-pack-index is an optional data structure, it does not make sense to include it by default in those tests. Instead, add a new GIT_TEST_MULTI_PACK_INDEX environment variable that enables core.multiPackIndex and writes a multi-pack-index after each 'git repack' command. This adds extra test coverage when needed. There are a few spots in the test suite that need to react to this change: * t5319-multi-pack-index.sh: there is a test that checks that 'git repack' deletes the multi-pack-index. Disable the environment variable to ensure this still happens. * t5310-pack-bitmaps.sh: One test moves a pack-file from the object directory to an alternate. This breaks the multi-pack-index, so delete the multi-pack-index at this point, if it exists. * t9300-fast-import.sh: One test verifies the number of files in the .git/objects/pack directory is exactly 8. Exclude the multi-pack-index from this count so it is still 8 in all cases. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/repack.c | 4 ++++ midx.c | 9 +++++++-- midx.h | 2 ++ t/README | 4 ++++ t/t5310-pack-bitmaps.sh | 1 + t/t5319-multi-pack-index.sh | 2 +- t/t9300-fast-import.sh | 2 +- 7 files changed, 20 insertions(+), 4 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index 44965cbaa3..26dcccdafc 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -553,6 +553,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix) if (!no_update_server_info) update_server_info(0); remove_temporary_files(); + + if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0)) + write_midx_file(get_object_directory()); + string_list_clear(&names, 0); string_list_clear(&rollback, 0); string_list_clear(&existing_packs, 0); diff --git a/midx.c b/midx.c index 2243c912fc..1a6fa96cda 100644 --- a/midx.c +++ b/midx.c @@ -338,9 +338,14 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i struct multi_pack_index *m; struct multi_pack_index *m_search; int config_value; + static int env_value = -1; - if (repo_config_get_bool(r, "core.multipackindex", &config_value) || - !config_value) + if (env_value < 0) + env_value = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0); + + if (!env_value && + (repo_config_get_bool(r, "core.multipackindex", &config_value) || + !config_value)) return 0; for (m_search = r->objects->multi_pack_index; m_search; m_search = m_search->next) diff --git a/midx.h b/midx.h index 7e60907596..228016088e 100644 --- a/midx.h +++ b/midx.h @@ -3,6 +3,8 @@ #include "repository.h" +#define GIT_TEST_MULTI_PACK_INDEX "GIT_TEST_MULTI_PACK_INDEX" + struct multi_pack_index { struct multi_pack_index *next; diff --git a/t/README b/t/README index 3ea6c85460..9d0277c338 100644 --- a/t/README +++ b/t/README @@ -327,6 +327,10 @@ GIT_TEST_COMMIT_GRAPH=, when true, forces the commit-graph to be written after every 'git commit' command, and overrides the 'core.commitGraph' setting to true. +GIT_TEST_MULTI_PACK_INDEX=, when true, forces the multi-pack- +index to be written after every 'git repack' command, and overrides the +'core.multiPackIndex' setting to true. + Naming Tests ------------ diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh index 1be3459c5b..82d7f7f6a5 100755 --- a/t/t5310-pack-bitmaps.sh +++ b/t/t5310-pack-bitmaps.sh @@ -191,6 +191,7 @@ test_expect_success 'pack-objects respects --honor-pack-keep (local bitmapped pa test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' ' mv .git/objects/pack/$packbitmap.* alt.git/objects/pack/ && + rm -f .git/objects/pack/multi-pack-index && test_when_finished "mv alt.git/objects/pack/$packbitmap.* .git/objects/pack/" && echo HEAD | git pack-objects --local --stdout --revs >3b.pack && git index-pack 3b.pack && diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index 6f56b38674..4024ff9a39 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -152,7 +152,7 @@ compare_results_with_midx "twelve packs" test_expect_success 'repack removes multi-pack-index' ' test_path_is_file $objdir/pack/multi-pack-index && - git repack -adf && + GIT_TEST_MULTI_PACK_INDEX=0 git repack -adf && test_path_is_missing $objdir/pack/multi-pack-index ' diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 40fe7e4976..59a13b6a77 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -1558,7 +1558,7 @@ test_expect_success 'O: blank lines not necessary after other commands' ' INPUT_END git fast-import actual && test_cmp expect actual From dc7d66433536b5acca653c3c5ecf9c2d91462eba Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 25 Oct 2018 12:54:05 +0000 Subject: [PATCH 4/4] packfile: close multi-pack-index in close_all_packs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whenever we delete pack-files from the object directory, we need to also delete the multi-pack-index that may refer to those objects. Sometimes, this connection is obvious, like during a repack. Other times, this is less obvious, like when gc calls a repack command and then does other actions on the objects, like write a commit-graph file. The pattern we use to avoid out-of-date in-memory packed_git structs is to call close_all_packs(). This should also call close_midx(). Since we already pass an object store to close_all_packs(), this is a nicely scoped operation. This fixes a test failure when running t6500-gc.sh with GIT_TEST_MULTI_PACK_INDEX=1. Reported-by: Szeder Gábor Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- packfile.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packfile.c b/packfile.c index 841b36182f..37fcd8f136 100644 --- a/packfile.c +++ b/packfile.c @@ -339,6 +339,11 @@ void close_all_packs(struct raw_object_store *o) BUG("want to close pack marked 'do-not-close'"); else close_pack(p); + + if (o->multi_pack_index) { + close_midx(o->multi_pack_index); + o->multi_pack_index = NULL; + } } /*