[v2,02/15] pack-bitmap: fix leak of haves/wants object lists
diff mbox series

Message ID 20200214182211.GB150965@coredump.intra.peff.net
State New
Headers show
Series
  • combining object filters and bitmaps
Related show

Commit Message

Jeff King Feb. 14, 2020, 6:22 p.m. UTC
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(+)

Comments

Taylor Blau Feb. 15, 2020, 12:15 a.m. UTC | #1
On Fri, Feb 14, 2020 at 01:22:11PM -0500, Jeff King wrote:
> 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.

>  /*
>   * 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.

>  	return bitmap_git;
>
>  cleanup:
>  	free_bitmap_index(bitmap_git);
> +	object_list_free(&wants);
> +	object_list_free(&haves);

Ditto.

>  	return NULL;
>  }
>
> --
> 2.25.0.796.gcc29325708

Thanks,
Taylor
Jeff King Feb. 15, 2020, 6:46 a.m. UTC | #2
On Fri, Feb 14, 2020 at 04:15:52PM -0800, Taylor Blau wrote:

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

Yep, linked lists are deceptive that way.

This _could_ be converted to use the generic list.h macros. Or better
still, we could probably ditch it entirely in favor of an object_array.
But I'd prefer to do that separately, if at all.

-Peff
Derrick Stolee Feb. 18, 2020, 5:58 p.m. UTC | #3
On 2/14/2020 1:22 PM, Jeff King wrote:
> 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.

It's surprising that we got away with this for so long. Is it
possible that this loop of freeing memory could provide significant
performance drawbacks? I'm assuming that the free() loop is much
faster than the malloc() loop, so the impact is negligible.

Thanks,
-Stolee
Jeff King Feb. 18, 2020, 8:02 p.m. UTC | #4
On Tue, Feb 18, 2020 at 12:58:10PM -0500, Derrick Stolee wrote:

> On 2/14/2020 1:22 PM, Jeff King wrote:
> > 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.
> 
> It's surprising that we got away with this for so long. Is it
> possible that this loop of freeing memory could provide significant
> performance drawbacks? I'm assuming that the free() loop is much
> faster than the malloc() loop, so the impact is negligible.

I don't think it's surprising. We're only talking about leaking hundreds
or perhaps thousands of structs. So maybe a few kilobytes at most. And
we'd typically only run one such bitmap traversal per program.

Meanwhile rev-list is generally storing that same set of objects in a
pending array, and I think we don't generally clean up after it (nor is
there even a convenient function to do so).

So I think it's sort of a drop in the bucket of Git's "eh, this pretty
much lasts about the whole program anyway" leaks. I don't think there's
much performance difference either way in freeing or not. It's mostly
just a cleanliness thing. (I do one day dream of being able to run the
test suite through a leakchecker, but we're still a ways off, I think).

-Peff

Patch
diff mbox series

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);
+	}
+}
+
 /*
  * 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);
+
 	return bitmap_git;
 
 cleanup:
 	free_bitmap_index(bitmap_git);
+	object_list_free(&wants);
+	object_list_free(&haves);
 	return NULL;
 }