Taylor Blau <me@xxxxxxxxxxxx> writes:
> After going through the "failed" label, load_bitmap() will return -1,
> and its caller (either prepare_bitmap_walk() or prepare_bitmap_git())
> will then call free_bitmap_index().
>
> That function would have done:
>
> struct stored_bitmap *sb;
> kh_foreach_value(b->bitmaps, sb {
> ewah_pool_free(sb->root);
> free(sb);
> });
>
> , but won't since load_bitmap() already called kh_destroy_oid_map() and
> NULL'd the "bitmaps" pointer from within its "failed" label.
Yikes.
> 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.
>
> I suspect the fix looks something like:
> ...
> --- 8< ---
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 5299f49d59..7f28532a69 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -631,41 +631,28 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,
> bitmap_git->ext_index.positions = kh_init_oid_pos();
>
> if (load_reverse_index(r, bitmap_git))
> - goto failed;
> + return -1;
(a lot of changes that simplifies the code snipped)
> -failed:
> - munmap(bitmap_git->map, bitmap_git->map_size);
> - bitmap_git->map = NULL;
> - bitmap_git->map_size = 0;
> -
> - kh_destroy_oid_map(bitmap_git->bitmaps);
> - bitmap_git->bitmaps = NULL;
> -
> - kh_destroy_oid_pos(bitmap_git->ext_index.positions);
> - bitmap_git->ext_index.positions = NULL;
> -
> - return -1;
> }
>
> static int open_pack_bitmap(struct repository *r,
> --- >8 ---
>
> , since all callers of load_bitmap() will themselves call
> free_bitmap_index(), so there is no need for us to open-code a portion
> of that function's implementation ourselves.
It is rare for a fix to be removing and simplifying this much code
;-)
