[PATCH] commit: free the right buffer in release_commit_memory

[PATCH] commit: free the right buffer in release_commit_memory

[Mike Hommey]

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(-)

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;
-- 
2.23.0

Re: [PATCH] commit: free the right buffer in release_commit_memory

[Junio C Hamano]

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;