On 11/21/2013 04:57 PM, Eric Blake wrote:
> volume defs during pool refresh and not from xml reparsing; but you do
> have a point that on CreateVol, we need to be careful to ignore any
> input key and use the one that the storage volume would have generated
> normally; likewise, we must ensure we don't leak memory.
>
> I'm still checking...
>
I verified that the only user XML paths are virStorageVolCreateXML and
virStorageVolCreateXMLFrom; and that both paths filter through the
storage backend->createVol() before ever attempting to use the key in
other locations. So all that remains is auditing each backend's createVol:
backend_fs.c always reset key before setting it during createVol;
nothing to change.
backend_disk.c, on the other hand, enters some code that behaves
differently if key is NULL (if so, image is newly created, populate the
key) compared to non-NULL (look for matching key - even if user's key
was wrong!)
so I didn't even bother checking backend_logical, backend_rbd, or
backend_sheepdog. Instead:
>>
>> ACK if you drop this hunk or fix virStorageBackendDiskCreateVol.
>
> I'll reply again with what I squash in after auditing all paths where
> the user passes in volume XML to make sure we aren't leaking or using
> the wrong key.
I squashed in this, which avoids the problem for all storage backends.
diff --git i/src/storage/storage_driver.c w/src/storage/storage_driver.c
index b3f0871..3b4715a 100644
--- i/src/storage/storage_driver.c
+++ w/src/storage/storage_driver.c
@@ -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;
}
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list