Stefan Sperling <stsp@stsp.name> writes:

> On Sun, Feb 13, 2022 at 10:49:05PM +0100, Omar Polo wrote:
>> Sure, but I fail to see how it's leaking if imsg_clear is called outside
>> of the loop.
>> 
>> If I'm reading correctly imsg{,-buf}.c all it does is to dequeue pending
>> ibufs and file descriptors and frees/closes them, so while I agree that
>> doing it inside the loop (possibly) frees these imsgbuf internal queues
>> more quickly, I don't see how it avoids leaking memory.
>> 
>> I'm clearly missing something obvious here, sorry, I just don't see it.
>
> This API is not very intuitive, and the imsg_init(3) man page does
> not clearly explain when imsg_clear(3) is intended to be used.
>
> Looking at the code it seems that only the writing side of the imsg
> channel is supposed to call imsg_clear() ever?
>
> The reader receives the data in a single buffer (ibuf->r.buf) which
> is pre-allocated as part of struct ibuf, so we don't need to free it
> since it lives on the stack.
>
> And any file descriptors that arrive in an ibuf are copied into struct
> imsg by imsg_get() and in such cases we close imsg->fd ourselves or
> copy its value elsewhere.
>
> So imsg_clear() at the receiving end looks like a no-op?

My reading is the same as yours.  Well, it kind of makes sense since you
can enqueue multiple messages but read one at a time.

I think that we could factor out all the imsg_clear in privsep.c, and
only when imsg_flush fails.  Sure, I'm being a bit pedantic here since
we always bail out pretty quickly if an error occurs so even if we leak
a bit of memory it's not the end of the world, but could save us future
confusions :)

(all the removed calls were only after we successfully receive
something, so there can't be any imsg pending in the sending queue.)

After this, the only other imsg_clear call in lib/ is repository.c:801
(offset after patch is applied): i don't think it's needed, but it since
it's a common cleanup step I thought it wouldn't hurt to keep it anyway.


(it's really two diffs, one for the missing close and the other for the
imsg_clear, err, cleanup, sending all together because it's easier.)

diff 32b5305fbed2d1a4c55b0eef6b93349b93ba7db0 /home/op/w/got-fetch-leak
blob - 6c0dbf4a0ba8148e5f95f882f51bda4b71ebc4f1
file + lib/fetch.c
--- lib/fetch.c
+++ lib/fetch.c
@@ -353,6 +353,10 @@ got_fetch_pack(struct got_object_id **pack_hash, struc
 			packfile_size = packfile_size_cur;
 		}
 	}
+	if (close(imsg_fetchfds[0]) == -1) {
+		err = got_error_from_errno("close");
+		goto done;
+	}
 	if (waitpid(fetchpid, &fetchstatus, 0) == -1) {
 		err = got_error_from_errno("waitpid");
 		goto done;
@@ -472,7 +476,6 @@ got_fetch_pack(struct got_object_id **pack_hash, struc
 			if (err)
 				break;
 		}
-		imsg_clear(&idxibuf);
 	}
 	if (close(imsg_idxfds[0]) == -1) {
 		err = got_error_from_errno("close");
blob - 664f170821f75b4356a1f141c1aebcf860ac1868
file + lib/gotconfig.c
--- lib/gotconfig.c
+++ lib/gotconfig.c
@@ -108,7 +108,6 @@ got_gotconfig_read(struct got_gotconfig **conf, const 
 	if (err)
 		goto done;
 
-	imsg_clear(ibuf);
 	err = got_privsep_send_stop(imsg_fds[0]);
 	child_err = got_privsep_wait_for_child(pid);
 	if (child_err && err == NULL)
blob - 8a01d22a8f4fe6686821dab1e44a139ea351a8a1
file + lib/pack.c
--- lib/pack.c
+++ lib/pack.c
@@ -577,7 +577,6 @@ pack_stop_privsep_child(struct got_pack *pack)
 	err = got_privsep_wait_for_child(pack->privsep_child->pid);
 	if (close(pack->privsep_child->imsg_fd) == -1 && err == NULL)
 		err = got_error_from_errno("close");
-	imsg_clear(pack->privsep_child->ibuf);
 	free(pack->privsep_child->ibuf);
 	free(pack->privsep_child);
 	pack->privsep_child = NULL;
blob - 9efd7ae9b91bb9c96b3d3a23e15d29ff1e58f89f
file + lib/privsep.c
--- lib/privsep.c
+++ lib/privsep.c
@@ -208,6 +208,7 @@ got_privsep_send_error(struct imsgbuf *ibuf, const str
 	if (ret == -1) {
 		fprintf(stderr, "%s: error %d \"%s\": imsg_flush: %s\n",
 		    getprogname(), err->code, err->msg, strerror(errno));
+		imsg_clear(ibuf);
 		return;
 	}
 }
@@ -221,8 +222,10 @@ flush_imsg(struct imsgbuf *ibuf)
 	if (err)
 		return err;
 
-	if (imsg_flush(ibuf) == -1)
+	if (imsg_flush(ibuf) == -1) {
+		imsg_clear(ibuf);
 		return got_error_from_errno("imsg_flush");
+	}
 
 	return NULL;
 }
@@ -245,7 +248,6 @@ got_privsep_send_stop(int fd)
 		return got_error_from_errno("imsg_compose STOP");
 
 	err = flush_imsg(&ibuf);
-	imsg_clear(&ibuf);
 	return err;
 }
 
blob - 40f5562db7ad3596203bf083a5084f34f1eb1b05
file + lib/repository.c
--- lib/repository.c
+++ lib/repository.c
@@ -577,7 +577,6 @@ parse_gitconfig_file(int *gitconfig_repository_format_
 			goto done;
 	}
 
-	imsg_clear(ibuf);
 	err = got_privsep_send_stop(imsg_fds[0]);
 	child_err = got_privsep_wait_for_child(pid);
 	if (child_err && err == NULL)
blob - f9be16f797d60019bfb883aac0e9b41c7beb846a
file + lib/repository_admin.c
--- lib/repository_admin.c
+++ lib/repository_admin.c
@@ -386,7 +386,6 @@ got_repo_index_pack(FILE *packfile, struct got_object_
 			if (err)
 				break;
 		}
-		imsg_clear(&idxibuf);
 	}
 	if (close(imsg_idxfds[0]) == -1) {
 		err = got_error_from_errno("close");