From: Omar Polo <op@omarpolo.com>
Subject: Re: memleak in commit_graph
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Mon, 05 Sep 2022 14:49:29 +0200

On 2022/09/05 13:19:33 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Mon, Sep 05, 2022 at 12:26:41PM +0200, Omar Polo wrote:
> > The idea now is that when calling got_commit_graph_iter_next the
> > returned pointer is safe until the next call to it.  Most callers were
> > already doing that, so this is not an invasive change.
> 
> Seems fine, for now.
> 
> > For a future improvement (post-release) would be fine to make the
> > callers provide the storage for the got_object_id that iter_next
> > returns?
> 
> Yes, that might be better indeed.
> 
> ok for the diff

I missed that got saves the id returned by iter_next, so something
like the following is needed too.

(i double-checked gotweb and it seems to be ok)

diff /home/op/w/got
commit - 11ca6b04e86de60f10769eaa34e40ecbd92b293e
path + /home/op/w/got
blob - d3fe18a13136b64b9e9ba153ae5a2deda0482b9c
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -2092,12 +2092,19 @@ alloc_commit_queue_entry(struct got_commit_object *com
     struct got_object_id *id)
 {
 	struct commit_queue_entry *entry;
+	struct got_object_id *dup;
 
 	entry = calloc(1, sizeof(*entry));
 	if (entry == NULL)
 		return NULL;
 
-	entry->id = id;
+	dup = got_object_id_dup(id);
+	if (dup == NULL) {
+		free(entry);
+		return NULL;
+	}
+
+	entry->id = dup;
 	entry->commit = commit;
 	return entry;
 }
@@ -2111,7 +2118,7 @@ pop_commit(struct commit_queue *commits)
 	TAILQ_REMOVE(&commits->head, entry, entry);
 	got_object_commit_close(entry->commit);
 	commits->ncommits--;
-	/* Don't free entry->id! It is owned by the commit graph. */
+	free(entry->id);
 	free(entry);
 }