On Sat, Jun 19, 2021 at 11:35:46AM +0200, Stefan Sperling wrote:
> 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.

Yes, you're right. It should call that.

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

I only reloaded the index a couple of times after the patch, and left to
drive home, so possibly! I'm not seeing anything with imsg_clear added.

> > +	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;
> > 
> > 

Ok for below then? The static change is your call.

-- 

Tracey Emery

diff 779e1159b25b2aa115e6b42f51003b7e2fa7c06b /home/tracey/src/got
blob - 137217342b5f0eb662a63182136a6750ffa7e2af
file + lib/pack.c
--- lib/pack.c
+++ lib/pack.c
@@ -552,6 +552,8 @@ got_pack_stop_privsep_child(struct got_pack *pack)
 	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");
+	imsg_clear(pack->privsep_child->ibuf);
+	free(pack->privsep_child->ibuf);
 	free(pack->privsep_child);
 	pack->privsep_child = NULL;
 	return err;