{"thread":{"id":"10754","subject":"[PATCH 01/11] Fix memory leak in traverse_commit_list","startedAt":"2007-11-09T11:06:10Z","lastAt":"2007-11-11T05:17:42Z","messageCount":4,"participants":["Shawn O. Pearce","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"59023","messageId":"20071109110610.GA19368@spearce.org","threadId":"10754","inReplyTo":null,"subject":"[PATCH 01/11] Fix memory leak in traverse_commit_list","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-11-09T11:06:10Z","receivedAt":"2007-11-09T11:06:10Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If we were listing objects too then the objects were buffered in an\narray only reachable from a stack allocated structure. When this\nfunction returns that array would be leaked as nobody would have\na reference to it anymore.\n\nHistorically this hasn't been a problem as the primary user of\ntraverse_commit_list() (the noble git-rev-list) would terminate\nas soon as the function was finished, thus allowing the operating\nsystem to cleanup memory. However we have been leaking this data\nin git-pack-objects ever since that program learned how to run the\nrevision listing internally, rather than relying on reading object\nnames from git-rev-list.\n\nTo better facilitate reuse of traverse_commit_list during other\nbuiltin tools (such as git-fetch) we shouldn't leak temporary memory\nlike this and instead we need to clean up properly after ourselves.\n\nSigned-off-by: Shawn O. Pearce \n---\n list-objects.c | 7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex e5c88c2..713bef9 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -170,4 +170,11 @@ void traverse_commit_list(struct rev_info *revs,\n \t}\n \tfor (i = 0; i < objects.nr; i++)\n \t\tshow_object(&objects.objects[i]);\n+\tfree(objects.objects);\n+\tif (revs->pending.nr) {\n+\t\trevs->pending.nr = 0;\n+\t\trevs->pending.alloc = 0;\n+\t\trevs->pending.objects = NULL;\n+\t\tfree(revs->pending.objects);\n+\t}\n }\n-- \n1.5.3.5.1622.g41d10\n"},{"id":"59133","messageId":"7vmytnqbu2.fsf@gitster.siamese.dyndns.org","threadId":"10754","inReplyTo":"20071109110610.GA19368@spearce.org","subject":"Re: [PATCH 01/11] Fix memory leak in traverse_commit_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-09T22:43:33Z","receivedAt":"2007-11-09T22:43:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> diff --git a/list-objects.c b/list-objects.c\n> index e5c88c2..713bef9 100644\n> --- a/list-objects.c\n> +++ b/list-objects.c\n> @@ -170,4 +170,11 @@ void traverse_commit_list(struct rev_info *revs,\n> \t}\n> \tfor (i = 0; i < objects.nr; i++)\n> \t\tshow_object(&objects.objects[i]);\n> +\tfree(objects.objects);\n> +\tif (revs->pending.nr) {\n> +\t\trevs->pending.nr = 0;\n> +\t\trevs->pending.alloc = 0;\n> +\t\trevs->pending.objects = NULL;\n> +\t\tfree(revs->pending.objects);\n> +\t}\n> }\n\nIt is locally verifiable that objects.objects are no longer\nneeded after this point, but it made me a bit nervous about\nfreeing of revs->pending.objects.\n\nI think the existing callers are all Ok, but somebody else\nshould double check.\n"},{"id":"59148","messageId":"Pine.LNX.4.64.0711092350440.4362@racer.site","threadId":"10754","inReplyTo":"7vmytnqbu2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 01/11] Fix memory leak in traverse_commit_list","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-09T23:51:12Z","receivedAt":"2007-11-09T23:51:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 9 Nov 2007, Junio C Hamano wrote:\n\n> \"Shawn O. Pearce\" writes:\n> \n> > diff --git a/list-objects.c b/list-objects.c\n> > index e5c88c2..713bef9 100644\n> > --- a/list-objects.c\n> > +++ b/list-objects.c\n> > @@ -170,4 +170,11 @@ void traverse_commit_list(struct rev_info *revs,\n> > \t}\n> > \tfor (i = 0; i < objects.nr; i++)\n> > \t\tshow_object(&objects.objects[i]);\n> > +\tfree(objects.objects);\n> > +\tif (revs->pending.nr) {\n> > +\t\trevs->pending.nr = 0;\n> > +\t\trevs->pending.alloc = 0;\n> > +\t\trevs->pending.objects = NULL;\n> > +\t\tfree(revs->pending.objects);\n\nUmm. Isn't this the wrong way around? Should you not free() first, and \nthen set to NULL?\n\nCiao,\nDscho\n"},{"id":"59265","messageId":"20071111051742.GT14735@spearce.org","threadId":"10754","inReplyTo":"7vmytnqbu2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 01/11] Fix memory leak in traverse_commit_list","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-11-11T05:17:42Z","receivedAt":"2007-11-11T05:17:42Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > diff --git a/list-objects.c b/list-objects.c\n> > index e5c88c2..713bef9 100644\n> > --- a/list-objects.c\n> > +++ b/list-objects.c\n> > @@ -170,4 +170,11 @@ void traverse_commit_list(struct rev_info *revs,\n> > \t}\n> > \tfor (i = 0; i < objects.nr; i++)\n> > \t\tshow_object(&objects.objects[i]);\n> > +\tfree(objects.objects);\n> > +\tif (revs->pending.nr) {\n> > +\t\trevs->pending.nr = 0;\n> > +\t\trevs->pending.alloc = 0;\n> > +\t\trevs->pending.objects = NULL;\n> > +\t\tfree(revs->pending.objects);\n> > +\t}\n> > }\n> \n> It is locally verifiable that objects.objects are no longer\n> needed after this point, but it made me a bit nervous about\n> freeing of revs->pending.objects.\n> \n> I think the existing callers are all Ok, but somebody else\n> should double check.\n\nThere are 5 calllers:\n\n* builtin-fetch.c:\n\n This one I added with my series. It doesn't care about the\n pending object list.\n\n* builtin-pack-objects.c:\n\n This doesn't care about the pending list after the call to\n traverse_commit_list.\n\n* builtin-rev-list.c (2):\n\n Two calls; the first one is for the bisect case where we print\n bisect stats and then return 0 and the second is the end of\n the program for the non-bisect case. Neither cares about the\n pending list.\n\n* upload-pack.c:\n\n This is called in the async thread spawned by upload-pack to\n feed pack-objects. The last thing the async thread does is run\n traverse_commit_list, at which point it exits. I actually have\n to wonder why we didn't just teach this trick to pack-objects\n so we could avoid the async complexity here in upload-pack.\n\nSo yea, the cleanup here is safe, assuming I didn't make the\nextremely obvious leak of setting to NULL then calling free()\n(as Dscho pointed out).\n\n-- \nShawn.\n"}]}