]> git.ipfire.org Git - thirdparty/libvirt.git/commitdiff
storage: use valid XML for awkward volume names
authorEric Blake <eblake@redhat.com>
Thu, 21 Nov 2013 00:04:05 +0000 (17:04 -0700)
committerCole Robinson <crobinso@redhat.com>
Mon, 10 Mar 2014 13:16:58 +0000 (09:16 -0400)
$ touch /var/lib/libvirt/images/'a<b>c'
$ virsh pool-refresh default
$ virsh vol-dumpxml 'a<b>c' default | head -n2
<volume>
  <name>a<b>c</name>

Oops.  That's not valid XML.  And when we fix the XML
generation, it fails RelaxNG validation.

I'm also tired of seeing <key>(null)</key> in the example
output for volume xml; while we used NULLSTR() to avoid
a NULL deref rather than relying on glibc's printf
extension behavior, it's even better if we avoid the issue
in the first place.  But this requires being careful that
we don't invalidate any storage backends that were relying
on key being unassigned during virStoragVolCreateXML[From].

I would have split this into two patches (one for escaping,
one for avoiding <key>(null)</key>), but since they both
end up touching a lot of the same test files, I ended up
merging it into one.

Note that this patch allows pretty much any volume name
that can appear in a directory (excluding . and .. because
those are special), but does nothing to change the current
(unenforced) RelaxNG claim that pool names will consist
only of letters, numbers, _, -, and +.  Tightening the C
code to match RelaxNG patterns and/or relaxing the grammar
to match the C code for pool names is a task for another
day (but remember, we DID recently tighten C code for
domain names to exclude a leading '.').

* src/conf/storage_conf.c (virStoragePoolSourceFormat)
(virStoragePoolDefFormat, virStorageVolTargetDefFormat)
(virStorageVolDefFormat): Escape user-controlled strings.
(virStorageVolDefParseXML): Parse key, for use in unit tests.
* src/storage/storage_driver.c (storageVolCreateXML)
(storageVolCreateXMLFrom): Ensure parsed key doesn't confuse
volume creation.
* docs/schemas/basictypes.rng (volName): Relax definition.
* tests/storagepoolxml2xmltest.c (mymain): Test it.
* tests/storagevolxml2xmltest.c (mymain): Likewise.
* tests/storagepoolxml2xmlin/pool-dir-naming.xml: New file.
* tests/storagepoolxml2xmlout/pool-dir-naming.xml: Likewise.
* tests/storagevolxml2xmlin/vol-file-naming.xml: Likewise.
* tests/storagevolxml2xmlout/vol-file-naming.xml: Likewise.
* tests/storagevolxml2xmlout/vol-*.xml: Fix fallout.

Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 6cc4d6a3fe82653c607c4f159901790298e80e1f)

21 files changed:
docs/schemas/basictypes.rng
src/conf/storage_conf.c
src/storage/storage_driver.c
tests/storagepoolxml2xmlin/pool-dir-naming.xml [new file with mode: 0644]
tests/storagepoolxml2xmlout/pool-dir-naming.xml [new file with mode: 0644]
tests/storagepoolxml2xmltest.c
tests/storagevolxml2xmlin/vol-file-backing.xml
tests/storagevolxml2xmlin/vol-file-naming.xml [new file with mode: 0644]
tests/storagevolxml2xmlout/vol-file-backing.xml
tests/storagevolxml2xmlout/vol-file-naming.xml [new file with mode: 0644]
tests/storagevolxml2xmlout/vol-file.xml
tests/storagevolxml2xmlout/vol-logical-backing.xml
tests/storagevolxml2xmlout/vol-logical.xml
tests/storagevolxml2xmlout/vol-partition.xml
tests/storagevolxml2xmlout/vol-qcow2-0.10-lazy.xml
tests/storagevolxml2xmlout/vol-qcow2-1.1.xml
tests/storagevolxml2xmlout/vol-qcow2-lazy.xml
tests/storagevolxml2xmlout/vol-qcow2-nobacking.xml
tests/storagevolxml2xmlout/vol-qcow2.xml
tests/storagevolxml2xmlout/vol-sheepdog.xml
tests/storagevolxml2xmltest.c

