| Message ID | 20190826020137.4703-1-mh@glandium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit: free the right buffer in release_commit_memory | expand |
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;
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;
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(-)