From: Mark Jamsek <mark@jamsek.com>
Subject: Re: Got status memory leak
To: Kyle Ackerman <kackerman0102@gmail.com>
Cc: gameoftrees@openbsd.org
Date: Sun, 03 Dec 2023 14:44:57 +1100

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?


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


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68