[libvirt] [PATCH 0/3] conf: reject '/' in object names
An embedded '/' in object names doesn't really work for any of our stateful drivers, so let's explicitly reject it. https://bugzilla.redhat.com/show_bug.cgi?id=639923 https://bugzilla.redhat.com/show_bug.cgi?id=787604 Cole Robinson (3): conf: domain: reject name containing '/' conf: network: reject name containing '/' conf: storage: pool: reject name containing '/' src/conf/domain_conf.c | 8 ++++++++ src/conf/domain_conf.h | 1 + src/conf/network_conf.c | 6 ++++++ src/conf/storage_conf.c | 6 ++++++ src/openvz/openvz_driver.c | 5 +++-- src/phyp/phyp_driver.c | 1 + src/vbox/vbox_common.c | 1 + src/vmx/vmx.c | 3 ++- src/xenapi/xenapi_driver.c | 1 + tests/genericxml2xmlindata/generic-name-slash-fail.xml | 17 +++++++++++++++++ tests/genericxml2xmltest.c | 3 +++ 11 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 tests/genericxml2xmlindata/generic-name-slash-fail.xml -- 2.7.4
Trying to define a domain name containing an embedded '/' will immediately fail when trying to write the XML to disk for our stateful drivers. This patch explicitly rejects names containing a '/', and provides an xmlopt feature for drivers to avoid this validation check, which is enabled in every non-stateful driver that already has xmlopt handling wired up. (Technically this could reject a previously accepted vmname like '/foo', however at least for the qemu driver that falls over later when starting qemu) https://bugzilla.redhat.com/show_bug.cgi?id=639923 --- src/conf/domain_conf.c | 8 ++++++++ src/conf/domain_conf.h | 1 + src/openvz/openvz_driver.c | 5 +++-- src/phyp/phyp_driver.c | 1 + src/vbox/vbox_common.c | 1 + src/vmx/vmx.c | 3 ++- src/xenapi/xenapi_driver.c | 1 + tests/genericxml2xmlindata/generic-name-slash-fail.xml | 17 +++++++++++++++++ tests/genericxml2xmltest.c | 3 +++ 9 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 tests/genericxml2xmlindata/generic-name-slash-fail.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index db567f5..113c9a9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4284,6 +4284,14 @@ virDomainDefPostParseCheckFeatures(virDomainDefPtr def, if (UNSUPPORTED(VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN)) virDomainDefRemoveOfflineVcpuPin(def); + if (UNSUPPORTED(VIR_DOMAIN_DEF_FEATURE_NAME_SLASH)) { + if (def->name && strchr(def->name, '/')) { + virReportError(VIR_ERR_XML_ERROR, + _("name %s cannot contain '/'"), def->name); + return -1; + } + } + return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fd540ed..eb2127c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2442,6 +2442,7 @@ typedef enum { VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI = (1 << 0), VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG = (1 << 1), VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN = (1 << 2), + VIR_DOMAIN_DEF_FEATURE_NAME_SLASH = (1 << 3), } virDomainDefFeatures; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index e154a0f..a7474ff 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -128,8 +128,9 @@ openvzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, virDomainDefParserConfig openvzDomainDefParserConfig = { - .domainPostParseCallback = openvzDomainDefPostParse, - .devicesPostParseCallback = openvzDomainDeviceDefPostParse, + .domainPostParseCallback = openvzDomainDefPostParse, + .devicesPostParseCallback = openvzDomainDeviceDefPostParse, + .features = VIR_DOMAIN_DEF_FEATURE_NAME_SLASH, }; diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 55a63e7..da87686 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1117,6 +1117,7 @@ phypDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, virDomainDefParserConfig virPhypDriverDomainDefParserConfig = { .devicesPostParseCallback = phypDomainDeviceDefPostParse, .domainPostParseCallback = phypDomainDefPostParse, + .features = VIR_DOMAIN_DEF_FEATURE_NAME_SLASH, }; diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 82286a8..abfb30a 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -273,6 +273,7 @@ static virDomainDefParserConfig vboxDomainDefParserConfig = { .macPrefix = { 0x08, 0x00, 0x27 }, .devicesPostParseCallback = vboxDomainDeviceDefPostParse, .domainPostParseCallback = vboxDomainDefPostParse, + .features = VIR_DOMAIN_DEF_FEATURE_NAME_SLASH, }; static virDomainXMLOptionPtr diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 8c4b4bb..5e57c39 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -547,7 +547,8 @@ static virDomainDefParserConfig virVMXDomainDefParserConfig = { .macPrefix = {0x00, 0x0c, 0x29}, .devicesPostParseCallback = virVMXDomainDevicesDefPostParse, .domainPostParseCallback = virVMXDomainDefPostParse, - .features = VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI, + .features = (VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI | + VIR_DOMAIN_DEF_FEATURE_NAME_SLASH), }; static void diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index a75a4f7..97a1ada 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -88,6 +88,7 @@ xenapiDomainDefPostParse(virDomainDefPtr def, virDomainDefParserConfig xenapiDomainDefParserConfig = { .devicesPostParseCallback = xenapiDomainDeviceDefPostParse, .domainPostParseCallback = xenapiDomainDefPostParse, + .features = VIR_DOMAIN_DEF_FEATURE_NAME_SLASH, }; diff --git a/tests/genericxml2xmlindata/generic-name-slash-fail.xml b/tests/genericxml2xmlindata/generic-name-slash-fail.xml new file mode 100644 index 0000000..4cdb834 --- /dev/null +++ b/tests/genericxml2xmlindata/generic-name-slash-fail.xml @@ -0,0 +1,17 @@ +<domain type='qemu'> + <name>foo/bar</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + </devices> +</domain> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index 05563fb..70a5203 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -81,6 +81,9 @@ mymain(void) DO_TEST_FULL("graphics-listen-back-compat-mismatch", 0, false, TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); + DO_TEST_FULL("name-slash-parse", 0, false, + TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); + virObjectUnref(caps); virObjectUnref(xmlopt); -- 2.7.4
Trying to define a network name containing an embedded '/' will immediately fail when trying to write the XML to disk. This patch explicitly rejects names containing a '/' Besides the network bridge driver, the only other network implementation is a very thin one for virtualbox, which seems to use the network name as a host interface name, which won't accept '/' anyways, so I think this is fine to do unconitionally. https://bugzilla.redhat.com/show_bug.cgi?id=787604 --- src/conf/network_conf.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index e6915ff..1f680d7 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2070,6 +2070,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; } + if (strchr(def->name, '/')) { + virReportError(VIR_ERR_XML_ERROR, + _("name %s cannot contain '/'"), def->name); + goto error; + } + /* Extract network uuid */ tmp = virXPathString("string(./uuid[1])", ctxt); if (!tmp) { -- 2.7.4
Trying to define a pool name containing an embedded '/' will immediately fail when trying to write the XML to disk. This patch explicitly rejects names containing a '/' Besides our stateful driver, there are two other storage impls: esx and phyp. esx doesn't support pool creation, so this should doesn't apply. phyp does support pool creation, and the name is passed to the 'mksp' tool, which google doesn't reveal whether it accepts '/' or not. IMO the likeliness of this impacting any users is near zero --- src/conf/storage_conf.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index daf8f99..6fa9695 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -853,6 +853,12 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; } + if (strchr(ret->name, '/')) { + virReportError(VIR_ERR_XML_ERROR, + _("name %s cannot contain '/'"), ret->name); + goto error; + } + uuid = virXPathString("string(./uuid)", ctxt); if (uuid == NULL) { if (virUUIDGenerate(ret->uuid) < 0) { -- 2.7.4
On 04/26/2016 03:21 PM, Cole Robinson wrote:
An embedded '/' in object names doesn't really work for any of our stateful drivers, so let's explicitly reject it.
https://bugzilla.redhat.com/show_bug.cgi?id=639923 https://bugzilla.redhat.com/show_bug.cgi?id=787604
Was there any discussion I may have missed about why we shouldn't escape characters like this in the name rather than just forbidding them? This doesn't prevent us doing that in the future if we want though, and it does provide a useful error message where there was previous semi-mystery, so ACK to the series (but do we really need the feature flag for domains? Seems like a bit of overkill.)
Cole Robinson (3): conf: domain: reject name containing '/' conf: network: reject name containing '/' conf: storage: pool: reject name containing '/'
src/conf/domain_conf.c | 8 ++++++++ src/conf/domain_conf.h | 1 + src/conf/network_conf.c | 6 ++++++ src/conf/storage_conf.c | 6 ++++++ src/openvz/openvz_driver.c | 5 +++-- src/phyp/phyp_driver.c | 1 + src/vbox/vbox_common.c | 1 + src/vmx/vmx.c | 3 ++- src/xenapi/xenapi_driver.c | 1 + tests/genericxml2xmlindata/generic-name-slash-fail.xml | 17 +++++++++++++++++ tests/genericxml2xmltest.c | 3 +++ 11 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 tests/genericxml2xmlindata/generic-name-slash-fail.xml
On 04/26/2016 04:26 PM, Laine Stump wrote:
On 04/26/2016 03:21 PM, Cole Robinson wrote:
An embedded '/' in object names doesn't really work for any of our stateful drivers, so let's explicitly reject it.
https://bugzilla.redhat.com/show_bug.cgi?id=639923 https://bugzilla.redhat.com/show_bug.cgi?id=787604
Was there any discussion I may have missed about why we shouldn't escape characters like this in the name rather than just forbidding them?
No discussion besides some old comments in those bugs. But how do you escape '/' for a unix filename? It's not really possible... we'd probably need some kind of URI escape sequence to make it work, which just opens up another world of pain making sure that the filename we put on disk doesn't collide with any legitimately named VM. Crazyness ensues
This doesn't prevent us doing that in the future if we want though, and it does provide a useful error message where there was previous semi-mystery, so ACK to the series (but do we really need the feature flag for domains? Seems like a bit of overkill.)
It's possible that existing VMs in non-stateful drivers already have a '/' in the name, and we shouldn't accidentally reject them. Googling indicates that vmware allows / in .vmx names at least. It may not matter in practice but I wanted to be conservative. Thanks for the review, I'll wait a couple days to see if anyone else wants to comment before pushing - Cole
On 04/26/2016 04:33 PM, Cole Robinson wrote:
On 04/26/2016 04:26 PM, Laine Stump wrote:
On 04/26/2016 03:21 PM, Cole Robinson wrote:
An embedded '/' in object names doesn't really work for any of our stateful drivers, so let's explicitly reject it.
https://bugzilla.redhat.com/show_bug.cgi?id=639923 https://bugzilla.redhat.com/show_bug.cgi?id=787604
Was there any discussion I may have missed about why we shouldn't escape characters like this in the name rather than just forbidding them?
No discussion besides some old comments in those bugs. But how do you escape '/' for a unix filename? It's not really possible... we'd probably need some kind of URI escape sequence to make it work, which just opens up another world of pain making sure that the filename we put on disk doesn't collide with any legitimately named VM. Crazyness ensues
This doesn't prevent us doing that in the future if we want though, and it does provide a useful error message where there was previous semi-mystery, so ACK to the series (but do we really need the feature flag for domains? Seems like a bit of overkill.)
It's possible that existing VMs in non-stateful drivers already have a '/' in the name, and we shouldn't accidentally reject them. Googling indicates that vmware allows / in .vmx names at least. It may not matter in practice but I wanted to be conservative.
Thanks for the review, I'll wait a couple days to see if anyone else wants to comment before pushing
couple days == until after the release - Cole
participants (2)
-
Cole Robinson -
Laine Stump