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 09:50:34 +0200

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.

> >  		}
> > +		imsg_free(&imsg);

I would
		imsg_free(&imsg);
		memset(&imsg, 0, sizeof(imsg));

here and keep the imsg_free() in the done path.

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