gvfs-helper: ignore .idx files in prefetch multi-part responses

The GVFS cache server can return multiple pairs of (.pack, .idx)
files.  If both are provided, `gvfs-helper` assumes that they are
valid without any validation.  This might cause problems if the
.pack file is corrupt inside the data stream.  (This might happen
if the cache server sends extra unexpected STDERR data or if the
.pack file is corrupt on the cache server's disk.)

All of the .pack file verification logic is already contained
within `git index-pack`, so let's ignore the .idx from the data
stream and force compute it.

This defeats the purpose of some of the data cacheing on the cache
server, but safety is more important.

Signed-off-by: Jeff Hostetler <[email protected]>

Author: jeffhostetler

gvfs-helper.c

@@ -2124,7 +2124,6 @@ static void extract_packfile_from_multipack(
 {
 	struct ph ph;
 	struct tempfile *tempfile_pack = NULL;
-	struct tempfile *tempfile_idx = NULL;
 	int result = -1;
 	int b_no_idx_in_multipack;
 	struct object_id packfile_checksum;
@@ -2158,16 +2157,14 @@ static void extract_packfile_from_multipack(
 	b_no_idx_in_multipack = (ph.idx_len == maximum_unsigned_value_of_type(uint64_t) ||
 				 ph.idx_len == 0);
 
-	if (b_no_idx_in_multipack) {
-		my_create_tempfile(status, 0, "pack", &tempfile_pack, NULL, NULL);
-		if (!tempfile_pack)
-			goto done;
-	} else {
-		/* create a pair of tempfiles with the same basename */
-		my_create_tempfile(status, 0, "pack", &tempfile_pack, "idx", &tempfile_idx);
-		if (!tempfile_pack || !tempfile_idx)
-			goto done;
-	}
+	/*
+	 * We are going to harden `gvfs-helper` here and ignore the .idx file
+	 * if it is provided and always compute it locally so that we get the
+	 * added verification that `git index-pack` provides.
+	 */
+	my_create_tempfile(status, 0, "pack", &tempfile_pack, NULL, NULL);
+	if (!tempfile_pack)
+		goto done;
 
 	/*
 	 * Copy the current packfile from the open stream and capture
@@ -2194,38 +2191,31 @@ static void extract_packfile_from_multipack(
 
 	oid_to_hex_r(hex_checksum, &packfile_checksum);
 
-	if (b_no_idx_in_multipack) {
-		/*
-		 * The server did not send the corresponding .idx, so
-		 * we have to compute it ourselves.
-		 */
-		strbuf_addbuf(&temp_path_idx, &temp_path_pack);
-		strbuf_strip_suffix(&temp_path_idx, ".pack");
-		strbuf_addstr(&temp_path_idx, ".idx");
+	/*
+	 * Always compute the .idx file from the .pack file.
+	 */
+	strbuf_addbuf(&temp_path_idx, &temp_path_pack);
+	strbuf_strip_suffix(&temp_path_idx, ".pack");
+	strbuf_addstr(&temp_path_idx, ".idx");
 
-		my_run_index_pack(params, status,
-				  &temp_path_pack, &temp_path_idx,
-				  NULL);
-		if (status->ec != GH__ERROR_CODE__OK)
-			goto done;
+	my_run_index_pack(params, status,
+			  &temp_path_pack, &temp_path_idx,
+			  NULL);
+	if (status->ec != GH__ERROR_CODE__OK)
+		goto done;
 
-	} else {
+	if (!b_no_idx_in_multipack) {
 		/*
 		 * Server sent the .idx immediately after the .pack in the
-		 * data stream.  I'm tempted to verify it, but that defeats
-		 * the purpose of having it cached...
+		 * data stream.  Skip over it.
 		 */
-		if (my_copy_fd_len(fd_multipack, get_tempfile_fd(tempfile_idx),
-				   ph.idx_len) < 0) {
+		if (lseek(fd_multipack, ph.idx_len, SEEK_CUR) < 0) {
 			strbuf_addf(&status->error_message,
-				    "could not extract index[%d] in multipack",
+				    "could not skip index[%d] in multipack",
 				    k);
 			status->ec = GH__ERROR_CODE__COULD_NOT_INSTALL_PREFETCH;
 			goto done;
 		}
-
-		strbuf_addstr(&temp_path_idx, get_tempfile_path(tempfile_idx));
-		close_tempfile_gently(tempfile_idx);
 	}
 
 	strbuf_addf(&buf_timestamp, "%u", (unsigned int)ph.timestamp);
@@ -2240,7 +2230,6 @@ static void extract_packfile_from_multipack(
 
 done:
 	delete_tempfile(&tempfile_pack);
-	delete_tempfile(&tempfile_idx);
 	strbuf_release(&temp_path_pack);
 	strbuf_release(&temp_path_idx);
 	strbuf_release(&final_path_pack);

t/t5799-gvfs-helper.sh

@@ -1367,14 +1367,11 @@ test_expect_success 'prefetch corrupt pack without idx' '
 
 # Send corrupt PACK files with IDX files.  Since the cache server
 # sends both, `gvfs-helper` might fail to verify both of them.
-test_expect_failure 'prefetch corrupt pack with corrupt idx' '
+test_expect_success 'prefetch corrupt pack with corrupt idx' '
 	test_when_finished "per_test_cleanup" &&
 	start_gvfs_protocol_server_with_mayhem \
 		bad_prefetch_pack_sha &&
 
-	# TODO This is a false-positive since `gvfs-helper`
-	# TODO does not verify either of them when a pair
-	# TODO is sent.
 	test_must_fail \
 		git -C "$REPO_T1" gvfs-helper \
 			--cache-server=disable \