From: Omar Polo <op@omarpolo.com>
Subject: plug a few fd leaks in fdopen() error paths
To: gameoftrees@openbsd.org
Date: Tue, 30 Jan 2024 13:04:41 +0100

did a sweep of the error paths of fdopen/dir and spotted these.

The got-read-blob.c change, which I'd commit separately if ok, is only a
style change: I think that if (fp...) else if (close(fd)) construct
tricked my eyes at least twice, and while it is correct, I prefer if it
were more like the the other code we have around.

diff /home/op/w/got
commit - 7614e0f6e88b262ccbce1f107e54a8f5b80c0fef
path + /home/op/w/got
blob - 8dfa9ac368e4140a0ffd66fbb7761f6ef2ce5b40
file + gotd/repo_read.c
--- gotd/repo_read.c
+++ gotd/repo_read.c
@@ -664,6 +664,10 @@ send_packfile(struct imsg *imsg, struct gotd_imsgev *i
 	    PROC_REPO_READ, -1, &idone, sizeof(idone)) == -1)
 		err = got_error_from_errno("imsg compose PACKFILE_DONE");
 done:
+	if (client->delta_cache_fd != -1 &&
+	    close(client->delta_cache_fd) == -1 && err == NULL)
+		err = got_error_from_errno("close");
+	client->delta_cache_fd = -1;
 	if (delta_cache != NULL && fclose(delta_cache) == EOF && err == NULL)
 		err = got_error_from_errno("fclose");
 	imsg_clear(&ibuf);
blob - 62f7df1b95d5d56bd0b23c83df32e7d74f3d4b00
file + lib/fileindex.c
--- lib/fileindex.c
+++ lib/fileindex.c
@@ -1047,8 +1047,13 @@ walk_dir(struct got_pathlist_entry **next, struct got_
 		}
 
 		subdir = fdopendir(subdirfd);
-		if (subdir == NULL)
-			return got_error_from_errno2("fdopendir", path);
+		if (subdir == NULL) {
+			err = got_error_from_errno2("fdopendir", path);
+			close(subdirfd);
+			free(subpath);
+			free(subdirpath);
+			return err;
+		}
 		subdirfd = -1;
 		err = read_dirlist(&subdirlist, subdir, subdirpath);
 		if (err) {
blob - 67ac8fd64b02a0f981bf9c39b34dbd03485649a2
file + lib/opentemp.c
--- lib/opentemp.c
+++ lib/opentemp.c
@@ -87,6 +87,7 @@ got_opentemp_named(char **path, FILE **outfile, const 
 	*outfile = fdopen(fd, "w+");
 	if (*outfile == NULL) {
 		err = got_error_from_errno2("fdopen", *path);
+		close(fd);
 		free(*path);
 		*path = NULL;
 	}
blob - d33d541d933aef39d8511a0f97db12f13b32ac22
file + lib/repository.c
--- lib/repository.c
+++ lib/repository.c
@@ -1430,6 +1430,7 @@ got_repo_list_packidx(struct got_pathlist_head *packid
 	packdir = fdopendir(packdir_fd);
 	if (packdir == NULL) {
 		err = got_error_from_errno("fdopendir");
+		close(packdir_fd);
 		goto done;
 	}
 
@@ -2546,6 +2547,7 @@ got_repo_get_packfile_info(int *npackfiles, int *nobje
 	packdir = fdopendir(packdir_fd);
 	if (packdir == NULL) {
 		err = got_error_from_errno("fdopendir");
+		close(packdir_fd);
 		goto done;
 	}
 
blob - ba7276ad6779e4b6a924ef8284b01c4bbdb79216
file + lib/repository_admin.c
--- lib/repository_admin.c
+++ lib/repository_admin.c
@@ -1525,6 +1525,7 @@ got_repo_remove_lonely_packidx(struct got_repository *
 	packdir = fdopendir(packdir_fd);
 	if (packdir == NULL) {
 		err = got_error_from_errno("fdopendir");
+		close(packdir_fd);
 		goto done;
 	}
 
blob - bef01d3c3adfe6a70c48ee294f475d29377f5400
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
@@ -161,6 +161,7 @@ main(int argc, char *argv[])
 			err = got_error_from_errno("fdopen");
 			goto done;
 		}
+		fd = -1;
 
 		if (obj->size + obj->hdrlen <=
 		    GOT_PRIVSEP_INLINE_BLOB_DATA_MAX) {
@@ -186,17 +187,12 @@ main(int argc, char *argv[])
 		err = got_privsep_send_blob(&ibuf, size, obj->hdrlen, buf);
 done:
 		free(buf);
-		if (f) {
-			if (fclose(f) == EOF && err == NULL)
-				err = got_error_from_errno("fclose");
-		} else if (fd != -1) {
-			if (close(fd) == -1 && err == NULL)
-				err = got_error_from_errno("close");
-		}
-		if (outfd != -1) {
-			if (close(outfd) == -1 && err == NULL)
-				err = got_error_from_errno("close");
-		}
+		if (f && fclose(f) == EOF && err == NULL)
+			err = got_error_from_errno("fclose");
+		if (fd != -1 && close(fd) == -1 && err == NULL)
+			err = got_error_from_errno("close");
+		if (outfd != -1 && close(outfd) == -1 && err == NULL)
+			err = got_error_from_errno("close");
 
 		imsg_free(&imsg);
 		imsg_free(&imsg_outfd);