This patch prevents pack-bitmap.c:load_bitmap() from nulling bitmap_git->bitmap when loading failed thus eliminates memory leak. This patch also add a test case in t5310 which use clang leak sanitizer to detect whether leak happens when loading failed. Lidong Yan (2): pack-bitmap: reword comments in test_bitmap_commits() pack-bitmap: add load corrupt bitmap test Taylor Blau (1): pack-bitmap: fix memory leak if load_bitmap() failed pack-bitmap.c | 88 ++++++++++++++++++++++++++++++----------- pack-bitmap.h | 1 + t/helper/test-bitmap.c | 8 ++++ t/t5310-pack-bitmaps.sh | 30 ++++++++++++++ 4 files changed, 103 insertions(+), 24 deletions(-) base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1962%2Fbrandb97%2Ffix-pack-bitmap-leak-v5 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1962/brandb97/fix-pack-bitmap-leak-v5 Pull-Request: https://github.com/git/git/pull/1962 Range-diff vs v4: 1: b6b3a83a224 ! 1: 9ce2135df2a pack-bitmap: fix memory leak if load_bitmap() failed @@ Commit message its caller will always call free_bitmap_index() in case of an error. Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx> + Signed-off-by: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx> ## pack-bitmap.c ## @@ pack-bitmap.c: static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git, -: ----------- > 2: a75d0a3cc7f pack-bitmap: reword comments in test_bitmap_commits() 2: 7876d9a9014 ! 3: 05140e2171d pack-bitmap: add load corrupt bitmap test @@ Commit message ## pack-bitmap.c ## @@ pack-bitmap.c: struct stored_bitmap { + struct object_id oid; + struct ewah_bitmap *root; + struct stored_bitmap *xor; ++ size_t map_pos; 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, @@ pack-bitmap.c: static struct stored_bitmap *store_bitmap(struct bitmap_index *in + 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 = xmalloc(sizeof(struct stored_bitmap)); ++ stored->map_pos = map_pos; stored->root = root; stored->xor = xor_with; stored->flags = flags; @@ pack-bitmap.c: int test_bitmap_commits(struct repository *r) return 0; } -+int test_bitmap_commits_offset(struct repository *r) ++int test_bitmap_commits_with_offset(struct repository *r) +{ + struct object_id oid; -+ struct stored_bitmap_tag_pos *tagged; ++ 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; + -+ tag_pos_on_bitmap = 1; + bitmap_git = prepare_bitmap_git(r); + if (!bitmap_git) + 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 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, tagged, { -+ commit_idx_pos_map_pos = tagged->map_pos; -+ xor_offset_map_pos = tagged->map_pos + sizeof(uint32_t); ++ 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); + @@ 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_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); @@ t/helper/test-bitmap.c: static int bitmap_list_commits(void) return test_bitmap_commits(the_repository); } -+static int bitmap_list_commits_offset(void) ++static int bitmap_list_commits_with_offset(void) +{ -+ return test_bitmap_commits_offset(the_repository); ++ return test_bitmap_commits_with_offset(the_repository); +} + static int bitmap_dump_hashes(void) @@ 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], "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")) @@ 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 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>\n" @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () { + bitmap="$(ls .git/objects/pack/pack-*.bitmap)" && + chmod +w $bitmap && + -+ read oid commit_off xor_off flag_off ewah_off <<-EOF && -+ $(test-tool bitmap list-commits-offset | head -n 1) -+ EOF ++ 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 && + -+ git rev-list --count HEAD > expect && -+ git rev-list --use-bitmap-index --count HEAD > actual && -+ test_cmp expect actual ++ test_cmp expect actual + ) + ' } -- gitgitgadget
