commit: free the right buffer in release_commit_memory
diff mbox series

Message ID 20190826020137.4703-1-mh@glandium.org
State New
Headers show
Series
  • commit: free the right buffer in release_commit_memory
Related show

Commit Message

Mike Hommey Aug. 26, 2019, 2:01 a.m. UTC
The index field in the commit object is used to find the buffer
corresponding to that commit in the buffer_slab. Resetting it first
means free_commit_buffer is not going to free the right buffer.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano Aug. 26, 2019, 5:52 p.m. UTC | #1
Mike Hommey <mh@glandium.org> writes:

> The index field in the commit object is used to find the buffer
> corresponding to that commit in the buffer_slab. Resetting it first
> means free_commit_buffer is not going to free the right buffer.
>
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  commit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Wow, good find.  Ever since 14ba97f8 ("alloc: allow arbitrary
repositories for alloc functions", 2018-05-15) introduced the
release function, it was doing the wrong thing.

I think the real damage at most would have been just leaked memory,
because (1) commit.c::free_commit_buffer() uses FREE_AND_NULL() to
null-out the pointer, so calling free for the 0-th slab entry over
and over will not cause us to double-free, and (2) the only caller
of this function is object.c::parsed_object_pool_clear() that wants
to release resources from all objects.  There won't be a case like
"after releasing resource for 15th commit and we try to look at the
buffer for 0th commit, and the latter is gone by mistake, resulting
us to dereference freed piece of memory".

That would explain why we didn't see a failure report earlier.

Again, thanks for finding and fixing.  Will queue.

>
> diff --git a/commit.c b/commit.c
> index a98de16e3d..3fe5f8fa9c 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -364,8 +364,8 @@ struct object_id *get_commit_tree_oid(const struct commit *commit)
>  void release_commit_memory(struct parsed_object_pool *pool, struct commit *c)
>  {
>  	set_commit_tree(c, NULL);
> -	c->index = 0;
>  	free_commit_buffer(pool, c);
> +	c->index = 0;
>  	free_commit_list(c->parents);
>  
>  	c->object.parsed = 0;

Patch
diff mbox series

diff --git a/commit.c b/commit.c
index a98de16e3d..3fe5f8fa9c 100644
--- a/commit.c
+++ b/commit.c
@@ -364,8 +364,8 @@  struct object_id *get_commit_tree_oid(const struct commit *commit)
 void release_commit_memory(struct parsed_object_pool *pool, struct commit *c)
 {
 	set_commit_tree(c, NULL);
-	c->index = 0;
 	free_commit_buffer(pool, c);
+	c->index = 0;
 	free_commit_list(c->parents);
 
 	c->object.parsed = 0;