From: Omar Polo <op@omarpolo.com>
Subject: plug some leak around imsg_init() error paths
To: gameoftrees@openbsd.org
Date: Fri, 08 Nov 2024 13:23:24 +0100

spotted while converting to the new imsg APIs.  only compile-tested so
far.

diff -s /home/op/w/got
commit - 28f88a8012eef7b02f7ada4a4e8a89d1d4342f70
path + /home/op/w/got (staged changes)
blob - e8c7b30d40cff93877de27625b5c08b7104940eb
blob + a729ba6cc63a9ea335701d415c19ad7346b7e1d9
--- gotd/repo_read.c
+++ gotd/repo_read.c
@@ -264,15 +264,10 @@ list_refs(struct imsg *imsg)
 	size_t datalen;
 	struct gotd_imsg_reflist irefs;
 	struct imsgbuf ibuf;
-	int client_fd;
 	struct got_object_id *head_target_id = NULL;
 
 	TAILQ_INIT(&refs);
 
-	client_fd = imsg_get_fd(imsg);
-	if (client_fd == -1)
-		return got_error(GOT_ERR_PRIVSEP_NO_FD);
-
 	datalen = imsg->hdr.len - IMSG_HEADER_SIZE;
 	if (datalen != 0)
 		return got_error(GOT_ERR_PRIVSEP_LEN);
@@ -283,9 +278,11 @@ list_refs(struct imsg *imsg)
 	}
 	repo_read.refs_listed = 1;
 
-	client->fd = client_fd;
+	client->fd = imsg_get_fd(imsg);
+	if (client->fd == -1)
+		return got_error(GOT_ERR_PRIVSEP_NO_FD);
 
-	imsg_init(&ibuf, client_fd);
+	imsg_init(&ibuf, client->fd);
 
 	err = got_ref_list(&refs, repo_read.repo, "",
 	    got_ref_cmp_by_name, NULL);
blob - ee4eb5cff4be8f41919997710c1933d9d70f9d98
blob + aa0da23214b2af11cc85a86d5e53b854a44d49fb
--- gotd/repo_write.c
+++ gotd/repo_write.c
@@ -252,14 +252,9 @@ list_refs(struct imsg *imsg)
 	size_t datalen;
 	struct gotd_imsg_reflist irefs;
 	struct imsgbuf ibuf;
-	int client_fd;
 
 	TAILQ_INIT(&refs);
 
-	client_fd = imsg_get_fd(imsg);
-	if (client_fd == -1)
-		return got_error(GOT_ERR_PRIVSEP_NO_FD);
-
 	datalen = imsg->hdr.len - IMSG_HEADER_SIZE;
 	if (datalen != 0)
 		return got_error(GOT_ERR_PRIVSEP_LEN);
@@ -270,13 +265,16 @@ list_refs(struct imsg *imsg)
 	}
 	repo_write.refs_listed = 1;
 
-	client->fd = client_fd;
+	client->fd = imsg_get_fd(imsg);
+	if (client->fd == -1)
+		return got_error(GOT_ERR_PRIVSEP_NO_FD);
+
 	client->nref_updates = 0;
 	client->nref_del = 0;
 	client->nref_new = 0;
 	client->nref_move = 0;
 
-	imsg_init(&ibuf, client_fd);
+	imsg_init(&ibuf, client->fd);
 
 	err = got_ref_list(&refs, repo_write.repo, "",
 	    got_ref_cmp_by_name, NULL);
@@ -1242,7 +1240,7 @@ recv_packfile(int *have_packfile, struct imsg *imsg)
 
 	err = got_delta_cache_alloc(&pack->delta_cache);
 	if (err)
