From: Kyle Ackerman Subject: Re: got_privsep_recv_send_remote_ref imsg leak To: Theo Buehler , Omar Polo Cc: gameoftrees@openbsd.org Date: Tue, 24 Sep 2024 14:31:33 -0500 Theo Buehler 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?