From: Stefan Sperling <stsp@stsp.name>
Subject: fix tempfile handling in gotd's got_object_raw_open()
To: gameoftrees@openbsd.org
Date: Mon, 9 Jan 2023 21:19:42 +0100

There is a bug where we reuse a file stored in *outfd from a
previous call, resulting in a raw object backed by a file but
with tempfile_idx -1. This causes bad confusion during deltification.
Fix this by passing tempfd to read_raw() functions and only
setting *outfd in case it is actually required, and returning
tempfd to the repository tempfile pool otherwise.

With this, a final "input/output" error I see while cloning
ports.git from gotd disappears. I attributed this error to
having egdb attached to gotd at first, but that was wrong.

The problem seen during deltification was that an object
ended up being backed by the wrong file, namely a slightly
smaller file which contained the delta-base instead of the object.
gotd would then an attempt to read beyond the end of this file.
 
diff 60c140aebbd1069094e75d8358cb305bc305b7f8 639adcb774ebc1459df58b791ffcc037002de351
commit - 60c140aebbd1069094e75d8358cb305bc305b7f8
commit + 639adcb774ebc1459df58b791ffcc037002de351
blob - 312719b3bcc84abe056dab97015657c4a30442f6
blob + dc5a7253bb299918f457cf7b0a2116b13328d7b1
--- lib/object_open_io.c
+++ lib/object_open_io.c
@@ -250,7 +250,7 @@ got_object_raw_open(struct got_raw_object **obj, int *
 {
 	const struct got_error *err = NULL;
 	struct got_packidx *packidx = NULL;
-	int idx, tempfile_idx = -1;
+	int idx, tempfd, tempfile_idx;
 	uint8_t *outbuf = NULL;
 	off_t size = 0;
 	size_t hdrlen = 0;
@@ -262,21 +262,10 @@ got_object_raw_open(struct got_raw_object **obj, int *
 		return NULL;
 	}
 
-	if (*outfd == -1) {
-		int tempfd;
+	err = got_repo_temp_fds_get(&tempfd, &tempfile_idx, repo);
+	if (err)
+		return err;
 
-		err = got_repo_temp_fds_get(&tempfd, &tempfile_idx, repo);
-		if (err)
-			return err;
-
-		/* Duplicate tempfile descriptor to allow use of fdopen(3). */
-		*outfd = dup(tempfd);
-		if (*outfd == -1) {
-			got_repo_temp_fds_put(tempfile_idx, repo);
-			return got_error_from_errno("dup");
-		}
-	}
-
 	err = got_repo_search_packidx(&packidx, &idx, repo, id);
 	if (err == NULL) {
 		struct got_pack *pack = NULL;
@@ -294,7 +283,7 @@ got_object_raw_open(struct got_raw_object **obj, int *
 				goto done;
 		}
 		err = read_packed_object_raw(&outbuf, &size, &hdrlen,
-		    *outfd, pack, packidx, idx, id);
+		    tempfd, pack, packidx, idx, id);
 		if (err)
 			goto done;
 	} else if (err->code == GOT_ERR_NO_OBJ) {
@@ -304,13 +293,30 @@ got_object_raw_open(struct got_raw_object **obj, int *
 		if (err)
 			goto done;
 		err = got_object_read_raw(&outbuf, &size, &hdrlen,
-		    GOT_DELTA_RESULT_SIZE_CACHED_MAX, *outfd, id, fd);
+		    GOT_DELTA_RESULT_SIZE_CACHED_MAX, tempfd, id, fd);
 		if (close(fd) == -1 && err == NULL)
 			err = got_error_from_errno("close");
 		if (err)
 			goto done;
 	}
 
+	if (outbuf == NULL) {
+		if (*outfd != -1) {
+			err = got_error_msg(GOT_ERR_NOT_IMPL, "bad outfd");
+			goto done;
+		}
+
+		/*
+		 * Duplicate tempfile descriptor to allow use of
+		 * fdopen(3) inside got_object_raw_alloc().
+		 */
+		*outfd = dup(tempfd);
+		if (*outfd == -1) {
+			err = got_error_from_errno("dup");
+			goto done;
+		}
+	}
+		
 	err = got_object_raw_alloc(obj, outbuf, outfd,
 	    GOT_DELTA_RESULT_SIZE_CACHED_MAX, hdrlen, size);
 	if (err)
@@ -330,12 +336,24 @@ done:
 			*obj = NULL;
 		}
 		free(outbuf);
-		if (tempfile_idx != -1)
-			got_repo_temp_fds_put(tempfile_idx, repo);
+		got_repo_temp_fds_put(tempfile_idx, repo);
+		if (*outfd != -1) {
+			close(*outfd);
+			*outfd = -1;
+		}
 	} else {
-		(*obj)->tempfile_idx = tempfile_idx;
-		(*obj)->close_cb = put_raw_object_tempfile;
-		(*obj)->close_arg = repo;
+		if (((*obj)->f == NULL && (*obj)->fd == -1)) {
+			/* This raw object is not backed by a file. */
+			got_repo_temp_fds_put(tempfile_idx, repo);
+			if (*outfd != -1) {
+				close(*outfd);
+				*outfd = -1;
+			}
+		} else {
+			(*obj)->tempfile_idx = tempfile_idx;
+			(*obj)->close_cb = put_raw_object_tempfile;
+			(*obj)->close_arg = repo;
+		}
 	}
 	return err;
 }