From: Omar Polo <op@omarpolo.com>
Subject: Re: got_privsep_recv_send_remote_ref imsg leak
To: Kyle Ackerman <kackerman0102@gmail.com>
Cc: gameoftrees@openbsd.org
Date: Tue, 24 Sep 2024 09:30:33 +0200

On 24/09/24 04:41, Kyle Ackerman wrote:
> Here is the kdump showing the leak
>
> ******** Start dump got *******
> M=8 I=1 F=1 U=1 J=2 R=0 X=0 C=0xea4f16f8 cache=0 G=4096
> Leak report:
>                  f     sum      #    avg
>      0x9aa994ca530      64      1     64 addr2line -e /usr/lib/libc.so.100.3 0x89530 dupgl /usr/src/lib/libc/locale/setlocale.c:0 
>      0x9aa994b3a7a     112      7     16 addr2line -e /usr/lib/libc.so.100.3 0x72a7a _libc_strdup /usr/src/lib/libc/string/strdup.c:45 
>      0x9aac9948491     192      3     64 addr2line -e /usr/lib/libutil.so.18.0 0x10491 ibuf_open /usr/src/lib/libutil/imsg-buffer.c:49 
>      0x9aac99484a6     224      3     74 addr2line -e /usr/lib/libutil.so.18.0 0x104a6 ibuf_open /usr/src/lib/libutil/imsg-buffer.c:51 
>      0x9a878ec0596     800      5    160 addr2line -e /home/kyle/bin/got 0xb7596 alloc_meta /home/kyle/src/got/got/../lib/pack_create.c:86 
>      0x9a878ebdb3c      32      1     32 addr2line -e /home/kyle/bin/got 0xb4b3c got_deltify_init_mem /home/kyle/src/got/got/../lib/deltify.c:420 
>      0x9a878ebdb9a    7168      1   7168 addr2line -e /home/kyle/bin/got 0xb4b9a got_deltify_init_mem /home/kyle/src/got/got/../lib/deltify.c:426 
>      0x9a878ebdbf0     512      1    512 addr2line -e /home/kyle/bin/got 0xb4bf0 got_deltify_init_mem /home/kyle/src/got/got/../lib/deltify.c:433 
>
> ******** End dump got *******
>
>
> Here is the patch to patch it
The leak is indeed a missing imsg_free(), but your diff could still leak
> diff /home/kyle/src/got
> commit - 370f10653490c6d8f3e587869c96c100535edc9f
> path + /home/kyle/src/got
> blob - 116131bdf6703cf5fe030ee142ac6ffed00267b4
> file + lib/privsep.c
> --- lib/privsep.c
> +++ lib/privsep.c
> @@ -999,11 +999,12 @@ got_privsep_recv_send_remote_refs(struct got_pathlist_
>  			err = got_error(GOT_ERR_PRIVSEP_MSG);
>  			break;
here. If we enter any of the error paths, we `break' out of the loop and don't free the imsg.
>  		}
> +		imsg_free(&imsg);

It's not pretty, but an alternative to add `imsg_free()` to all break/goto done could be to add here

if (!done)

imsg_free(&imsg);

>  	}
>  done:
>  	free(id);
>  	free(refname);
and leave this here
> -	imsg_free(&imsg);
> +
>  	return err;
>  }