From aa86eeb1d8833c61489f2bfcc44ee1489d56b954 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 9f569df0290c16..403b2e9bd224d0 100644 --- a/gvfs-helper.c +++ b/gvfs-helper.c @@ -2126,7 +2126,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; @@ -2160,16 +2159,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 @@ -2196,38 +2193,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); @@ -2243,7 +2233,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 dc91ee57ea3e67..0a2d6180430b50 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 \