From: Theo Buehler <tb@theobuehler.org>
Subject: Re: got_privsep_recv_send_remote_ref imsg leak
To: Omar Polo <op@omarpolo.com>
Cc: Kyle Ackerman <kackerman0102@gmail.com>, gameoftrees@openbsd.org
Date: Tue, 24 Sep 2024 10:05:43 +0200

On Tue, Sep 24, 2024 at 10:01:18AM +0200, Omar Polo wrote:
> On 24/09/24 09:50, Theo Buehler wrote:
> > On Tue, Sep 24, 2024 at 09:30:33AM +0200, Omar Polo wrote:
> >> 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.
> > break only breaks out of the switch, but your point about goto done stands.
> 
> ops, yeah, I meant the goto done, then got confused trying to show the issue in the context of the diff ^^
> 
> actually that break should be converted to a goto done as well, otherwise we might ignore unexpected messages.

Right. I think you can also replace 'done = 1;' with a goto done and
get rid of the done variable. Then the memset becomes unnecessary,
unless I'm missing something.

> 
> >>>  		}
> >>> +		imsg_free(&imsg);
> > I would
> > 		imsg_free(&imsg);
> > 		memset(&imsg, 0, sizeof(imsg));
> >
> > here and keep the imsg_free() in the done path.
> 
> If you and Claudio are fine with this, I'm all for it!
> 
> >> 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;
> >>>  }