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

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.

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