index 34c22543306321b2062f89180f4059fa513f563d..48806fcb7ee722c1560cb40a00597ba639036f54 100644 (file)
   </define>
 
   <define name='volName'>
+    <!-- directory pools allow almost any file name as a volume name -->
     <data type='string'>
-      <param name="pattern">[a-zA-Z0-9_\+\-\.]+</param>
+      <param name="pattern">[^/]+</param>
+      <except>
+        <choice>
+          <value>.</value>
+          <value>..</value>
+        </choice>
+      </except>
     </data>
   </define>
 
index 33e4cafc56bc703ecda368f0fa76d868e6f97db6..8b378c208ebbd7c9ec1e137c68f35d1ae8651c90 100644 (file)
@@ -1056,7 +1056,8 @@ virStoragePoolSourceFormat(virBufferPtr buf,
     virBufferAddLit(buf, "  <source>\n");
     if ((options->flags & VIR_STORAGE_POOL_SOURCE_HOST) && src->nhost) {
         for (i = 0; i < src->nhost; i++) {
-            virBufferAsprintf(buf, "    <host name='%s'", src->hosts[i].name);
+            virBufferEscapeString(buf, "    <host name='%s'",
+                                  src->hosts[i].name);
             if (src->hosts[i].port)
                 virBufferAsprintf(buf, " port='%d'", src->hosts[i].port);
             virBufferAddLit(buf, "/>\n");
@@ -1067,8 +1068,8 @@ virStoragePoolSourceFormat(virBufferPtr buf,
         src->ndevice) {
         for (i = 0; i < src->ndevice; i++) {
             if (src->devices[i].nfreeExtent) {
-                virBufferAsprintf(buf, "    <device path='%s'>\n",
-                                  src->devices[i].path);
+                virBufferEscapeString(buf, "    <device path='%s'>\n",
+                                      src->devices[i].path);
                 for (j = 0; j < src->devices[i].nfreeExtent; j++) {
                     virBufferAsprintf(buf, "    <freeExtent start='%llu' end='%llu'/>\n",
                                       src->devices[i].freeExtents[j].start,
@@ -1076,15 +1077,14 @@ virStoragePoolSourceFormat(virBufferPtr buf,
                 }
                 virBufferAddLit(buf, "    </device>\n");
             } else {
-                virBufferAsprintf(buf, "    <device path='%s'/>\n",
-                                  src->devices[i].path);
+                virBufferEscapeString(buf, "    <device path='%s'/>\n",
+                                      src->devices[i].path);
             }
         }
     }
 
-    if ((options->flags & VIR_STORAGE_POOL_SOURCE_DIR) &&
-        src->dir)
-        virBufferAsprintf(buf, "    <dir path='%s'/>\n", src->dir);
+    if (options->flags & VIR_STORAGE_POOL_SOURCE_DIR)
+        virBufferEscapeString(buf, "    <dir path='%s'/>\n", src->dir);
 
     if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER)) {
         if (src->adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST ||
@@ -1104,9 +1104,8 @@ virStoragePoolSourceFormat(virBufferPtr buf,
         }
     }
 
-    if ((options->flags & VIR_STORAGE_POOL_SOURCE_NAME) &&
-        src->name)
-        virBufferAsprintf(buf, "    <name>%s</name>\n", src->name);
+    if (options->flags & VIR_STORAGE_POOL_SOURCE_NAME)
+        virBufferEscapeString(buf, "    <name>%s</name>\n", src->name);
 
     if ((options->flags & VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN) &&
         src->initiator.iqn) {
@@ -1129,11 +1128,12 @@ virStoragePoolSourceFormat(virBufferPtr buf,
 
     if (src->authType == VIR_STORAGE_POOL_AUTH_CHAP ||
         src->authType == VIR_STORAGE_POOL_AUTH_CEPHX) {
-        virBufferAsprintf(buf, "    <auth type='%s' username='%s'>\n",
-                          virStoragePoolAuthTypeTypeToString(src->authType),
-                          (src->authType == VIR_STORAGE_POOL_AUTH_CHAP ?
-                           src->auth.chap.username :
-                           src->auth.cephx.username));
+        virBufferAsprintf(buf, "    <auth type='%s' ",
+                          virStoragePoolAuthTypeTypeToString(src->authType));
+        virBufferEscapeString(buf, "username='%s'>\n",
+                              (src->authType == VIR_STORAGE_POOL_AUTH_CHAP ?
+                               src->auth.chap.username :
+                               src->auth.cephx.username));
 
         virBufferAddLit(buf, "      <secret");
         if (src->auth.cephx.secret.uuidUsable) {
@@ -1149,13 +1149,8 @@ virStoragePoolSourceFormat(virBufferPtr buf,
         virBufferAddLit(buf, "    </auth>\n");
     }
 
-    if (src->vendor != NULL) {
-        virBufferEscapeString(buf, "    <vendor name='%s'/>\n", src->vendor);
-    }
-
-    if (src->product != NULL) {
-        virBufferEscapeString(buf, "    <product name='%s'/>\n", src->product);
-    }
+    virBufferEscapeString(buf, "    <vendor name='%s'/>\n", src->vendor);
+    virBufferEscapeString(buf, "    <product name='%s'/>\n", src->product);
 
     virBufferAddLit(buf, "  </source>\n");
 
@@ -1182,7 +1177,7 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def)
         goto cleanup;
     }
     virBufferAsprintf(&buf, "<pool type='%s'>\n", type);
-    virBufferAsprintf(&buf, "  <name>%s</name>\n", def->name);
+    virBufferEscapeString(&buf, "  <name>%s</name>\n", def->name);
 
     virUUIDFormat(def->uuid, uuid);
     virBufferAsprintf(&buf, "  <uuid>%s</uuid>\n", uuid);
@@ -1203,8 +1198,7 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def)
         def->type != VIR_STORAGE_POOL_SHEEPDOG) {
         virBufferAddLit(&buf, "  <target>\n");
 
-        if (def->target.path)
-            virBufferAsprintf(&buf, "    <path>%s</path>\n", def->target.path);
+        virBufferEscapeString(&buf, "    <path>%s</path>\n", def->target.path);
 
         virBufferAddLit(&buf, "    <permissions>\n");
         virBufferAsprintf(&buf, "      <mode>0%o</mode>\n",
@@ -1214,9 +1208,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def)
         virBufferAsprintf(&buf, "      <group>%d</group>\n",
                           (int) def->target.perms.gid);
 
-        if (def->target.perms.label)
-            virBufferAsprintf(&buf, "      <label>%s</label>\n",
-                            def->target.perms.label);
+        virBufferEscapeString(&buf, "      <label>%s</label>\n",
+                              def->target.perms.label);
 
         virBufferAddLit(&buf, "    </permissions>\n");
         virBufferAddLit(&buf, "  </target>\n");
@@ -1282,8 +1275,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
         goto error;
     }
 
-    /* Auto-generated so deliberately ignore */
-    /* ret->key = virXPathString("string(./key)", ctxt); */
+    /* Normally generated by pool refresh, but useful for unit tests */
+    ret->key = virXPathString("string(./key)", ctxt);
 
     capacity = virXPathString("string(./capacity)", ctxt);
     unit = virXPathString("string(./capacity/@unit)", ctxt);
@@ -1485,11 +1478,11 @@ static int
 virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
                              virBufferPtr buf,
                              virStorageVolTargetPtr def,
-                             const char *type) {
+                             const char *type)
+{
     virBufferAsprintf(buf, "  <%s>\n", type);
 
-    if (def->path)
-        virBufferAsprintf(buf, "    <path>%s</path>\n", def->path);
+    virBufferEscapeString(buf, "    <path>%s</path>\n", def->path);
 
     if (options->formatToString) {
         const char *format = (options->formatToString)(def->format);
@@ -1511,8 +1504,7 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options,
                       (unsigned int) def->perms.gid);
 
 
-    if (def->perms.label)
-        virBufferAsprintf(buf, "      <label>%s</label>\n",
+    virBufferEscapeString(buf, "      <label>%s</label>\n",
                           def->perms.label);
 
     virBufferAddLit(buf, "    </permissions>\n");
@@ -1572,8 +1564,8 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool,
         return NULL;
 
     virBufferAddLit(&buf, "<volume>\n");
-    virBufferAsprintf(&buf, "  <name>%s</name>\n", def->name);
-    virBufferAsprintf(&buf, "  <key>%s</key>\n", NULLSTR(def->key));
+    virBufferEscapeString(&buf, "  <name>%s</name>\n", def->name);
+    virBufferEscapeString(&buf, "  <key>%s</key>\n", def->key);
     virBufferAddLit(&buf, "  <source>\n");
 
     if (def->source.nextent) {
@@ -1585,8 +1577,8 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool,
                 if (thispath != NULL)
                     virBufferAddLit(&buf, "    </device>\n");
 
-                virBufferAsprintf(&buf, "    <device path='%s'>\n",
-                                  def->source.extents[i].path);
+                virBufferEscapeString(&buf, "    <device path='%s'>\n",
+                                      def->source.extents[i].path);
             }
 
             virBufferAsprintf(&buf,
index 6c392840f48b7f83229186b79c28b2e6071b41e8..702a118065d0b50777dd523892c50353b14b37dc 100644 (file)
@@ -1554,6 +1554,9 @@ storageVolCreateXML(virStoragePoolPtr obj,
         goto cleanup;
     }
 
+    /* Wipe any key the user may have suggested, as volume creation
+     * will generate the canonical key.  */
+    VIR_FREE(voldef->key);
     if (backend->createVol(obj->conn, pool, voldef) < 0) {
         goto cleanup;
     }
@@ -1729,7 +1732,10 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
                       pool->volumes.count+1) < 0)
         goto cleanup;
 
-    /* 'Define' the new volume so we get async progress reporting */
+    /* 'Define' the new volume so we get async progress reporting.
+     * Wipe any key the user may have suggested, as volume creation
+     * will generate the canonical key.  */
+    VIR_FREE(newvol->key);
     if (backend->createVol(obj->conn, pool, newvol) < 0) {
         goto cleanup;
     }
diff --git a/tests/storagepoolxml2xmlin/pool-dir-naming.xml b/tests/storagepoolxml2xmlin/pool-dir-naming.xml
new file mode 100644 (file)
index 0000000..aa043be
--- /dev/null
@@ -0,0 +1,18 @@
+<pool type='dir'>
+  <name>virtimages</name>
+  <uuid>70a7eb15-6c34-ee9c-bf57-69e8e5ff3fb2</uuid>
+  <capacity>0</capacity>
+  <allocation>0</allocation>
+  <available>0</available>
+  <source>
+  </source>
+  <target>
+    <path>///var/////lib/libvirt/&lt;images&gt;//</path>
+    <permissions>
+      <mode>0700</mode>
+      <owner>-1</owner>
+      <group>-1</group>
+      <label>some_label_t</label>
+    </permissions>
+  </target>
+</pool>
diff --git a/tests/storagepoolxml2xmlout/pool-dir-naming.xml b/tests/storagepoolxml2xmlout/pool-dir-naming.xml
new file mode 100644 (file)
index 0000000..536f58c
--- /dev/null
@@ -0,0 +1,18 @@
+<pool type='dir'>
+  <name>virtimages</name>
+  <uuid>70a7eb15-6c34-ee9c-bf57-69e8e5ff3fb2</uuid>
+  <capacity unit='bytes'>0</capacity>
+  <allocation unit='bytes'>0</allocation>
+  <available unit='bytes'>0</available>
+  <source>
+  </source>
+  <target>
+    <path>/var/lib/libvirt/&lt;images&gt;</path>
+    <permissions>
+      <mode>0700</mode>
+      <owner>-1</owner>
+      <group>-1</group>
+      <label>some_label_t</label>
+    </permissions>
+  </target>
+</pool>
index d59cff9746ab610a7ac2602c3f3f38d9ea0140ab..c7159c6acc5be9d36c94a8a5d6c5b2fe576fc6b1 100644 (file)
@@ -85,6 +85,7 @@ mymain(void)
         ret = -1
 
     DO_TEST("pool-dir");
+    DO_TEST("pool-dir-naming");
     DO_TEST("pool-fs");
     DO_TEST("pool-logical");
     DO_TEST("pool-logical-nopath");
index d23349e658aa03a558e5295b8482cc2d641613d4..73e7f288df3d737677a69d85a79d35f3d2b76141 100644 (file)
@@ -1,5 +1,6 @@
 <volume>
   <name>sparse.img</name>
+  <key>/var/lib/libvirt/images/sparse.img</key>
   <source/>
   <capacity unit='GB'>10</capacity>
   <allocation unit='MiB'>0</allocation>
diff --git a/tests/storagevolxml2xmlin/vol-file-naming.xml b/tests/storagevolxml2xmlin/vol-file-naming.xml
new file mode 100644 (file)
index 0000000..9a33e2b
--- /dev/null
@@ -0,0 +1,20 @@
+<volume>
+  <name>&lt;sparse&gt;.img</name>
+  <source/>
+  <capacity unit="TiB">1</capacity>
+  <allocation unit="bytes">0</allocation>
+  <target>
+    <path>/var/lib/libvirt/images/&lt;sparse&gt;.img</path>
+    <permissions>
+      <mode>0</mode>
+      <owner>0744</owner>
+      <group>0</group>
+      <label>virt_image_t</label>
+    </permissions>
+    <timestamps>
+      <atime>1341933637.273190990</atime>
+      <mtime>1341930622.047245868</mtime>
+      <ctime>1341930622.047245868</ctime>
+    </timestamps>
+  </target>
+</volume>
index c0f152e84f19d744d2559b29b03497d73b8e2af8..8d2fb576242d816c8786ac36d195c2007bf33b06 100644 (file)
@@ -1,6 +1,6 @@
 <volume>
   <name>sparse.img</name>
-  <key>(null)</key>
+  <key>/var/lib/libvirt/images/sparse.img</key>
   <source>
   </source>
   <capacity unit='bytes'>10000000000</capacity>
diff --git a/tests/storagevolxml2xmlout/vol-file-naming.xml b/tests/storagevolxml2xmlout/vol-file-naming.xml
new file mode 100644 (file)
index 0000000..7022b02
--- /dev/null
@@ -0,0 +1,17 @@
+<volume>
+  <name>&lt;sparse&gt;.img</name>
+  <source>
+  </source>
+  <capacity unit='bytes'>1099511627776</capacity>
+  <allocation unit='bytes'>0</allocation>
+  <target>
+    <path>/var/lib/libvirt/images/&lt;sparse&gt;.img</path>
+    <format type='raw'/>
+    <permissions>
+      <mode>00</mode>
+      <owner>744</owner>
+      <group>0</group>
+      <label>virt_image_t</label>
+    </permissions>
+  </target>
+</volume>
index a3d64738672e56be5a722620653e364bd962b9a2..b97dd5040140ce8db378c71988c7b900f9b01011 100644 (file)
@@ -1,6 +1,5 @@
 <volume>
   <name>sparse.img</name>
-  <key>(null)</key>
   <source>
   </source>
   <capacity unit='bytes'>1099511627776</capacity>
index 6b010e33bb7915ccccc759e53fd59d7293d12bf1..bf34b086782f61ab1acfbbbf8f7c05b0654d1658 100644 (file)
@@ -1,6 +1,6 @@
 <volume>
   <name>Swap</name>
-  <key>(null)</key>
+  <key>r4xkCv-MQhr-WKIT-R66x-Epn2-e8hG-1Z5gY0</key>
   <source>
   </source>
   <capacity unit='bytes'>2080374784</capacity>
index 7bf309eddd284261dced658b0b56aa4dfaffa4c2..e9b4e4baf4a5fa98598c649de81e1a516fba4dd1 100644 (file)
@@ -1,6 +1,6 @@
 <volume>
   <name>Swap</name>
-  <key>(null)</key>
+  <key>r4xkCv-MQhr-WKIT-R66x-Epn2-e8hG-1Z5gY0</key>
   <source>
   </source>
   <capacity unit='bytes'>2080374784</capacity>
index 271964f5bf2f642916fafe269a4b29188d3235d1..9be1cf175b22fe27c1bb38e99bc98996d0662cf1 100644 (file)
@@ -1,6 +1,6 @@
 <volume>
   <name>sda1</name>
-  <key>(null)</key>
+  <key>/dev/sda1</key>
   <source>
   </source>
   <capacity unit='bytes'>106896384</capacity>
index a7b5fedfb8f2b87cca569259766872d791d356ac..fd3d6069e0b244f8639e02c5fde85cc35ff6e808 100644 (file)
@@ -1,6 +1,6 @@
 <volume>
   <name>OtherDemo.img</name>
-  <key>(null)</key>
+  <key>/var/lib/libvirt/images/OtherDemo.img</key>
   <source>
   </source>
   <capacity unit='bytes'>5368709120</capacity>
index b7df8a6caecb4aca083408da84c650816a1fe595..99fb5acafbf0feae01e4597907d4165c39839959 100644 (file)
@@ -1,6 +1,6 @@
 <volume>
   <name>OtherDemo.img</name>
-  <key>(null)</key>
+  <key>/var/lib/libvirt/images/OtherDemo.img</key>
   <source>
   </source>
   <capacity unit='bytes'>5368709120</capacity>
index 92b78756689245f0d3d1140dde5e40dcf9a9f41e..3708ea742bfde58527e135cadfd9785445d704dc 100644 (file)
@@ -1,6 +1,6 @@
 <volume>
   <name>OtherDemo.img</name>
-  <key>(null)</key>
+  <key>/var/lib/libvirt/images/OtherDemo.img</key>
   <source>
   </source>
   <capacity unit='bytes'>5368709120</capacity>
index e2da702bcd4a72430c2123de803ad69d12cbc10e..f6a2e2183da93c7d45faf538af8e3ac0623cf6fa 100644 (file)
@@ -1,6 +1,6 @@
 <volume>
   <name>OtherDemo.img</name>
-  <key>(null)</key>
+  <key>/var/lib/libvirt/images/OtherDemo.img</key>
   <source>
   </source>
   <capacity unit='bytes'>5368709120</capacity>
index f931a629cdb7e5622d2b3c06a74825eb8fdcdc69..b9adcb4e205f035367bad8b632ee620a5a76708a 100644 (file)
@@ -1,6 +1,6 @@
 <volume>
   <name>OtherDemo.img</name>
-  <key>(null)</key>
+  <key>/var/lib/libvirt/images/OtherDemo.img</key>
   <source>
   </source>
   <capacity unit='bytes'>5368709120</capacity>
index 2f19af8f1942fd132b08b6623a3c557e8fc4923a..bd5d6d8f06bc6951905d503eb3b8eeb0d6e05efc 100644 (file)
@@ -1,6 +1,5 @@
 <volume>
   <name>test2</name>
-  <key>(null)</key>
   <source>
   </source>
   <capacity unit='bytes'>1024</capacity>
index 5b0a60b64086e9a8b9ee131044dea9d8505f8b4d..d62c29f3428fe9845d600649982ed6985c5ddd70 100644 (file)
@@ -110,6 +110,7 @@ mymain(void)
     while (0);
 
     DO_TEST("pool-dir", "vol-file");
+    DO_TEST("pool-dir", "vol-file-naming");
     DO_TEST("pool-dir", "vol-file-backing");
     DO_TEST("pool-dir", "vol-qcow2");
     DO_TEST("pool-dir", "vol-qcow2-1.1");