-		return err;
+		goto done;
 
 	for (i = 0; i < nitems(repo_tempfiles); i++) {
 		struct repo_tempfile *t = &repo_tempfiles[i];
blob - e7520800792bf32be7c1cc4480dd40aeedfaed96
blob + 5108cc4c18f29e0fe295fff9013b0d434cfb971d
--- lib/object_open_privsep.c
+++ lib/object_open_privsep.c
@@ -360,6 +360,8 @@ start_child(struct got_repository *repo, int type)
 	pid = fork();
 	if (pid == -1) {
 		err = got_error_from_errno("fork");
+		close(imsg_fds[0]);
+		close(imsg_fds[1]);
 		free(ibuf);
 		return err;
 	}
@@ -370,6 +372,7 @@ start_child(struct got_repository *repo, int type)
 
 	if (close(imsg_fds[1]) == -1) {
 		err = got_error_from_errno("close");
+		close(imsg_fds[0]);
 		free(ibuf);
 		return err;
 	}
blob - 8dc9adc210203ebedb74801ccf880e731d62df8b
blob + 392472bce98fd2f6da0c0089c8185d3e2fb76257
--- lib/pack.c
+++ lib/pack.c
@@ -765,6 +765,8 @@ got_pack_start_privsep_child(struct got_pack *pack, st
 	pid = fork();
 	if (pid == -1) {
 		err = got_error_from_errno("fork");
+		close(imsg_fds[0]);
+		close(imsg_fds[1]);
 		goto done;
 	} else if (pid == 0) {
 		set_max_datasize();
@@ -773,8 +775,11 @@ got_pack_start_privsep_child(struct got_pack *pack, st
 		/* not reached */
 	}
 
-	if (close(imsg_fds[1]) == -1)
-		return got_error_from_errno("close");
+	if (close(imsg_fds[1]) == -1) {
+		err = got_error_from_errno("close");
+		close(imsg_fds[0]);
+		goto done;
+	}
 	pack->privsep_child->imsg_fd = imsg_fds[0];
 	pack->privsep_child->pid = pid;
 	imsg_init(ibuf, imsg_fds[0]);
blob - 69ea82550f16667b29fd557fbb1fd94110971355
blob + 572f79cfc16c871370788135675712401d386ddf
--- lib/send.c
+++ lib/send.c
@@ -340,7 +340,7 @@ got_send_pack(const char *remote_name, struct got_path
     struct got_repository *repo, got_send_progress_cb progress_cb,
     void *progress_arg, got_cancel_cb cancel_cb, void *cancel_arg)
 {
-	int imsg_sendfds[2];
+	int imsg_sendfds[2] = { -1, -1 };
 	int npackfd = -1, nsendfd = -1;
 	int sendstatus, done = 0;
 	const struct got_error *err;
@@ -454,7 +454,7 @@ got_send_pack(const char *remote_name, struct got_path
 	if (sendpid == -1) {
 		err = got_error_from_errno("fork");
 		goto done;
-	} else if (sendpid == 0){
+	} else if (sendpid == 0) {
 		got_privsep_exec_child(imsg_sendfds,
 		    GOT_PATH_PROG_SEND_PACK, got_repo_get_path(repo));
 	}
@@ -463,6 +463,7 @@ got_send_pack(const char *remote_name, struct got_path
 		err = got_error_from_errno("close");
 		goto done;
 	}
+	imsg_sendfds[1] = -1;
 	imsg_init(&sendibuf, imsg_sendfds[0]);
 	nsendfd = dup(sendfd);
 	if (nsendfd == -1) {
@@ -691,6 +692,10 @@ done:
 		if (waitpid(sendpid, &sendstatus, 0) == -1 && err == NULL)
 			err = got_error_from_errno("waitpid");
 	}
+	if (imsg_sendfds[0] != -1 && close(imsg_sendfds[0]) == -1 && err == NULL)
+		err = got_error_from_errno("close");
+	if (imsg_sendfds[1] != -1 && close(imsg_sendfds[1]) == -1 && err == NULL)
+		err = got_error_from_errno("close");
 	if (packfd != -1 && close(packfd) == -1 && err == NULL)
 		err = got_error_from_errno("close");
 	if (delta_cache && fclose(delta_cache) == EOF && err == NULL)