{"thread":{"id":"51735","subject":"[PATCH] commit: free the right buffer in release_commit_memory","startedAt":"2019-08-26T02:01:43Z","lastAt":"2019-08-26T17:52:30Z","messageCount":2,"participants":["Mike Hommey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"381152","messageId":"20190826020137.4703-1-mh@glandium.org","threadId":"51735","inReplyTo":null,"subject":"[PATCH] commit: free the right buffer in release_commit_memory","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-08-26T02:01:37Z","receivedAt":"2019-08-26T02:01:43Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"The index field in the commit object is used to find the buffer\ncorresponding to that commit in the buffer_slab. Resetting it first\nmeans free_commit_buffer is not going to free the right buffer.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n commit.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex a98de16e3d..3fe5f8fa9c 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -364,8 +364,8 @@ struct object_id *get_commit_tree_oid(const struct commit *commit)\n void release_commit_memory(struct parsed_object_pool *pool, struct commit *c)\n {\n \tset_commit_tree(c, NULL);\n-\tc->index = 0;\n \tfree_commit_buffer(pool, c);\n+\tc->index = 0;\n \tfree_commit_list(c->parents);\n \n \tc->object.parsed = 0;\n-- \n2.23.0\n\n"},{"id":"381220","messageId":"xmqqk1azhl0p.fsf@gitster-ct.c.googlers.com","threadId":"51735","inReplyTo":"20190826020137.4703-1-mh@glandium.org","subject":"Re: [PATCH] commit: free the right buffer in release_commit_memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-26T17:52:22Z","receivedAt":"2019-08-26T17:52:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> The index field in the commit object is used to find the buffer\n> corresponding to that commit in the buffer_slab. Resetting it first\n> means free_commit_buffer is not going to free the right buffer.\n>\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n>  commit.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n\nWow, good find.  Ever since 14ba97f8 (\"alloc: allow arbitrary\nrepositories for alloc functions\", 2018-05-15) introduced the\nrelease function, it was doing the wrong thing.\n\nI think the real damage at most would have been just leaked memory,\nbecause (1) commit.c::free_commit_buffer() uses FREE_AND_NULL() to\nnull-out the pointer, so calling free for the 0-th slab entry over\nand over will not cause us to double-free, and (2) the only caller\nof this function is object.c::parsed_object_pool_clear() that wants\nto release resources from all objects.  There won't be a case like\n\"after releasing resource for 15th commit and we try to look at the\nbuffer for 0th commit, and the latter is gone by mistake, resulting\nus to dereference freed piece of memory\".\n\nThat would explain why we didn't see a failure report earlier.\n\nAgain, thanks for finding and fixing.  Will queue.\n\n>\n> diff --git a/commit.c b/commit.c\n> index a98de16e3d..3fe5f8fa9c 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -364,8 +364,8 @@ struct object_id *get_commit_tree_oid(const struct commit *commit)\n>  void release_commit_memory(struct parsed_object_pool *pool, struct commit *c)\n>  {\n>  \tset_commit_tree(c, NULL);\n> -\tc->index = 0;\n>  \tfree_commit_buffer(pool, c);\n> +\tc->index = 0;\n>  \tfree_commit_list(c->parents);\n>  \n>  \tc->object.parsed = 0;\n"}]}