virStorageSourceGetMetadata: Use depth limit instead of unique path checking

Prevent unbounded chains by limiting the recursion depth of
virStorageSourceGetMetadataRecurse to the maximum number of image layers
we limit anyways.

This removes the last use of virStorageSourceGetUniqueIdentifier which
will allow us to delete some crusty old infrastructure which isn't
really needed.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3624ed035f5dc80897f6e32283e4ccd9169f9028..c3d85209bb319aefabb74dcf6bbee7b8bba736c5 100644 (file)
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7637,7 +7637,9 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
 
     qemuDomainGetImageIds(cfg, vm, src, disksrc, &uid, &gid);
 
-    if (virStorageSourceGetMetadata(src, uid, gid, report_broken) < 0)
+    if (virStorageSourceGetMetadata(src, uid, gid,
+                                    QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH,
+                                    report_broken) < 0)
         return -1;
 
     for (n = src->backingStore; virStorageSourceIsBacking(n); n = n->backingStore) {

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index c6153053202d6c87cf91527ecef27add18aa42fc..7e29e43c2e9a54df93433c530c4419e26e4fea2c 100644 (file)
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -937,9 +937,12 @@ get_files(vahControl * ctl)
             continue;
         /* XXX - if we knew the qemu user:group here we could send it in
          *        so that the open could be re-tried as that user:group.
+         *
+         * The maximum depth is limited to 200 layers similarly to the qemu
+         * implementation.
          */
         if (!disk->src->backingStore)
-            virStorageSourceGetMetadata(disk->src, -1, -1, false);
+            virStorageSourceGetMetadata(disk->src, -1, -1, 200, false);
 
          /* XXX should handle open errors more careful than just ignoring them.
          */

diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c
index ffe150a9b0be33239051b87454493e939fb0b506..19b06b02b87917f9a2bc8ccf8637eaf595ccd06d 100644 (file)
--- a/src/storage_file/storage_source.c
+++ b/src/storage_file/storage_source.c
@@ -1297,11 +1297,9 @@ virStorageSourceGetMetadataRecurseReadHeader(virStorageSourcePtr src,
                                              uid_t uid,
                                              gid_t gid,
                                              char **buf,
-                                             size_t *headerLen,
-                                             GHashTable *cycle)
+                                             size_t *headerLen)
 {
     int ret = -1;
-    const char *uniqueName;
     ssize_t len;
 
     if (virStorageSourceInitAs(src, uid, gid) < 0)
@@ -1312,19 +1310,6 @@ virStorageSourceGetMetadataRecurseReadHeader(virStorageSourcePtr src,
         goto cleanup;
     }
 
-    if (!(uniqueName = virStorageSourceGetUniqueIdentifier(src)))
-        goto cleanup;
-
-    if (virHashHasEntry(cycle, uniqueName)) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("backing store for %s (%s) is self-referential"),
-                       NULLSTR(src->path), uniqueName);
-        goto cleanup;
-    }
-
-    if (virHashAddEntry(cycle, uniqueName, NULL) < 0)
-        goto cleanup;
-
     if ((len = virStorageSourceRead(src, 0, VIR_STORAGE_MAX_HEADER, buf)) < 0)
         goto cleanup;
 
@@ -1343,7 +1328,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
                                    virStorageSourcePtr parent,
                                    uid_t uid, gid_t gid,
                                    bool report_broken,
-                                   GHashTable *cycle,
+                                   size_t max_depth,
                                    unsigned int depth)
 {
     virStorageFileFormat orig_format = src->format;
@@ -1352,9 +1337,16 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
     g_autofree char *buf = NULL;
     g_autoptr(virStorageSource) backingStore = NULL;
 
-    VIR_DEBUG("path=%s format=%d uid=%u gid=%u",
+    VIR_DEBUG("path=%s format=%d uid=%u gid=%u depth=%u",
               NULLSTR(src->path), src->format,
-              (unsigned int)uid, (unsigned int)gid);
+              (unsigned int)uid, (unsigned int)gid, depth);
+
+    if (depth > max_depth) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("backing store for %s is self-referential or too deeply nested"),
+                       NULLSTR(src->path));
+        return -1;
+    }
 
     if (src->format == VIR_STORAGE_FILE_AUTO_SAFE)
         src->format = VIR_STORAGE_FILE_AUTO;
