]> 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)
committerEric Blake <eblake@redhat.com>
Fri, 22 Nov 2013 00:29:47 +0000 (17:29 -0700)
$ 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>
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 7fbfa5340a810fb4603822306834d71e48ff3e8e..268bc5a7b40fe2889babcaaf3415de8ab35ba60c 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 b3f0871fce9318b1344dd41d416a07d16c04e290..3b4715a240710daa12ec89cb26c85e84461fe12f 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 743e1cb86d84fb7c76d4c4c3ab49a144789e4c45..a81cf9970486b00e8eeeaa3001f3b7b8b7f2e4af 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 1fd01f15c4695b5bb920fed92abd281467a7bedd..e1db4653f06b696dc27526841ded2fbc3c83de31 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");