In pack-bitmap.c:load_bitmap_entries_v1, the function read_bitmap_1 allocates a bitmap and reads index data into it. However, if any of the validation checks following the allocation fail, the allocated bitmap is not freed, resulting in a memory leak. To avoid this, the validation checks should be performed before the bitmap is allocated. Lidong Yan (1): pack-bitmap: add load corrupt bitmap test Taylor Blau (1): pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed pack-bitmap.c | 94 +++++++++++++++++++++++++++++++---------- pack-bitmap.h | 1 + t/helper/test-bitmap.c | 8 ++++ t/t5310-pack-bitmaps.sh | 27 ++++++++++++ 4 files changed, 107 insertions(+), 23 deletions(-) base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1962%2Fbrandb97%2Ffix-pack-bitmap-leak-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1962/brandb97/fix-pack-bitmap-leak-v3 Pull-Request: https://github.com/git/git/pull/1962 Range-diff vs v2: 1: 130c3dc5dcd < -: ----------- pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed 2: b515c278a8f = 1: cf87aad7c99 pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed 3: 5be22d563af ! 2: f5371d7daa9 pack-bitmap: add loading corrupt bitmap_index test @@ Metadata Author: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx> ## Commit message ## - pack-bitmap: add loading corrupt bitmap_index test + pack-bitmap: add load corrupt bitmap test - This patch add "load corrupt bitmap" test case in t5310-pack-bitmaps.sh. + This patch add test_bitmap_list_commits_offset() in patch-bitmap.c, + a new test helper command `test-tool bitmap list-commits-offset`, + and a `load corrupt bitmap` test case in t5310. - This test case intentionally corrupt the "xor_offset" field of the first - entry. To find position of first entry in *.bitmap, we need to skip 4 - ewah_bitmaps before entries. And I add a function `skip_ewah_bitmap()` - to do this. + The `load corrupt bitmap` test case intentionally corrupt the + "xor_offset" field of the first entry. And the newly added helper + can help to find position of "xor_offset" in bitmap file. Signed-off-by: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx> - ## t/t5310-pack-bitmaps.sh ## -@@ t/t5310-pack-bitmaps.sh: has_any () { - grep -Ff "$1" "$2" + ## pack-bitmap.c ## +@@ pack-bitmap.c: struct stored_bitmap { + int flags; + }; + ++struct stored_bitmap_tag_pos { ++ struct stored_bitmap stored; ++ size_t map_pos; ++}; ++ + /* + * The active bitmap index for a repository. By design, repositories only have + * a single bitmap index available (the index for the biggest packfile in +@@ pack-bitmap.c: static int existing_bitmaps_hits_nr; + static int existing_bitmaps_misses_nr; + static int roots_with_bitmaps_nr; + static int roots_without_bitmaps_nr; ++static int tag_pos_on_bitmap; + + static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st) + { +@@ pack-bitmap.c: 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; ++ struct stored_bitmap_tag_pos *tagged; + khiter_t hash_pos; + int ret; + +- stored = xmalloc(sizeof(struct stored_bitmap)); ++ tagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) : ++ sizeof(struct stored_bitmap)); ++ stored = &tagged->stored; ++ if (tag_pos_on_bitmap) ++ tagged->map_pos = map_pos; + stored->root = root; + stored->xor = xor_with; + stored->flags = flags; +@@ pack-bitmap.c: 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); +@@ pack-bitmap.c: 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; +@@ pack-bitmap.c: 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; +@@ pack-bitmap.c: 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); +@@ pack-bitmap.c: 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--; + } + +@@ pack-bitmap.c: 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); +@@ pack-bitmap.c: 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); +@@ pack-bitmap.c: int test_bitmap_commits(struct repository *r) + return 0; } -+skip_ewah_bitmap() { -+ local bitmap="$1" && -+ local offset="$2" && -+ local size= && ++int test_bitmap_commits_offset(struct repository *r) ++{ ++ struct object_id oid; ++ struct stored_bitmap_tag_pos *tagged; ++ struct bitmap_index *bitmap_git; ++ size_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos, ++ ewah_bitmap_map_pos; ++ ++ tag_pos_on_bitmap = 1; ++ bitmap_git = prepare_bitmap_git(r); ++ if (!bitmap_git) ++ die(_("failed to load bitmap indexes")); + -+ offset=$(($offset + 4)) && -+ size=0x$(od -An -v -t x1 -j $offset -N 4 $bitmap | tr -d ' \n') && -+ size=$(($size * 8)) && -+ offset=$(($offset + 4 + $size + 4)) && -+ echo $offset ++ /* ++ * As this function is only used to print bitmap selected ++ * commits, we don't have to read the commit table. ++ */ ++ 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, tagged, { ++ commit_idx_pos_map_pos = tagged->map_pos; ++ xor_offset_map_pos = tagged->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); + + ## pack-bitmap.h ## +@@ pack-bitmap.h: 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_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); + + ## t/helper/test-bitmap.c ## +@@ t/helper/test-bitmap.c: static int bitmap_list_commits(void) + return test_bitmap_commits(the_repository); + } + ++static int bitmap_list_commits_offset(void) ++{ ++ return test_bitmap_commits_offset(the_repository); +} + - # Since name-hash values are stored in the .bitmap files, add a test - # that checks that the name-hash calculations are stable across versions. - # Not exhaustive, but these hashing algorithms would be hard to change + static int bitmap_dump_hashes(void) + { + return test_bitmap_hashes(the_repository); +@@ t/helper/test-bitmap.c: 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-offset")) ++ return bitmap_list_commits_offset(); + if (argc == 2 && !strcmp(argv[1], "dump-hashes")) + return bitmap_dump_hashes(); + if (argc == 2 && !strcmp(argv[1], "dump-pseudo-merges")) +@@ t/helper/test-bitmap.c: 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-offset\n" + "\ttest-tool bitmap dump-hashes\n" + "\ttest-tool bitmap dump-pseudo-merges\n" + "\ttest-tool bitmap dump-pseudo-merge-commits <n>\n" + + ## t/t5310-pack-bitmaps.sh ## @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () { grep "ignoring extra bitmap" trace2.txt ) ' + -+ # A `.bitmap` file has the following structure: -+ # | Header | Commits | Trees | Blobs | Tags | Entries... | -+ # -+ # - The header is 32 bytes long when using SHA-1. -+ # - Commits, Trees, Blobs, and Tags are all stored as EWAH bitmaps. -+ # -+ # This test intentionally corrupts the `xor_offset` field of the first entry -+ # to verify robustness against malformed bitmap data. + test_expect_success 'load corrupt bitmap' ' + rm -fr repo && + git init repo && @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () { + + git repack -adb && + bitmap="$(ls .git/objects/pack/pack-*.bitmap)" && -+ chmod +w "$bitmap" && -+ -+ hdr_sz=$((12 + $(test_oid rawsz))) && -+ offset=$(skip_ewah_bitmap $bitmap $hdr_sz) && -+ offset=$(skip_ewah_bitmap $bitmap $offset) && -+ offset=$(skip_ewah_bitmap $bitmap $offset) && -+ offset=$(skip_ewah_bitmap $bitmap $offset) && -+ offset=$((offset + 4)) && ++ chmod +w $bitmap && + ++ read oid commit_off xor_off flag_off ewah_off <<-EOF && ++ $(test-tool bitmap list-commits-offset | head -n 1) ++ EOF + printf '\161' | -+ dd of=$bitmap count=1 bs=1 conv=notrunc seek=$offset && ++ dd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off && ++ + + git rev-list --count HEAD > expect && + git rev-list --use-bitmap-index --count HEAD > actual && -- gitgitgadget
