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

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?

> 
> diff /home/op/w/got
> commit - 3cadd3686d6e8ddb12b29f5a32197610438ac67d
> path + /home/op/w/got
> blob - d8514a11ea110093a8ab4f21841ed7be3cde176e
> file + gotwebd/got_operations.c
> --- gotwebd/got_operations.c
> +++ gotwebd/got_operations.c
> @@ -758,9 +758,11 @@ got_get_repo_tags(struct request *c, int limit)
>  			if (new_repo_tag->commit_msg == NULL) {
>  				error = got_error_from_errno("strdup");
>  				free(commit_msg0);
> +				commit_msg0 = NULL;
>  				goto err;
>  			}
>  			free(commit_msg0);
> +			commit_msg0 = NULL;
>  		}
>  
>  		if (limit && --limit == 0) {
> @@ -806,6 +808,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);
> 
>