From: Omar Polo <op@omarpolo.com>
Subject: Re: gotwebd: plug leaks in got_get_repo_tags
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Fri, 02 Sep 2022 11:49:21 +0200

On 2022/09/02 11:45:48 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Sep 02, 2022 at 11:37:28AM +0200, Omar Polo wrote:
> > On 2022/09/02 11:31:31 +0200, Omar Polo <op@omarpolo.com> wrote:
> > > Here's an improved version of the diff i sent yesterday; now without
> > > leaks due to `continue'.  (there is still some work to do here wrt
> > > `id' and `commit_msg0', will do as follow-up diff)
> > 
> > (i meant only commit_msg0)
> > 
> > here's the follow-up: always free commit_msg0 in case we fail between
> > when it's allocated and when it's released.
> > 
> > (it seems tricky, an to some extent it is, but outside of the summary
> > and tags we only visit the loop once.)
> 
> I don't see why it has to be freed inside the loop in first place?
> Can't we just leave it allocated and free only on exit?

ah, yes.  it's better like this, thanks!

diff b163541dd9b5a61e6be0e3956ab1ed2c591aace5 ac9ca4edfe025b2bb5184778b39330ac8fd330aa
commit - b163541dd9b5a61e6be0e3956ab1ed2c591aace5
commit + ac9ca4edfe025b2bb5184778b39330ac8fd330aa
blob - d8514a11ea110093a8ab4f21841ed7be3cde176e
blob + 04cc799c9db1c0359740a967662340d9eb76f2cd
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -757,10 +757,8 @@ got_get_repo_tags(struct request *c, int limit)
 			new_repo_tag->commit_msg = strdup(commit_msg);
 			if (new_repo_tag->commit_msg == NULL) {
 				error = got_error_from_errno("strdup");
-				free(commit_msg0);
 				goto err;
 			}
-			free(commit_msg0);
 		}
 
 		if (limit && --limit == 0) {
@@ -806,6 +804,7 @@ err:
 	if (tag)
 		got_object_tag_close(tag);
 	got_ref_list_free(&refs);
+	free(commit_msg0);
 	free(in_repo_path);
 	free(repo_path);
 	free(id);