From b4f06b47a38cea303f678e20d89943a2187b963f Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Mon, 11 Jan 2021 16:05:05 -0700 Subject: [PATCH 1/3] tests: Ensure no dangling commit partials on remote depth pull This was already being done on the local depth pull test, so this just adds the matching logic to the remote depth pull test. --- tests/test-pull-depth.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test-pull-depth.sh b/tests/test-pull-depth.sh index 1206c6c43d..998a18f552 100755 --- a/tests/test-pull-depth.sh +++ b/tests/test-pull-depth.sh @@ -35,21 +35,31 @@ ${CMD_PREFIX} ostree --repo=repo remote add --set=gpg-verify=false origin $(cat ${CMD_PREFIX} ostree --repo=repo pull --depth=0 origin main find repo/objects -name '*.commit' | wc -l > commitcount assert_file_has_content commitcount "^1$" +find repo/state -name '*.commitpartial' | wc -l > commitpartialcount +assert_file_has_content commitpartialcount "^0$" ${CMD_PREFIX} ostree --repo=repo pull --depth=0 origin main find repo/objects -name '*.commit' | wc -l > commitcount assert_file_has_content commitcount "^1$" +find repo/state -name '*.commitpartial' | wc -l > commitpartialcount +assert_file_has_content commitpartialcount "^0$" ${CMD_PREFIX} ostree --repo=repo pull --depth=1 origin main find repo/objects -name '*.commit' | wc -l > commitcount assert_file_has_content commitcount "^2$" +find repo/state -name '*.commitpartial' | wc -l > commitpartialcount +assert_file_has_content commitpartialcount "^0$" ${CMD_PREFIX} ostree --repo=repo pull --depth=1 origin main find repo/objects -name '*.commit' | wc -l > commitcount assert_file_has_content commitcount "^2$" +find repo/state -name '*.commitpartial' | wc -l > commitpartialcount +assert_file_has_content commitpartialcount "^0$" ${CMD_PREFIX} ostree --repo=repo pull --depth=-1 origin main find repo/objects -name '*.commit' | wc -l > commitcount assert_file_has_content commitcount "^3$" +find repo/state -name '*.commitpartial' | wc -l > commitpartialcount +assert_file_has_content commitpartialcount "^0$" echo "ok pull depth" From 20047ff1fea1b0d9193ee21d6df72d70b086090d Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Mon, 11 Jan 2021 12:40:38 -0700 Subject: [PATCH 2/3] pull: Error on depth pull with missing head commit When pulling with depth, missing parent commits are ignored. However, the check was applying to any commit, which means that it would succeed even if the requested commit was missing. This might happen on a corrupted remote repo or when using ref data from a stale summary. To achieve this, the semantics of the `commit_to_depth` hash table is changed slightly to only ever includes parent commits. This makes it easy to detect when a parent commit is being referenced (although there is a minor bug there when multiple refs are being pulled) while keeping references to commits that need their `commitpartial` files cleaned up. It also means that the table is only populated on depth pulls, which saves some memory and processing in the common depth=0 case. Fixes: #2265 --- src/libostree/ostree-repo-pull-private.h | 2 +- src/libostree/ostree-repo-pull.c | 56 ++++++++++-------------- tests/test-pull-depth.sh | 34 +++++++++++++- 3 files changed, 57 insertions(+), 35 deletions(-) diff --git a/src/libostree/ostree-repo-pull-private.h b/src/libostree/ostree-repo-pull-private.h index d4c3e971a7..59b72e88e4 100644 --- a/src/libostree/ostree-repo-pull-private.h +++ b/src/libostree/ostree-repo-pull-private.h @@ -88,7 +88,7 @@ typedef struct { GHashTable *ref_keyring_map; /* Maps OstreeCollectionRef to keyring remote name */ GPtrArray *static_delta_superblocks; GHashTable *expected_commit_sizes; /* Maps commit checksum to known size */ - GHashTable *commit_to_depth; /* Maps commit checksum maximum depth */ + GHashTable *commit_to_depth; /* Maps parent commit checksum maximum depth */ GHashTable *scanned_metadata; /* Maps object name to itself */ GHashTable *fetched_detached_metadata; /* Map */ GHashTable *requested_metadata; /* Maps object name to itself */ diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 7d4b91e286..abbb5a0dd5 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -1113,6 +1113,18 @@ on_metadata_written (GObject *object, check_outstanding_requests_handle_error (pull_data, &local_error); } +static gboolean +is_parent_commit (OtPullData *pull_data, + const char *checksum) +{ + /* FIXME: Only parent commits are added to the commit_to_depth table, + * so if the checksum isn't in the table then a new commit chain is + * being started. However, if the desired commit was a parent in a + * previously followed chain, then this will be wrong. + */ + return g_hash_table_contains (pull_data->commit_to_depth, checksum); +} + static void meta_fetch_on_complete (GObject *object, GAsyncResult *result, @@ -1158,7 +1170,8 @@ meta_fetch_on_complete (GObject *object, * We may be pulling from a partial repository that ends in * a dangling parent reference. */ else if (objtype == OSTREE_OBJECT_TYPE_COMMIT && - pull_data->maxdepth != 0) + pull_data->maxdepth != 0 && + is_parent_commit (pull_data, checksum)) { g_clear_error (&local_error); /* If the remote repo supports tombstone commits, check if the commit was intentionally @@ -1542,8 +1555,6 @@ scan_commit_object (OtPullData *pull_data, else { depth = pull_data->maxdepth; - g_hash_table_insert (pull_data->commit_to_depth, g_strdup (checksum), - GINT_TO_POINTER (depth)); } #ifndef OSTREE_DISABLE_GPGME @@ -1684,40 +1695,19 @@ scan_commit_object (OtPullData *pull_data, return FALSE; } - if (parent_csum_bytes != NULL && pull_data->maxdepth == -1) - { - queue_scan_one_metadata_object_c (pull_data, parent_csum_bytes, - OSTREE_OBJECT_TYPE_COMMIT, NULL, - recursion_depth + 1, NULL); - } - else if (parent_csum_bytes != NULL && depth > 0) + if (parent_csum_bytes != NULL && (pull_data->maxdepth == -1 || depth > 0)) { char parent_checksum[OSTREE_SHA256_STRING_LEN+1]; - gpointer parent_depthp; - int parent_depth; - ostree_checksum_inplace_from_bytes (parent_csum_bytes, parent_checksum); - if (g_hash_table_lookup_extended (pull_data->commit_to_depth, parent_checksum, - NULL, &parent_depthp)) - { - parent_depth = GPOINTER_TO_INT (parent_depthp); - } - else - { - parent_depth = depth - 1; - } - - if (parent_depth >= 0) - { - g_hash_table_insert (pull_data->commit_to_depth, g_strdup (parent_checksum), - GINT_TO_POINTER (parent_depth)); - queue_scan_one_metadata_object_c (pull_data, parent_csum_bytes, - OSTREE_OBJECT_TYPE_COMMIT, - NULL, - recursion_depth + 1, - NULL); - } + int parent_depth = (depth > 0) ? depth - 1 : -1; + g_hash_table_insert (pull_data->commit_to_depth, g_strdup (parent_checksum), + GINT_TO_POINTER (parent_depth)); + queue_scan_one_metadata_object_c (pull_data, parent_csum_bytes, + OSTREE_OBJECT_TYPE_COMMIT, + NULL, + recursion_depth + 1, + NULL); } /* We only recurse to looking whether we need dirtree/dirmeta diff --git a/tests/test-pull-depth.sh b/tests/test-pull-depth.sh index 998a18f552..8fb2f5973f 100755 --- a/tests/test-pull-depth.sh +++ b/tests/test-pull-depth.sh @@ -25,7 +25,7 @@ set -euo pipefail setup_fake_remote_repo1 "archive" -echo '1..1' +echo '1..3' cd ${test_tmpdir} mkdir repo @@ -63,3 +63,35 @@ find repo/state -name '*.commitpartial' | wc -l > commitpartialcount assert_file_has_content commitpartialcount "^0$" echo "ok pull depth" + +# Check that pulling with depth != 0 succeeds with a missing parent +# commit. Prune the remote to truncate the history. +cd ${test_tmpdir} +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo prune --refs-only --depth=0 + +rm -rf repo/refs/heads/* repo/refs/remotes/* repo/objects/*/*.commit +${CMD_PREFIX} ostree --repo=repo pull --depth=1 origin main +find repo/objects -name '*.commit' | wc -l > commitcount +assert_file_has_content commitcount "^1$" +find repo/state -name '*.commitpartial' | wc -l > commitpartialcount +assert_file_has_content commitpartialcount "^0$" + +rm -rf repo/refs/heads/* repo/refs/remotes/* repo/objects/*/*.commit +${CMD_PREFIX} ostree --repo=repo pull --depth=-1 origin main +find repo/objects -name '*.commit' | wc -l > commitcount +assert_file_has_content commitcount "^1$" +find repo/state -name '*.commitpartial' | wc -l > commitpartialcount +assert_file_has_content commitpartialcount "^0$" + +echo "ok pull depth missing parent" + +# Check that it errors if the ref head commit is missing. +cd ${test_tmpdir} +rm -f ostree-srv/gnomerepo/objects/*/*.commit + +rm -rf repo/refs/heads/* repo/refs/remotes/* repo/objects/*/*.commit +if ${CMD_PREFIX} ostree --repo=repo pull --depth=-1 origin main; then + fatal "Pull with depth -1 succeeded with missing HEAD commit" +fi + +echo "ok pull depth missing HEAD commit" From d7f2955f3717b452b71cb16c5360ff21f79515e7 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Mon, 11 Jan 2021 16:52:34 -0700 Subject: [PATCH 3/3] pull: Fix local pull with depth and truncated source history The local pull path was erroring on any missing commit, but that prevents a depth pull where the source repo has truncated history. As in the remote case, this also tries to pull in a tombstone commit if the source repo supports it. Fixes: #2266 --- src/libostree/ostree-repo-pull.c | 46 +++++++++++++++++++++++++++++--- tests/test-local-pull-depth.sh | 34 ++++++++++++++++++++++- 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index abbb5a0dd5..a95190a5b1 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -1167,8 +1167,10 @@ meta_fetch_on_complete (GObject *object, } /* When traversing parents, do not fail on a missing commit. - * We may be pulling from a partial repository that ends in - * a dangling parent reference. */ + * We may be pulling from a partial repository that ends in a + * dangling parent reference. This logic should match the + * local case in scan_one_metadata_object. + */ else if (objtype == OSTREE_OBJECT_TYPE_COMMIT && pull_data->maxdepth != 0 && is_parent_commit (pull_data, checksum)) @@ -1820,10 +1822,46 @@ scan_one_metadata_object (OtPullData *pull_data, return FALSE; } + g_autoptr(GError) local_error = NULL; if (!_ostree_repo_import_object (pull_data->repo, pull_data->remote_repo_local, objtype, checksum, pull_data->importflags, - cancellable, error)) - return FALSE; + cancellable, &local_error)) + { + /* When traversing parents, do not fail on a missing commit. + * We may be pulling from a partial repository that ends in a + * dangling parent reference. This logic should match the + * remote case in meta_fetch_on_complete. + * + * Note early return. + */ + if (g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND) && + objtype == OSTREE_OBJECT_TYPE_COMMIT && + pull_data->maxdepth != 0 && + is_parent_commit (pull_data, checksum)) + { + g_clear_error (&local_error); + + /* If the remote repo supports tombstone commits, check if + * the commit was intentionally deleted. + */ + if (pull_data->has_tombstone_commits) + { + if (!_ostree_repo_import_object (pull_data->repo, pull_data->remote_repo_local, + OSTREE_OBJECT_TYPE_TOMBSTONE_COMMIT, + checksum, pull_data->importflags, + cancellable, error)) + return FALSE; + } + + return TRUE; + } + else + { + g_propagate_error (error, g_steal_pointer (&local_error)); + return FALSE; + } + } + /* The import API will fetch both the commit and detached metadata, so * add it to the hash to avoid re-fetching it below. */ diff --git a/tests/test-local-pull-depth.sh b/tests/test-local-pull-depth.sh index 96b20b9cd9..80413bc923 100755 --- a/tests/test-local-pull-depth.sh +++ b/tests/test-local-pull-depth.sh @@ -25,7 +25,7 @@ set -euo pipefail setup_test_repository "archive" -echo "1..1" +echo "1..3" cd ${test_tmpdir} mkdir repo2 @@ -62,3 +62,35 @@ find repo2/state -name '*.commitpartial' | wc -l > commitpartialcount assert_file_has_content commitpartialcount "^0$" echo "ok local pull depth" + +# Check that pulling with depth != 0 succeeds with a missing parent +# commit. Prune the remote to truncate the history. +cd ${test_tmpdir} +${CMD_PREFIX} ostree --repo=repo prune --refs-only --depth=0 + +rm -rf repo2/refs/heads/* repo2/refs/remotes/* repo2/objects/*/*.commit +${CMD_PREFIX} ostree --repo=repo2 pull-local --depth=1 repo +find repo/objects -name '*.commit' | wc -l > commitcount +assert_file_has_content commitcount "^1$" +find repo/state -name '*.commitpartial' | wc -l > commitpartialcount +assert_file_has_content commitpartialcount "^0$" + +rm -rf repo2/refs/heads/* repo2/refs/remotes/* repo2/objects/*/*.commit +${CMD_PREFIX} ostree --repo=repo2 pull-local --depth=-1 repo +find repo/objects -name '*.commit' | wc -l > commitcount +assert_file_has_content commitcount "^1$" +find repo/state -name '*.commitpartial' | wc -l > commitpartialcount +assert_file_has_content commitpartialcount "^0$" + +echo "ok local pull depth missing parent" + +# Check that it errors if the ref head commit is missing. +cd ${test_tmpdir} +rm -f repo/objects/*/*.commit + +rm -rf repo2/refs/heads/* repo2/refs/remotes/* repo2/objects/*/*.commit +if ${CMD_PREFIX} ostree --repo=repo2 pull-local --depth=-1 repo; then + fatal "Local pull with depth -1 succeeded with missing HEAD commit" +fi + +echo "ok local pull depth missing HEAD commit"