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.

diff --git a/src/libostree/ostree-core.c b/src/libostree/ostree-core.c
index 921401c14391153c2e884951b94ff52ce2ff8439..bbe76aaacb61fcb5c1502247dd145e5cafac0d80 100644 (file)
--- 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 0dd97664fba3b81cff9c1fdd28a9b997bb5fced5..6d69d79ee56b075836261779e9f6d9e948d4c742 100644 (file)
--- 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