From 3367b6657c456788e37c335bdd3225e517d2c804 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Tue, 1 Jul 2025 05:32:07 +0000 Subject: [PATCH 1/3] pack-bitmap: fix memory leak if load_bitmap() failed After going through the "failed" label, load_bitmap() will return -1, and its caller (either prepare_bitmap_walk() or prepare_bitmap_git()) will then call free_bitmap_index(). That function would have done: struct stored_bitmap *sb; kh_foreach_value(b->bitmaps, sb { ewah_pool_free(sb->root); free(sb); }); , but won't since load_bitmap() already called kh_destroy_oid_map() and NULL'd the "bitmaps" pointer from within its "failed" label. Thus if you got part of the way through loading bitmap entries and then failed, you would leak all of the previous entries that you were able to load successfully. The solution is to remove the error handling code in load_bitmap(), because its caller will always call free_bitmap_index() in case of an error. Signed-off-by: Taylor Blau Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> Signed-off-by: Junio C Hamano --- pack-bitmap.c | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/pack-bitmap.c b/pack-bitmap.c index ac6d62b980..fd19c22551 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -630,41 +630,28 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git, bitmap_git->ext_index.positions = kh_init_oid_pos(); if (load_reverse_index(r, bitmap_git)) - goto failed; + return -1; if (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) || !(bitmap_git->trees = read_bitmap_1(bitmap_git)) || !(bitmap_git->blobs = read_bitmap_1(bitmap_git)) || !(bitmap_git->tags = read_bitmap_1(bitmap_git))) - goto failed; + return -1; if (!bitmap_git->table_lookup && load_bitmap_entries_v1(bitmap_git) < 0) - goto failed; + return -1; if (bitmap_git->base) { if (!bitmap_is_midx(bitmap_git)) BUG("non-MIDX bitmap has non-NULL base bitmap index"); if (load_bitmap(r, bitmap_git->base, 1) < 0) - goto failed; + return -1; } if (!recursing) load_all_type_bitmaps(bitmap_git); return 0; - -failed: - munmap(bitmap_git->map, bitmap_git->map_size); - bitmap_git->map = NULL; - bitmap_git->map_size = 0; - - kh_destroy_oid_map(bitmap_git->bitmaps); - bitmap_git->bitmaps = NULL; - - kh_destroy_oid_pos(bitmap_git->ext_index.positions); - bitmap_git->ext_index.positions = NULL; - - return -1; } static int open_pack_bitmap(struct repository *r, From 73bf771b958b0d28fc5839cd94c4b7ad2029f631 Mon Sep 17 00:00:00 2001 From: Lidong Yan <502024330056@smail.nju.edu.cn> Date: Tue, 1 Jul 2025 05:32:08 +0000 Subject: [PATCH 2/3] pack-bitmap: reword comments in test_bitmap_commits() The comment in pack-bitmap.c:test_bitmap_commits(), suggests that we can avoid reading the commit table altogether. However, this comment is misleading. The reason we load bitmap entries here is because test_bitmap_commits() needs to print the commit IDs from the bitmap, and we must read the bitmap entries to obtain those commit IDs. So reword this comment. Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> Signed-off-by: Junio C Hamano --- pack-bitmap.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pack-bitmap.c b/pack-bitmap.c index fd19c22551..d1d8814be7 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -2839,8 +2839,9 @@ int test_bitmap_commits(struct repository *r) die(_("failed to load bitmap indexes")); /* - * As this function is only used to print bitmap selected - * commits, we don't have to read the commit table. + * Since this function needs to print the bitmapped + * commits, bypass the commit lookup table (if one exists) + * by forcing the bitmap to eagerly load its entries. */ if (bitmap_git->table_lookup) { if (load_bitmap_entries_v1(bitmap_git) < 0) From bfd5522e98cead32d7bbdf54eca4ffeb3e01fa6b Mon Sep 17 00:00:00 2001 From: Lidong Yan <502024330056@smail.nju.edu.cn> Date: Tue, 1 Jul 2025 05:32:09 +0000 Subject: [PATCH 3/3] pack-bitmap: add load corrupt bitmap test t5310 lacks a test to ensure git works correctly when commit bitmap data is corrupted. So this patch add test helper in pack-bitmap.c to list each commit bitmap position in bitmap file and `load corrupt bitmap` test case in t/t5310 to corrupt a commit bitmap before loading it. Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn> Signed-off-by: Junio C Hamano --- pack-bitmap.c | 62 +++++++++++++++++++++++++++++++++++++---- pack-bitmap.h | 1 + t/helper/test-bitmap.c | 8 ++++++ t/t5310-pack-bitmaps.sh | 30 ++++++++++++++++++++ 4 files changed, 96 insertions(+), 5 deletions(-) diff --git a/pack-bitmap.c b/pack-bitmap.c index d1d8814be7..d707bc52f7 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -31,6 +31,7 @@ struct stored_bitmap { struct object_id oid; struct ewah_bitmap *root; struct stored_bitmap *xor; + size_t map_pos; int flags; }; @@ -314,13 +315,14 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index, struct ewah_bitmap *root, const struct object_id *oid, struct stored_bitmap *xor_with, - int flags) + int flags, size_t map_pos) { struct stored_bitmap *stored; khiter_t hash_pos; int ret; stored = xmalloc(sizeof(struct stored_bitmap)); + stored->map_pos = map_pos; stored->root = root; stored->xor = xor_with; stored->flags = flags; @@ -376,10 +378,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index) struct stored_bitmap *xor_bitmap = NULL; uint32_t commit_idx_pos; struct object_id oid; + size_t entry_map_pos; if (index->map_size - index->map_pos < 6) return error(_("corrupt ewah bitmap: truncated header for entry %d"), i); + entry_map_pos = index->map_pos; commit_idx_pos = read_be32(index->map, &index->map_pos); xor_offset = read_u8(index->map, &index->map_pos); flags = read_u8(index->map, &index->map_pos); @@ -402,8 +406,9 @@ static int load_bitmap_entries_v1(struct bitmap_index *index) if (!bitmap) return -1; - recent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap( - index, bitmap, &oid, xor_bitmap, flags); + recent_bitmaps[i % MAX_XOR_OFFSET] = + store_bitmap(index, bitmap, &oid, xor_bitmap, flags, + entry_map_pos); } return 0; @@ -869,6 +874,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_ int xor_flags; khiter_t hash_pos; struct bitmap_lookup_table_xor_item *xor_item; + size_t entry_map_pos; if (is_corrupt) return NULL; @@ -928,6 +934,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_ goto corrupt; } + entry_map_pos = bitmap_git->map_pos; bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t); xor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos); bitmap = read_bitmap_1(bitmap_git); @@ -935,7 +942,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_ if (!bitmap) goto corrupt; - xor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid, xor_bitmap, xor_flags); + xor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid, + xor_bitmap, xor_flags, entry_map_pos); xor_items_nr--; } @@ -969,6 +977,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_ * Instead, we can skip ahead and immediately read the flags and * ewah bitmap. */ + entry_map_pos = bitmap_git->map_pos; bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t); flags = read_u8(bitmap_git->map, &bitmap_git->map_pos); bitmap = read_bitmap_1(bitmap_git); @@ -976,7 +985,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_ if (!bitmap) goto corrupt; - return store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags); + return store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags, + entry_map_pos); corrupt: free(xor_items); @@ -2857,6 +2867,48 @@ int test_bitmap_commits(struct repository *r) return 0; } +int test_bitmap_commits_with_offset(struct repository *r) +{ + struct object_id oid; + struct stored_bitmap *stored; + struct bitmap_index *bitmap_git; + size_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos, + ewah_bitmap_map_pos; + + bitmap_git = prepare_bitmap_git(r); + if (!bitmap_git) + die(_("failed to load bitmap indexes")); + + /* + * Since this function needs to know the position of each individual + * bitmap, bypass the commit lookup table (if one exists) by forcing + * the bitmap to eagerly load its entries. + */ + if (bitmap_git->table_lookup) { + if (load_bitmap_entries_v1(bitmap_git) < 0) + die(_("failed to load bitmap indexes")); + } + + kh_foreach (bitmap_git->bitmaps, oid, stored, { + commit_idx_pos_map_pos = stored->map_pos; + xor_offset_map_pos = stored->map_pos + sizeof(uint32_t); + flag_map_pos = xor_offset_map_pos + sizeof(uint8_t); + ewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t); + + printf_ln("%s %"PRIuMAX" %"PRIuMAX" %"PRIuMAX" %"PRIuMAX, + oid_to_hex(&oid), + (uintmax_t)commit_idx_pos_map_pos, + (uintmax_t)xor_offset_map_pos, + (uintmax_t)flag_map_pos, + (uintmax_t)ewah_bitmap_map_pos); + }) + ; + + free_bitmap_index(bitmap_git); + + return 0; +} + int test_bitmap_hashes(struct repository *r) { struct bitmap_index *bitmap_git = prepare_bitmap_git(r); diff --git a/pack-bitmap.h b/pack-bitmap.h index 382d39499a..1bd7a791e2 100644 --- a/pack-bitmap.h +++ b/pack-bitmap.h @@ -81,6 +81,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *, show_reachable_fn show_reachable); void test_bitmap_walk(struct rev_info *revs); int test_bitmap_commits(struct repository *r); +int test_bitmap_commits_with_offset(struct repository *r); int test_bitmap_hashes(struct repository *r); int test_bitmap_pseudo_merges(struct repository *r); int test_bitmap_pseudo_merge_commits(struct repository *r, uint32_t n); diff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c index 3f23f21072..16a01669e4 100644 --- a/t/helper/test-bitmap.c +++ b/t/helper/test-bitmap.c @@ -10,6 +10,11 @@ static int bitmap_list_commits(void) return test_bitmap_commits(the_repository); } +static int bitmap_list_commits_with_offset(void) +{ + return test_bitmap_commits_with_offset(the_repository); +} + static int bitmap_dump_hashes(void) { return test_bitmap_hashes(the_repository); @@ -36,6 +41,8 @@ int cmd__bitmap(int argc, const char **argv) if (argc == 2 && !strcmp(argv[1], "list-commits")) return bitmap_list_commits(); + if (argc == 2 && !strcmp(argv[1], "list-commits-with-offset")) + return bitmap_list_commits_with_offset(); if (argc == 2 && !strcmp(argv[1], "dump-hashes")) return bitmap_dump_hashes(); if (argc == 2 && !strcmp(argv[1], "dump-pseudo-merges")) @@ -46,6 +53,7 @@ int cmd__bitmap(int argc, const char **argv) return bitmap_dump_pseudo_merge_objects(atoi(argv[2])); usage("\ttest-tool bitmap list-commits\n" + "\ttest-tool bitmap list-commits-with-offset\n" "\ttest-tool bitmap dump-hashes\n" "\ttest-tool bitmap dump-pseudo-merges\n" "\ttest-tool bitmap dump-pseudo-merge-commits \n" diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh index a62b463eaf..df05d74191 100755 --- a/t/t5310-pack-bitmaps.sh +++ b/t/t5310-pack-bitmaps.sh @@ -486,6 +486,36 @@ test_bitmap_cases () { grep "ignoring extra bitmap" trace2.txt ) ' + + test_expect_success 'load corrupt bitmap' ' + rm -fr repo && + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + git config pack.writeBitmapLookupTable '"$writeLookupTable"' && + + test_commit base && + + git repack -adb && + bitmap="$(ls .git/objects/pack/pack-*.bitmap)" && + chmod +w $bitmap && + + test-tool bitmap list-commits-with-offset >offsets && + xor_off=$(head -n1 offsets | awk "{print \$3}") && + printf '\161' | + dd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off && + + git rev-list --objects --no-object-names HEAD >expect.raw && + git rev-list --objects --use-bitmap-index --no-object-names HEAD \ + >actual.raw && + + sort expect.raw >expect && + sort actual.raw >actual && + + test_cmp expect actual + ) + ' } test_bitmap_cases