From: "Omar Polo" <op@omarpolo.com>
Subject: fix invalid imsg_free() in got_privsep_recv_printed_commits()
To: gameoftrees@openbsd.org
Date: Sat, 24 Feb 2024 18:46:59 +0100

This is probably the cause of the crash stsp observed by ctrl-c'ing
gotadmin pack -a (https://paste.apache.org/jpr92), while also fixing a
possible memory leak too.

if got_privsep_recv_imsg() fails before reading the imsg, the imsg
struct is still un-initialized and can't be passed to imsg_free().
Luckily, there are only two offenders in the tree which may call
imsg_free() in the error path of got_privsep_recv_imsg().

(technically this introduces a possible leak in libexec/got-read-patch
but it's just once per process so it should be acceptable.)

I think that got_privsep_recv_imsg() should free the imsg on error after
it has read something.

ok?

p.s. as a follow-up now that I noticed i think we should swap the order
of the checks in got_privsep_recv_imsg(), so to have the GOT_IMSG_ERROR
case first, then the length.

diff /home/op/w/got
commit - 7a86002db34d49472a7d75c1802ee99c2120ef3c
path + /home/op/w/got
blob - 79db43c49ad84782e167cf267f4958c7fa944561
file + lib/privsep.c
--- lib/privsep.c
+++ lib/privsep.c
@@ -141,12 +141,16 @@ got_privsep_recv_imsg(struct imsg *imsg, struct imsgbu
 			return got_error_from_errno("imsg_get");
 	}
 
-	if (imsg->hdr.len < IMSG_HEADER_SIZE + min_datalen)
+	if (imsg->hdr.len < IMSG_HEADER_SIZE + min_datalen) {
+		imsg_free(imsg);
 		return got_error(GOT_ERR_PRIVSEP_LEN);
+	}
 
 	if (imsg->hdr.type == GOT_IMSG_ERROR) {
 		size_t datalen = imsg->hdr.len - IMSG_HEADER_SIZE;
-		return recv_imsg_error(imsg, datalen);
+		err = recv_imsg_error(imsg, datalen);
+		imsg_free(imsg);
+		return err;
 	}
 
 	return NULL;
@@ -3510,10 +3514,8 @@ got_privsep_recv_painted_commits(struct got_object_id_
 
 	for (;;) {
 		err = got_privsep_recv_imsg(&imsg, ibuf, 0);
-		if (err){
-			imsg_free(&imsg);
+		if (err)
 			return err;
-		}
 
 		datalen = imsg.hdr.len - IMSG_HEADER_SIZE;
 		if (imsg.hdr.type == GOT_IMSG_COMMIT_PAINTING_DONE) {
blob - 508821d598050b15d1b623785750f1a0c1040d10
file + libexec/got-read-patch/got-read-patch.c
--- libexec/got-read-patch/got-read-patch.c
+++ libexec/got-read-patch/got-read-patch.c
@@ -682,8 +682,8 @@ main(int argc, char **argv)
 		goto done;
 	}
 	err = got_privsep_flush_imsg(&ibuf);
-done:
 	imsg_free(&imsg);
+done:
 	if (fd != -1 && close(fd) == -1 && err == NULL)
 		err = got_error_from_errno("close");
 	if (fp != NULL && fclose(fp) == EOF && err == NULL)