From: Stefan Sperling <stsp@stsp.name>
Subject: Re: fix ibuf memory leak
To: Tracey Emery <tracey@traceyemery.net>
Cc: gameoftrees@openbsd.org
Date: Sat, 19 Jun 2021 11:35:46 +0200

On Fri, Jun 18, 2021 at 04:21:22PM -0600, Tracey Emery wrote:
> Hello,
> 
> While testing gotwebd, I noticed memory growing quite rapidly. After an
> eternity if debugging, I finally tracked it down to an unfreed ibuf. The
> ibuf is opened in start_pack_privsep_child in lib/object.c. This frees
> it in got_pack_stop_privsep_chil in lib/pack.c, right before
> pack->privsep_child is freed.
> 
> When reloading the gotwebd index, this seems to stop the memory bloat.
> 
> ok?
> 
> -- 
> 
> Tracey Emery
> 
> diff 779e1159b25b2aa115e6b42f51003b7e2fa7c06b /home/tracey/src/got
> blob - 137217342b5f0eb662a63182136a6750ffa7e2af
> file + lib/pack.c
> --- lib/pack.c
> +++ lib/pack.c
> @@ -552,6 +552,7 @@ got_pack_stop_privsep_child(struct got_pack *pack)

Just a side note, it looks like got_pack_stop_privsep_child() could
be a static function? It is not called from anywhere else. If we decide
to change this, we'll probably want to write a separate patch.

>  	err = got_privsep_wait_for_child(pack->privsep_child->pid);
>  	if (close(pack->privsep_child->imsg_fd) == -1 && err == NULL)
>  		err = got_error_from_errno("close");

Shouldn't we also call imsg_clear() here?
That would match what got_repo_close() is doing.

imsg_clear frees memory allocated for the ibuf internally, and might
also close file descriptors. Are these still being leaked with your
patch?

> +	free(pack->privsep_child->ibuf);

Note that got_repo_close() checks whether privsep_child->imsg_fd == -1,
before it calls imsg_clear(ibuf). got_repo_close() code should probably
check whether ibuf is NULL such that got_pack_stop_privsep_child() could
set it to NULL here, after freeing it.

>  	free(pack->privsep_child);
>  	pack->privsep_child = NULL;
>  	return err;
> 
>