# [PATCH 01/11] Fix memory leak in traverse_commit_list 4 messages from 2007-11-09 to 2007-11-11. Participants: Shawn O. Pearce, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/10754 ## Shawn O. Pearce, 2007-11-09 11:06 Subject: [PATCH 01/11] Fix memory leak in traverse_commit_list Message-ID: <20071109110610.GA19368@spearce.org> URL: https://gitlist.dev/e/20071109110610.GA19368%40spearce.org ``` If we were listing objects too then the objects were buffered in an array only reachable from a stack allocated structure. When this function returns that array would be leaked as nobody would have a reference to it anymore. Historically this hasn't been a problem as the primary user of traverse_commit_list() (the noble git-rev-list) would terminate as soon as the function was finished, thus allowing the operating system to cleanup memory. However we have been leaking this data in git-pack-objects ever since that program learned how to run the revision listing internally, rather than relying on reading object names from git-rev-list. To better facilitate reuse of traverse_commit_list during other builtin tools (such as git-fetch) we shouldn't leak temporary memory like this and instead we need to clean up properly after ourselves. Signed-off-by: Shawn O. Pearce --- list-objects.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/list-objects.c b/list-objects.c index e5c88c2..713bef9 100644 --- a/list-objects.c +++ b/list-objects.c @@ -170,4 +170,11 @@ void traverse_commit_list(struct rev_info *revs, } for (i = 0; i < objects.nr; i++) show_object(&objects.objects[i]); + free(objects.objects); + if (revs->pending.nr) { + revs->pending.nr = 0; + revs->pending.alloc = 0; + revs->pending.objects = NULL; + free(revs->pending.objects); + } } -- 1.5.3.5.1622.g41d10 ``` ## Junio C Hamano, 2007-11-09 22:43 Subject: Re: [PATCH 01/11] Fix memory leak in traverse_commit_list Message-ID: <7vmytnqbu2.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmytnqbu2.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20071109110610.GA19368@spearce.org> ``` "Shawn O. Pearce" writes: > diff --git a/list-objects.c b/list-objects.c > index e5c88c2..713bef9 100644 > --- a/list-objects.c > +++ b/list-objects.c > @@ -170,4 +170,11 @@ void traverse_commit_list(struct rev_info *revs, > } > for (i = 0; i < objects.nr; i++) > show_object(&objects.objects[i]); > + free(objects.objects); > + if (revs->pending.nr) { > + revs->pending.nr = 0; > + revs->pending.alloc = 0; > + revs->pending.objects = NULL; > + free(revs->pending.objects); > + } > } It is locally verifiable that objects.objects are no longer needed after this point, but it made me a bit nervous about freeing of revs->pending.objects. I think the existing callers are all Ok, but somebody else should double check. ``` ## Johannes Schindelin, 2007-11-09 23:51 Subject: Re: [PATCH 01/11] Fix memory leak in traverse_commit_list Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711092350440.4362%40racer.site In-Reply-To: <7vmytnqbu2.fsf@gitster.siamese.dyndns.org> ``` Hi, On Fri, 9 Nov 2007, Junio C Hamano wrote: > "Shawn O. Pearce" writes: > > > diff --git a/list-objects.c b/list-objects.c > > index e5c88c2..713bef9 100644 > > --- a/list-objects.c > > +++ b/list-objects.c > > @@ -170,4 +170,11 @@ void traverse_commit_list(struct rev_info *revs, > > } > > for (i = 0; i < objects.nr; i++) > > show_object(&objects.objects[i]); > > + free(objects.objects); > > + if (revs->pending.nr) { > > + revs->pending.nr = 0; > > + revs->pending.alloc = 0; > > + revs->pending.objects = NULL; > > + free(revs->pending.objects); Umm. Isn't this the wrong way around? Should you not free() first, and then set to NULL? Ciao, Dscho ``` ## Shawn O. Pearce, 2007-11-11 05:17 Subject: Re: [PATCH 01/11] Fix memory leak in traverse_commit_list Message-ID: <20071111051742.GT14735@spearce.org> URL: https://gitlist.dev/e/20071111051742.GT14735%40spearce.org In-Reply-To: <7vmytnqbu2.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > "Shawn O. Pearce" writes: > > > diff --git a/list-objects.c b/list-objects.c > > index e5c88c2..713bef9 100644 > > --- a/list-objects.c > > +++ b/list-objects.c > > @@ -170,4 +170,11 @@ void traverse_commit_list(struct rev_info *revs, > > } > > for (i = 0; i < objects.nr; i++) > > show_object(&objects.objects[i]); > > + free(objects.objects); > > + if (revs->pending.nr) { > > + revs->pending.nr = 0; > > + revs->pending.alloc = 0; > > + revs->pending.objects = NULL; > > + free(revs->pending.objects); > > + } > > } > > It is locally verifiable that objects.objects are no longer > needed after this point, but it made me a bit nervous about > freeing of revs->pending.objects. > > I think the existing callers are all Ok, but somebody else > should double check. There are 5 calllers: * builtin-fetch.c: This one I added with my series. It doesn't care about the pending object list. * builtin-pack-objects.c: This doesn't care about the pending list after the call to traverse_commit_list. * builtin-rev-list.c (2): Two calls; the first one is for the bisect case where we print bisect stats and then return 0 and the second is the end of the program for the non-bisect case. Neither cares about the pending list. * upload-pack.c: This is called in the async thread spawned by upload-pack to feed pack-objects. The last thing the async thread does is run traverse_commit_list, at which point it exits. I actually have to wonder why we didn't just teach this trick to pack-objects so we could avoid the async complexity here in upload-pack. So yea, the cleanup here is safe, assuming I didn't make the extremely obvious leak of setting to NULL then calling free() (as Dscho pointed out). -- Shawn. ```