Theo Buehler <tb@theobuehler.org> writes:

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

Here is an updated patch:

diff /home/kyle/src/got
commit - 370f10653490c6d8f3e587869c96c100535edc9f
path + /home/kyle/src/got
blob - 116131bdf6703cf5fe030ee142ac6ffed00267b4
file + lib/privsep.c
--- lib/privsep.c
+++ lib/privsep.c
@@ -941,13 +941,12 @@ got_privsep_recv_send_remote_refs(struct got_pathlist_
 	const struct got_error *err = NULL;
 	struct imsg imsg;
 	size_t datalen;
-	int done = 0;
 	struct got_imsg_send_remote_ref iremote_ref;
 	struct got_object_id *id = NULL;
 	char *refname = NULL;
 	struct got_pathlist_entry *new;
 
-	while (!done) {
+	while (1) {
 		err = got_privsep_recv_imsg(&imsg, ibuf, 0);
 		if (err)
 			return err;
@@ -986,6 +985,7 @@ got_privsep_recv_send_remote_refs(struct got_pathlist_
 			}
 			id = NULL;
 			refname = NULL;
+			imsg_free(&imsg);
 			break;
 		case GOT_IMSG_SEND_PACK_REQUEST:
 			if (datalen != 0) {
@@ -993,11 +993,10 @@ got_privsep_recv_send_remote_refs(struct got_pathlist_
 				goto done;
 			}
 			/* got-send-pack is now waiting for a pack file. */
-			done = 1;
-			break;
+			goto done;
 		default:
 			err = got_error(GOT_ERR_PRIVSEP_MSG);
-			break;
+			goto done;
 		}
 	}
 done:


Theo:
I have removed `done` as recommended, do you still think we need memset
the memory to 0?

Omar:
I just put the imsg_free before the breaks, it is probably more readable
than what I originally did.


Thank you both for your feedback! Any other suggestions with this new diff?