pack-bitmap: fix memory leak if load_bitmap failed

Since it seems this patch has been inactive for some time, I have revised the comments according to Taylor's feedback and submitted a new version.

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: f0135a9047ca37d4d117dcf21f7e3e89fad85d00
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1962%2Fbrandb97%2Ffix-pack-bitmap-leak-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1962/brandb97/fix-pack-bitmap-leak-v6
Pull-Request: https://github.com/git/git/pull/1962

Range-diff vs v5:

1: 9ce2135df2a ! 1: 3d70e14e415 pack-bitmap: fix memory leak if load_bitmap() failed
@@ Commit message
});

, but won't since load_bitmap() already called kh_destroy_oid_map() and
- NULL'd the "bitmaps" pointer from within its "failed" label.
-
- So I think 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.
+ 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.
2: a75d0a3cc7f ! 2: 6a082930ea3 pack-bitmap: reword comments in test_bitmap_commits()
@@ Metadata
## Commit message ##
pack-bitmap: reword comments in test_bitmap_commits()

- In pack-bitmap.c:test_bitmap_commits(), it comments
-
- /*
- * As this function is only used to print bitmap selected
- * commits, we don't have to read the commit table.
- */
-
- This 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
+ 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.

@@ pack-bitmap.c: int test_bitmap_commits(struct repository *r)
/*
- * 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 bitmap selected
++ * 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.
*/
3: 05140e2171d ! 3: c1b5d030133 pack-bitmap: add load corrupt bitmap test
@@ Metadata
## Commit message ##
pack-bitmap: add load corrupt bitmap test

- 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.
-
- 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.
+ 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.

gitgitgadget