@@ -1369,7 +1361,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
     }
 
     if (virStorageSourceGetMetadataRecurseReadHeader(src, parent, uid, gid,
-                                                     &buf, &headerLen, cycle) < 0)
+                                                     &buf, &headerLen) < 0)
         return -1;
 
     if (virStorageFileProbeGetMetadata(src, buf, headerLen) < 0)
@@ -1396,7 +1388,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
         if ((rv = virStorageSourceGetMetadataRecurse(backingStore, parent,
                                                      uid, gid,
                                                      report_broken,
-                                                     cycle, depth + 1)) < 0) {
+                                                     max_depth, depth + 1)) < 0) {
             if (!report_broken)
                 return 0;
 
@@ -1427,7 +1419,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
  * Extract metadata about the storage volume with the specified
  * image format. If image format is VIR_STORAGE_FILE_AUTO, it
  * will probe to automatically identify the format.  Recurses through
- * the entire chain.
+ * the chain up to @max_depth layers.
  *
  * Open files using UID and GID (or pass -1 for the current user/group).
  * Treat any backing files without explicit type as raw, unless ALLOW_PROBE.
@@ -1445,14 +1437,14 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src,
 int
 virStorageSourceGetMetadata(virStorageSourcePtr src,
                             uid_t uid, gid_t gid,
+                            size_t max_depth,
                             bool report_broken)
 {
-    g_autoptr(GHashTable) cycle = virHashNew(NULL);
     virStorageType actualType = virStorageSourceGetActualType(src);
 
-    VIR_DEBUG("path=%s format=%d uid=%u gid=%u report_broken=%d",
+    VIR_DEBUG("path=%s format=%d uid=%u gid=%u max_depth=%zu report_broken=%d",
               src->path, src->format, (unsigned int)uid, (unsigned int)gid,
-              report_broken);
+              max_depth, report_broken);
 
     if (src->format <= VIR_STORAGE_FILE_NONE) {
         if (actualType == VIR_STORAGE_TYPE_DIR)
@@ -1462,5 +1454,5 @@ virStorageSourceGetMetadata(virStorageSourcePtr src,
     }
 
     return virStorageSourceGetMetadataRecurse(src, src, uid, gid,
-                                              report_broken, cycle, 1);
+                                              report_broken, max_depth, 1);
 }

diff --git a/src/storage_file/storage_source.h b/src/storage_file/storage_source.h
index 6eb795b301f94284ca110b454eba567fa939c236..5e05bde7b1e1413bc9ab1040f3463c21c38e9de3 100644 (file)
--- a/src/storage_file/storage_source.h
+++ b/src/storage_file/storage_source.h
@@ -134,6 +134,7 @@ virStorageSourceSupportsBackingChainTraversal(const virStorageSource *src);
 int
 virStorageSourceGetMetadata(virStorageSourcePtr src,
                             uid_t uid, gid_t gid,
+                            size_t max_depth,
                             bool report_broken)
     ATTRIBUTE_NONNULL(1);
 

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 84dd813b8046a8c8e5af685da2c2f265b790c3ff..157d577c7d5202cefdcc2af725280747fbc37430 100644 (file)
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -101,7 +101,8 @@ testStorageFileGetMetadata(const char *path,
 
     def->path = g_strdup(path);
 
-    if (virStorageSourceGetMetadata(def, uid, gid, true) < 0)
+    /* 20 is picked as an arbitrary depth, since the chains used here don't exceed it */
+    if (virStorageSourceGetMetadata(def, uid, gid, 20, true) < 0)
         return NULL;
 
     return g_steal_pointer(&def);