[PATCH v5 0/3] pack-bitmap: fix memory leak if load_bitmap failed

["Lidong Yan via GitGitGadget" <gitgitgadget@gmail.com>]

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@ttaylorr.com>
     +    Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
      
       ## 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