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. Signed-off-by: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx> -- gitgitgadget
