From 34759a3d0860c3953e942159cccfbc2901349164 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Thu, 13 Apr 2023 16:35:16 -0400 Subject: [PATCH] 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 --- gvfs-helper.c | 57 +++++++++++++++++------------------------- t/t5799-gvfs-helper.sh | 5 +--- 2 files changed, 24 insertions(+), 38 deletions(-) diff --git a/gvfs-helper.c b/gvfs-helper.c index e4c669a92b8434..df978099aec411 100644 --- a/gvfs-helper.c +++ b/gvfs-helper.c @@ -2121,7 +2121,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; @@ -2155,16 +2154,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 @@ -2191,38 +2188,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); @@ -2237,7 +2227,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); diff --git a/t/t5799-gvfs-helper.sh b/t/t5799-gvfs-helper.sh index 8ceb71e69b4205..73604aabb78f7f 100755 --- a/t/t5799-gvfs-helper.sh +++ b/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 \