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 12:45:34 +0200

On 2022/09/02 11:42:49 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Sep 02, 2022 at 11:31:31AM +0200, Omar Polo 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)
> 
> Looks fine, ok stsp
> 
> It's quite hard to nagivate this jungle of loops, in the long term
> we should consider splitting the function up into subroutines, as
> this helps with leaks by scoping use of temp variables better.

+1

in fact, there was an issue with this diff

> > diff /home/op/w/got
> > commit - 06e5b2b9820d183e373bdfcd7238af841a255001
> > path + /home/op/w/got
> > blob - ac3b54fcdf929d17f3aea822d4162937114cf8a7
> > file + gotwebd/got_operations.c
> > --- gotwebd/got_operations.c
> > +++ gotwebd/got_operations.c
> > @@ -644,24 +644,24 @@ got_get_repo_tags(struct request *c, int limit)
> >  			goto err;
> >  		}
> >  
> > +		free(id);
> > +		id = NULL;
> > +
> > +		free(id_str);
> > +		id_str = NULL;
> > +
> >  		error = got_ref_resolve(&id, repo, re->ref);
> >  		if (error)
> >  			goto done;
> >  
> >  		error = got_object_open_as_tag(&tag, repo, id);
> >  		if (error) {
> > -			if (error->code != GOT_ERR_OBJ_TYPE) {
> > -				free(id);
> > -				id = NULL;
> > +			if (error->code != GOT_ERR_OBJ_TYPE)
> >  				goto done;
> > -			}
> >  			/* "lightweight" tag */
> >  			error = got_object_open_as_commit(&commit, repo, id);
> > -			if (error) {
> > -				free(id);
> > -				id = NULL;
> > +			if (error)
> >  				goto done;
> > -			}
> >  			new_repo_tag->tagger =
> >  			    strdup(got_object_commit_get_committer(commit));
> >  			if (new_repo_tag->tagger == NULL) {
> > @@ -673,11 +673,7 @@ got_get_repo_tags(struct request *c, int limit)
> >  			error = got_object_id_str(&id_str, id);
> >  			if (error)
> >  				goto err;
> > -			free(id);
> > -			id = NULL;
> >  		} else {
> > -			free(id);
> > -			id = NULL;
> >  			new_repo_tag->tagger =
> >  			    strdup(got_object_tag_get_tagger(tag));
> >  			if (new_repo_tag->tagger == NULL) {
> > @@ -690,6 +686,9 @@ got_get_repo_tags(struct request *c, int limit)
> >  			    got_object_tag_get_object_id(tag));
> >  			if (error)
> >  				goto err;
> > +
> > +			got_object_tag_close(tag);
> > +			tag = NULL;

some code later needs `tag' so we can't free it here.


this fixes it:

diff 5a57034b6f08eec784b8fe3c0b0b734eb2d4605a bc95141ca7ce90e4b19a251b36c87601c150bb3f
commit - 5a57034b6f08eec784b8fe3c0b0b734eb2d4605a
commit + bc95141ca7ce90e4b19a251b36c87601c150bb3f
blob - 04cc799c9db1c0359740a967662340d9eb76f2cd
blob + 4beb308778696b4c4299304065d186d28b072d3e
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -654,6 +654,8 @@ got_get_repo_tags(struct request *c, int limit)
 		if (error)
 			goto done;
 
+		if (tag)
+			got_object_tag_close(tag);
 		error = got_object_open_as_tag(&tag, repo, id);
 		if (error) {
 			if (error->code != GOT_ERR_OBJ_TYPE)
@@ -686,9 +688,6 @@ got_get_repo_tags(struct request *c, int limit)
 			    got_object_tag_get_object_id(tag));
 			if (error)
 				goto err;
-
-			got_object_tag_close(tag);
-			tag = NULL;
 		}
 
 		new_repo_tag->commit_id = strdup(id_str);