From c71fc3d9940a09287e215797926e591cd72f72c6 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 15 Jun 2023 14:26:12 -0400 Subject: [PATCH] Add more error prefixing when parsing commit objects I've got more debug information in the error case that motivated https://github.com/ostreedev/ostree/pull/2884/commits/bae4347abeaa2a66d213758f790058f42cb71fd1 "pull: Add error prefixing for corrupt checksums" where the sole error is `error: Invalid checksum of length 0 expected 32` This must be coming from the pull code in the case where we've already fetched the commit object. - Add some error prefixing here in the core commit validation code - Ensure that we do the validation immediately after loading, including of the parent commit reference where I think this error must be coming from - Then the pull code can just safely call `ostree_commit_get_parent` which already does the hex conversion etc. --- src/libostree/ostree-core.c | 6 +++--- src/libostree/ostree-repo-pull.c | 34 +++++++++++++------------------- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/src/libostree/ostree-core.c b/src/libostree/ostree-core.c index 921401c1..bbe76aaa 100644 --- a/src/libostree/ostree-core.c +++ b/src/libostree/ostree-core.c @@ -2152,18 +2152,18 @@ ostree_validate_structureof_commit (GVariant *commit, GError **error) if (n_elts > 0) { if (!ostree_validate_structureof_csum_v (parent_csum_v, error)) - return FALSE; + return glnx_prefix_error (error, "Invalid commit parent"); } g_autoptr (GVariant) content_csum_v = NULL; g_variant_get_child (commit, 6, "@ay", &content_csum_v); if (!ostree_validate_structureof_csum_v (content_csum_v, error)) - return FALSE; + return glnx_prefix_error (error, "Invalid commit tree content checksum"); g_autoptr (GVariant) metadata_csum_v = NULL; g_variant_get_child (commit, 7, "@ay", &metadata_csum_v); if (!ostree_validate_structureof_csum_v (metadata_csum_v, error)) - return FALSE; + return glnx_prefix_error (error, "Invalid commit tree metadata checksum"); return TRUE; } diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 0dd97664..6d69d79e 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -1538,6 +1538,10 @@ scan_commit_object (OtPullData *pull_data, const char *checksum, guint recursion if (!ostree_repo_load_commit (pull_data->repo, checksum, &commit, &commitstate, error)) return FALSE; + /* Do this early because if it's corrupt, something else is going wrong */ + if (!ostree_validate_structureof_commit (commit, error)) + return glnx_prefix_error (error, "Validating commit %s", checksum); + if (!pull_data->disable_verify_bindings) { /* If ref is non-NULL then the commit we fetched was requested through @@ -1590,27 +1594,17 @@ scan_commit_object (OtPullData *pull_data, const char *checksum, guint recursion /* If we found a legacy transaction flag, assume all commits are partial */ gboolean is_partial = commitstate_is_partial (pull_data, commitstate); - /* PARSE OSTREE_SERIALIZED_COMMIT_VARIANT */ - g_autoptr (GVariant) parent_csum = NULL; - const guchar *parent_csum_bytes = NULL; - g_variant_get_child (commit, 1, "@ay", &parent_csum); - if (g_variant_n_children (parent_csum) > 0) - { - parent_csum_bytes = ostree_checksum_bytes_peek_validate (parent_csum, error); - if (parent_csum_bytes == NULL) - return FALSE; - } - - if (parent_csum_bytes != NULL && (pull_data->maxdepth == -1 || depth > 0)) + if (pull_data->maxdepth == -1 || depth > 0) { - char parent_checksum[OSTREE_SHA256_STRING_LEN + 1]; - ostree_checksum_inplace_from_bytes (parent_csum_bytes, parent_checksum); - - 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); + g_autofree char *parent_checksum = ostree_commit_get_parent (commit); + if (parent_checksum) + { + 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 (pull_data, parent_checksum, OSTREE_OBJECT_TYPE_COMMIT, + NULL, recursion_depth + 1, NULL); + } } /* We only recurse to looking whether we need dirtree/dirmeta -- 2.30.2