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

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

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@smail.nju.edu.cn>
      

-- 
gitgitgadget