"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Kyle Ackerman <kack@kyleackerman.net>
Subject:
Plug memory leak in some libexecs
To:
"gameoftrees@openbsd.org" <gameoftrees@openbsd.org>
Date:
Sun, 04 May 2025 23:59:20 +0000

Download raw body.

Thread
There is currently an imsg memory leak in a couple of our libexecs.
This occur when we these particular libexecs get an imsg of type
GOT_IMSG_STOP. Which thet then just call break, not freeing the
imsg it just received.  Of the libexecs I noticed having this
behavior, we can jump to "done" to cleanup when we get
GOT_IMSG_STOP. This would also introduce the need for a variable
(which I called "finished") that we can set to determine whether 
we need to break out of the main loop.


diff /home/kyle/src/got
path + /home/kyle/src/got
commit - 4492e47bc914650ecd587fcc94010ae0373ab91b
blob - d23947e452d72ebf846600e7bf03c9ea2d1d2bd0
file + libexec/got-read-blob/got-read-blob.c
--- libexec/got-read-blob/got-read-blob.c
+++ libexec/got-read-blob/got-read-blob.c
@@ -78,7 +78,7 @@ main(int argc, char *argv[])
 	for (;;) {
 		struct imsg imsg, imsg_outfd;
 		FILE *f = NULL;
-		int fd = -1, outfd = -1;
+		int fd = -1, outfd = -1, finished = 0;
 		size_t size;
 		struct got_object *obj = NULL;
 		uint8_t *buf = NULL;
@@ -102,9 +102,12 @@ main(int argc, char *argv[])
 			break;
 		}
 
-		if (imsg.hdr.type == GOT_IMSG_STOP)
-			break;
+		if (imsg.hdr.type == GOT_IMSG_STOP) {
+			finished = 1;
+			goto done;
+		}
 
+
 		if (imsg.hdr.type != GOT_IMSG_BLOB_REQUEST) {
 			err = got_error(GOT_ERR_PRIVSEP_MSG);
 			goto done;
@@ -202,9 +205,12 @@ done:
 
 		imsg_free(&imsg);
 		imsg_free(&imsg_outfd);
-		if (obj)
+		if (obj) {
 			got_object_close(obj);
-		if (err)
+			obj = NULL;
+		}
+
+		if (err || finished)
 			break;
 	}
 
commit - 4492e47bc914650ecd587fcc94010ae0373ab91b
blob - a37151e9c26c717e0134c3112302bbaa24cbefb6
file + libexec/got-read-gitconfig/got-read-gitconfig.c
--- libexec/got-read-gitconfig/got-read-gitconfig.c
+++ libexec/got-read-gitconfig/got-read-gitconfig.c
@@ -365,7 +365,7 @@ main(int argc, char *argv[])
 
 	for (;;) {
 		struct imsg imsg;
-		int fd = -1;
+		int fd = -1, finished = 0;
 
 		memset(&imsg, 0, sizeof(imsg));
 
@@ -381,8 +381,10 @@ main(int argc, char *argv[])
 			break;
 		}
 
-		if (imsg.hdr.type == GOT_IMSG_STOP)
-			break;
+		if (imsg.hdr.type == GOT_IMSG_STOP) {
+			finished = 1;
+			goto done;
+		}
 
 		switch (imsg.hdr.type) {
 		case GOT_IMSG_GITCONFIG_PARSE_REQUEST:
@@ -427,13 +429,14 @@ main(int argc, char *argv[])
 			break;
 		}
 
+	done:
 		if (fd != -1) {
 			if (close(fd) == -1 && err == NULL)
 				err = got_error_from_errno("close");
 		}
 
 		imsg_free(&imsg);
-		if (err)
+		if (err || finished)
 			break;
 	}
 
commit - 4492e47bc914650ecd587fcc94010ae0373ab91b
blob - c7cf9727a6e3f4be0a7d3e1b689f108ee2a99d9c
file + libexec/got-read-gotconfig/got-read-gotconfig.c
--- libexec/got-read-gotconfig/got-read-gotconfig.c
+++ libexec/got-read-gotconfig/got-read-gotconfig.c
@@ -513,7 +513,7 @@ main(int argc, char *argv[])
 
 	for (;;) {
 		struct imsg imsg;
-		int fd = -1;
+		int fd = -1, finished = 0;
 
 		memset(&imsg, 0, sizeof(imsg));
 
@@ -529,8 +529,10 @@ main(int argc, char *argv[])
 			break;
 		}
 
-		if (imsg.hdr.type == GOT_IMSG_STOP)
-			break;
+		if (imsg.hdr.type == GOT_IMSG_STOP) {
+			finished = 1;
+			goto done;
+		}
 
 		switch (imsg.hdr.type) {
 		case GOT_IMSG_GOTCONFIG_PARSE_REQUEST:
@@ -598,14 +600,14 @@ main(int argc, char *argv[])
 			err = got_error(GOT_ERR_PRIVSEP_MSG);
 			break;
 		}
-
+	done:
 		if (fd != -1) {
 			if (close(fd) == -1 && err == NULL)
 				err = got_error_from_errno("close");
 		}
 
 		imsg_free(&imsg);
-		if (err)
+		if (err || finished)
 			break;
 	}
 
commit - 4492e47bc914650ecd587fcc94010ae0373ab91b
blob - cacaca8f4f78e8ba52a4e42ad12a6e42e9c6d950
file + libexec/got-read-object/got-read-object.c
--- libexec/got-read-object/got-read-object.c
+++ libexec/got-read-object/got-read-object.c
@@ -116,7 +116,7 @@ main(int argc, char *argv[])
 #endif
 
 	for (;;) {
-		int fd = -1, outfd = -1;
+		int fd = -1, outfd = -1, finished = 0;
 
 		if (sigint_received) {
 			err = got_error(GOT_ERR_CANCELLED);
@@ -130,8 +130,10 @@ main(int argc, char *argv[])
 			break;
 		}
 
-		if (imsg.hdr.type == GOT_IMSG_STOP)
-			break;
+		if (imsg.hdr.type == GOT_IMSG_STOP) {
+			finished = 1;
+			goto done;
+		}
 
 		if (imsg.hdr.type != GOT_IMSG_OBJECT_REQUEST &&
 		    imsg.hdr.type != GOT_IMSG_RAW_OBJECT_REQUEST) {
@@ -203,9 +205,11 @@ done:
 		if (fd != -1 && close(fd) == -1 && err == NULL)
 			err = got_error_from_errno("close");
 		imsg_free(&imsg);
-		if (obj)
+		if (obj) {
 			got_object_close(obj);
-		if (err)
+			obj = NULL;
+		}
+		if (err || finished)
 			break;
 	}
 

Thoughts/Comments/Suggestions/oks?