Re: [PATCH v2 02/15] pack-bitmap: fix leak of haves/wants object lists
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Feb 15, 2020, 00:15 UTC
- Message-ID
- <20200215001552.GE11400@syl.local>
- In-Reply-To
- <20200214182211.GB150965@coredump.intra.peff.net>
On Fri, Feb 14, 2020 at 01:22:11PM -0500, Jeff King wrote:
Show 28 quoted lines
> When we do a bitmap-aware revision traversal, we create an object_list
> for each of the "haves" and "wants" tips. After creating the result
> bitmaps these are no longer needed or used, but we never free the list
> memory.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> object.c | 9 +++++++++
> object.h | 2 ++
> pack-bitmap.c | 5 +++++
> 3 files changed, 16 insertions(+)
>
> diff --git a/object.c b/object.c
> index 142ef69399..4d11949b38 100644
> --- a/object.c
> +++ b/object.c
> @@ -307,6 +307,15 @@ int object_list_contains(struct object_list *list, struct object *obj)
> return 0;
> }
>
> +void object_list_free(struct object_list **list)
> +{
> + while (*list) {
> + struct object_list *p = *list;
> + *list = p->next;
> + free(p);
> + }
> +}Hmm. I was going to write a comment saying more-or-less, "I think I'm nitpicking here, but I'm not crazy about this as a 'while' loop". But, when I wrote this as a 'for' loop instead, I wrote a use-after-free, and then when I rid the code of that, it wasn't any more readable than the version above.
Show 27 quoted lines
> /* > * A zero-length string to which object_array_entry::name can be > * initialized without requiring a malloc/free. > diff --git a/object.h b/object.h > index 25f5ab3d54..2dbabfca0a 100644 > --- a/object.h > +++ b/object.h > @@ -151,6 +151,8 @@ struct object_list *object_list_insert(struct object *item, > > int object_list_contains(struct object_list *list, struct object *obj); > > +void object_list_free(struct object_list **list); > + > /* Object array handling .. */ > void add_object_array(struct object *obj, const char *name, struct object_array *array); > void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path); > diff --git a/pack-bitmap.c b/pack-bitmap.c > index 9ca356ee29..6c06099dc7 100644 > --- a/pack-bitmap.c > +++ b/pack-bitmap.c > @@ -787,10 +787,15 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs) > bitmap_git->result = wants_bitmap; > bitmap_git->haves = haves_bitmap; > > + object_list_free(&wants); > + object_list_free(&haves); > +
Makes sense.
Show 6 quoted lines
> return bitmap_git; > > cleanup: > free_bitmap_index(bitmap_git); > + object_list_free(&wants); > + object_list_free(&haves);
Ditto.
Show 5 quoted lines
> return NULL; > } > > -- > 2.25.0.796.gcc29325708
Thanks, Taylor