From: Omar Polo <op@omarpolo.com>
Subject: Re: Got status memory leak
To: Mark Jamsek <mark@jamsek.com>
Cc: Kyle Ackerman <kackerman0102@gmail.com>, gameoftrees@openbsd.org
Date: Sun, 03 Dec 2023 12:24:12 +0100

On 2023/12/03 14:44:57 +1100, Mark Jamsek <mark@jamsek.com> wrote:
> Kyle Ackerman <kackerman0102@gmail.com> wrote:
> > Here is the method to my madness for finding the memory leak.  I used
> > Otto's malloc on got to identify memory leaks.  Here is the kdump of
> > running `got status`.
> > 
> > ******** Start dump got *******
> > M=8 I=1 F=0 U=0 J=1 R=0 X=0 C=0 cache=64 G=0
> > Leak report:
> >                  f     sum      #    avg
> >      0x75ae9750e5f     384      8     48 addr2line -e got 0x5be5f
> >      0x75ae976dd27      32      1     32 addr2line -e got 0x78d27
> >      0x75ae9785cc8      32      2     16 addr2line -e got 0x90cc8
> >      0x75ae978acdc      96      1     96 addr2line -e got 0x95cdc
> >      0x75ae978b449      80      3     26 addr2line -e got 0x96449
> >      0x75ae97b8cda      48      1     48 addr2line -e got 0xc3cda
> >      0x75db48c0b3f     128      1    128 addr2line -e /usr/lib/libc.so.97.1 0xb4b3f
> >      0x75db48c3833   65536      1  65536 addr2line -e /usr/lib/libc.so.97.1 0xb7833
> >      0x75db48dbd71     144      8     18 addr2line -e /usr/lib/libc.so.97.1 0xcfd71
> >      0x75db48dd7d0      64      1     64 addr2line -e /usr/lib/libc.so.97.1 0xd17d0
> > 
> > ******** End dump got *******
> > 
> > The reason for the memory leak is that the worktree is never
> > closed/freed.  Here is the diff that closes the worktree.
> > 
> > diff /home/kyle/src/gotfork
> > commit - 1772f6cfbdaab8df400578320b52df07c48c5351
> > path + /home/kyle/src/gotfork
> > blob - 1f980e0d7946002349e8aa40fa0eedcdfbd92322
> > file + got/got.c
> > --- got/got.c
> > +++ got/got.c
> > @@ -6519,6 +6519,7 @@ done:
> >  
> >  	got_pathlist_free(&paths, GOT_PATHLIST_FREE_PATH);
> >  	free(cwd);
> > +	got_worktree_close(worktree);
> >  	return error;
> >  }
> >  
> > 
> 
> Thanks for identifying this leak, Kyle! (Otto's recent malloc changes
> have been really nice :)
> 
> We can't unconditionally close the worktree here, however, as this
> produces a segv in the error case when 'got status' is run outside a
> worktree as status's regress reveals.
> 
> We just need to guard it with a NULL check as per the below diff.
> 
> ok?

ok op@

While your here, can you fix cmd_checkout() and cmd_update() too?

Thanks,

> diff /home/mark/src/got
> commit - 227f81a40643e9c82bb019336421e17e914d015a
> path + /home/mark/src/got
> blob - 1f980e0d7946002349e8aa40fa0eedcdfbd92322
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -6408,7 +6408,7 @@ print:
>  static const struct got_error *
>  cmd_status(int argc, char *argv[])
>  {
> -	const struct got_error *error = NULL;
> +	const struct got_error *close_err, *error = NULL;
>  	struct got_repository *repo = NULL;
>  	struct got_worktree *worktree = NULL;
>  	struct got_status_arg st;
> @@ -6512,10 +6512,15 @@ done:
>  			error = pack_err;
>  	}
>  	if (repo) {
> -		const struct got_error *close_err = got_repo_close(repo);
> +		close_err = got_repo_close(repo);
>  		if (error == NULL)
>  			error = close_err;
>  	}
> +	if (worktree != NULL) {
> +		close_err = got_worktree_close(worktree);
> +		if (error == NULL)
> +			error = close_err;
> +	}
>  
>  	got_pathlist_free(&paths, GOT_PATHLIST_FREE_PATH);
>  	free(cwd);