[libvirt] [PATCH 00/11] Add checks to ensure a name isn't all whitespace
https://bugzilla.redhat.com/show_bug.cgi?id=1107420 As unusual as the case may be, having a name of all white space has been allowed. This leads to the obvious problem of how a future usage would be able to "utilize" that resource since it's not "simple" to determine what combination of spaces and tabs are being used for the name. So add code to various drivers in order to inhibit this for those resources that would consider using a name for some sort of lookup type reference. For a majority of drivers, the schema validation also allowed the name; however, a couple (secrets and nwfilters) the virt-xml-validate would fail. Still this wouldn't necessarily stop someone - just slow them down. So for those the test xml file must use the *-invalid.xml type syntax to inhibit virschematest from looking at the schema. Similar validation for node devices and interfaces is not necessary since those are referencing system named elements which wouldn't have the oddly named resource. Also, while it could be checked, avoid the check for Storage Volumes as there's no way to "inhibit" in the parse logic algorithm. This is not for 4.6.0, but figured I'd get it on list for consideration for 4.7.0. Yes, it's quite unusual and one has to wonder who thinks up this type of situation, but still in the long run we should attempt to avoid it anyway. I've only added the extra checking for qemu and lxc domains, leaving others to those maintainers to choose to utilize. John Ferlan (11): conf: Add @flags to Storage Pool Def processing conf: Disallow new storage pools to use all white space as name conf: Add @flags to Network Def processing conf: Disallow new networks to use all white space as name conf: Disallow new qemu,lxc domains to use all white space as name conf: Split and rename secretXMLParseNode conf: Add @flags to Secret Def processing conf: Disallow new secrets to use all white space as usage id conf: Add @flags to NWFilter Def processing conf: Disallow new nwfilters to use all white space as name conf: Disallow new snapshots to use all white space as name src/conf/domain_conf.c | 11 ++- src/conf/domain_conf.h | 4 + src/conf/network_conf.c | 34 +++++--- src/conf/network_conf.h | 19 ++++- src/conf/nwfilter_conf.c | 32 +++++--- src/conf/nwfilter_conf.h | 16 +++- src/conf/secret_conf.c | 79 +++++++++++++------ src/conf/secret_conf.h | 16 +++- src/conf/snapshot_conf.c | 7 ++ src/conf/snapshot_conf.h | 1 + src/conf/storage_conf.c | 32 +++++--- src/conf/storage_conf.h | 19 ++++- src/conf/virnetworkobj.c | 4 +- src/conf/virnwfilterobj.c | 2 +- src/conf/virsecretobj.c | 2 +- src/conf/virstorageobj.c | 4 +- src/esx/esx_network_driver.c | 2 +- src/lxc/lxc_driver.c | 6 +- src/network/bridge_driver.c | 6 +- src/nwfilter/nwfilter_driver.c | 3 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 9 ++- src/qemu/qemu_process.c | 2 +- src/secret/secret_driver.c | 3 +- src/storage/storage_driver.c | 6 +- src/test/test_driver.c | 12 +-- src/vbox/vbox_network.c | 2 +- .../name_whitespace.xml | 3 + tests/domainsnapshotxml2xmltest.c | 38 ++++++--- tests/networkxml2conftest.c | 2 +- tests/networkxml2firewalltest.c | 2 +- .../network-whitespace-name.xml | 6 ++ tests/networkxml2xmltest.c | 4 +- tests/networkxml2xmlupdatetest.c | 2 +- tests/nwfilterxml2firewalltest.c | 2 +- .../name-whitespace-invalid.xml | 4 + tests/nwfilterxml2xmltest.c | 7 +- tests/qemuxml2argvdata/name-whitespace.xml | 29 +++++++ tests/qemuxml2argvtest.c | 5 +- .../usage-whitespace-invalid.xml | 7 ++ tests/secretxml2xmltest.c | 30 +++++-- tests/storagebackendsheepdogtest.c | 4 +- .../pool-dir-whitespace-name.xml | 18 +++++ tests/storagepoolxml2xmltest.c | 45 ++++++++--- tests/storagevolxml2argvtest.c | 4 +- tests/storagevolxml2xmltest.c | 2 +- 46 files changed, 418 insertions(+), 131 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlin/name_whitespace.xml create mode 100644 tests/networkxml2xmlin/network-whitespace-name.xml create mode 100644 tests/nwfilterxml2xmlin/name-whitespace-invalid.xml create mode 100644 tests/qemuxml2argvdata/name-whitespace.xml create mode 100644 tests/secretxml2xmlin/usage-whitespace-invalid.xml create mode 100644 tests/storagepoolxml2xmlin/pool-dir-whitespace-name.xml -- 2.17.1
Add a @flags argument when parsing the storage pool definition via the virStoragePoolDefParse{XML|Node|String|File} API's as this will allow us to in the future make parsing decisions based on the @flags. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/storage_conf.c | 25 ++++++++++++++++--------- src/conf/storage_conf.h | 12 ++++++++---- src/conf/virstorageobj.c | 4 ++-- src/phyp/phyp_driver.c | 2 +- src/storage/storage_driver.c | 4 ++-- src/test/test_driver.c | 6 +++--- tests/storagebackendsheepdogtest.c | 4 ++-- tests/storagepoolxml2xmltest.c | 2 +- tests/storagevolxml2argvtest.c | 4 ++-- tests/storagevolxml2xmltest.c | 2 +- 11 files changed, 39 insertions(+), 28 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eff8af20e7..42b55f01d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30183,7 +30183,7 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def) if (!(poolxml = virStoragePoolGetXMLDesc(pool, 0))) goto cleanup; - if (!(pooldef = virStoragePoolDefParseString(poolxml))) + if (!(pooldef = virStoragePoolDefParseString(poolxml, 0))) goto cleanup; def->src->srcpool->pooltype = pooldef->type; diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 5036ab9ef8..e5d35cd254 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -671,7 +671,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, virStoragePoolDefPtr -virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) +virStoragePoolDefParseXML(xmlXPathContextPtr ctxt, + unsigned int flags) { virStoragePoolOptionsPtr options; virStoragePoolDefPtr ret; @@ -680,6 +681,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) char *uuid = NULL; char *target_path = NULL; + virCheckFlags(0, NULL); + if (VIR_ALLOC(ret) < 0) return NULL; @@ -818,7 +821,8 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) virStoragePoolDefPtr virStoragePoolDefParseNode(xmlDocPtr xml, - xmlNodePtr root) + xmlNodePtr root, + unsigned int flags) { xmlXPathContextPtr ctxt = NULL; virStoragePoolDefPtr def = NULL; @@ -838,7 +842,7 @@ virStoragePoolDefParseNode(xmlDocPtr xml, } ctxt->node = root; - def = virStoragePoolDefParseXML(ctxt); + def = virStoragePoolDefParseXML(ctxt, flags); cleanup: xmlXPathFreeContext(ctxt); return def; @@ -847,13 +851,14 @@ virStoragePoolDefParseNode(xmlDocPtr xml, static virStoragePoolDefPtr virStoragePoolDefParse(const char *xmlStr, - const char *filename) + const char *filename, + unsigned int flags) { virStoragePoolDefPtr ret = NULL; xmlDocPtr xml; if ((xml = virXMLParse(filename, xmlStr, _("(storage_pool_definition)")))) { - ret = virStoragePoolDefParseNode(xml, xmlDocGetRootElement(xml)); + ret = virStoragePoolDefParseNode(xml, xmlDocGetRootElement(xml), flags); xmlFreeDoc(xml); } @@ -862,16 +867,18 @@ virStoragePoolDefParse(const char *xmlStr, virStoragePoolDefPtr -virStoragePoolDefParseString(const char *xmlStr) +virStoragePoolDefParseString(const char *xmlStr, + unsigned int flags) { - return virStoragePoolDefParse(xmlStr, NULL); + return virStoragePoolDefParse(xmlStr, NULL, flags); } virStoragePoolDefPtr -virStoragePoolDefParseFile(const char *filename) +virStoragePoolDefParseFile(const char *filename, + unsigned int flags) { - return virStoragePoolDefParse(NULL, filename); + return virStoragePoolDefParse(NULL, filename, flags); } diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 15dfd8becf..d6886ad6ca 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -236,17 +236,21 @@ struct _virStoragePoolSourceList { }; virStoragePoolDefPtr -virStoragePoolDefParseXML(xmlXPathContextPtr ctxt); +virStoragePoolDefParseXML(xmlXPathContextPtr ctxt, + unsigned int flags); virStoragePoolDefPtr -virStoragePoolDefParseString(const char *xml); +virStoragePoolDefParseString(const char *xml, + unsigned int flags); virStoragePoolDefPtr -virStoragePoolDefParseFile(const char *filename); +virStoragePoolDefParseFile(const char *filename, + unsigned int flags); virStoragePoolDefPtr virStoragePoolDefParseNode(xmlDocPtr xml, - xmlNodePtr root); + xmlNodePtr root, + unsigned int flags); char * virStoragePoolDefFormat(virStoragePoolDefPtr def); diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index e66b2ebfb2..47209fae96 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1110,7 +1110,7 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def; virStoragePoolObjPtr obj; - if (!(def = virStoragePoolDefParseFile(path))) + if (!(def = virStoragePoolDefParseFile(path, 0))) return NULL; if (!virFileMatchesNameSuffix(file, def->name, ".xml")) { @@ -1172,7 +1172,7 @@ virStoragePoolObjLoadState(virStoragePoolObjListPtr pools, } ctxt->node = node; - if (!(def = virStoragePoolDefParseXML(ctxt))) + if (!(def = virStoragePoolDefParseXML(ctxt, 0))) goto error; if (STRNEQ(name, def->name)) { diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index d78de83231..99d5dedd81 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2635,7 +2635,7 @@ phypStoragePoolCreateXML(virConnectPtr conn, virStoragePoolPtr dup_sp = NULL; virStoragePoolPtr sp = NULL; - if (!(def = virStoragePoolDefParseString(xml))) + if (!(def = virStoragePoolDefParseString(xml, 0))) goto err; /* checking if this name already exists on this system */ diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 8070d159ea..491c4fab9b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -699,7 +699,7 @@ storagePoolCreateXML(virConnectPtr conn, VIR_EXCLUSIVE_FLAGS_RET(VIR_STORAGE_POOL_BUILD_OVERWRITE, VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, NULL); - if (!(newDef = virStoragePoolDefParseString(xml))) + if (!(newDef = virStoragePoolDefParseString(xml, 0))) goto cleanup; if (virStoragePoolCreateXMLEnsureACL(conn, newDef) < 0) @@ -790,7 +790,7 @@ storagePoolDefineXML(virConnectPtr conn, virCheckFlags(0, NULL); - if (!(newDef = virStoragePoolDefParseString(xml))) + if (!(newDef = virStoragePoolDefParseString(xml, 0))) goto cleanup; if (virXMLCheckIllegalChars("name", newDef->name, "\n") < 0) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index dfca95c981..1a42a4f74b 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1104,7 +1104,7 @@ testParseStorage(testDriverPtr privconn, if (!node) goto error; - def = virStoragePoolDefParseNode(ctxt->doc, node); + def = virStoragePoolDefParseNode(ctxt->doc, node, 0); if (!def) goto error; @@ -4519,7 +4519,7 @@ testStoragePoolCreateXML(virConnectPtr conn, virCheckFlags(0, NULL); testDriverLock(privconn); - if (!(newDef = virStoragePoolDefParseString(xml))) + if (!(newDef = virStoragePoolDefParseString(xml, 0))) goto cleanup; if (virStoragePoolObjIsDuplicate(privconn->pools, newDef, true) < 0) @@ -4589,7 +4589,7 @@ testStoragePoolDefineXML(virConnectPtr conn, virCheckFlags(0, NULL); testDriverLock(privconn); - if (!(newDef = virStoragePoolDefParseString(xml))) + if (!(newDef = virStoragePoolDefParseString(xml, 0))) goto cleanup; newDef->capacity = defaultPoolCap; diff --git a/tests/storagebackendsheepdogtest.c b/tests/storagebackendsheepdogtest.c index cc7b163241..c9b80127b8 100644 --- a/tests/storagebackendsheepdogtest.c +++ b/tests/storagebackendsheepdogtest.c @@ -65,7 +65,7 @@ test_node_info_parser(const void *opaque) char *output = NULL; virStoragePoolDefPtr pool = NULL; - if (!(pool = virStoragePoolDefParseFile(data->poolxml))) + if (!(pool = virStoragePoolDefParseFile(data->poolxml, 0))) goto cleanup; if (VIR_STRDUP(output, test.output) < 0) @@ -100,7 +100,7 @@ test_vdi_list_parser(const void *opaque) virStoragePoolDefPtr pool = NULL; virStorageVolDefPtr vol = NULL; - if (!(pool = virStoragePoolDefParseFile(data->poolxml))) + if (!(pool = virStoragePoolDefParseFile(data->poolxml, 0))) goto cleanup; if (!(vol = virStorageVolDefParseFile(pool, data->volxml, 0))) diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 974f0afe39..84f2bfb9ec 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -23,7 +23,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml) int ret = -1; virStoragePoolDefPtr dev = NULL; - if (!(dev = virStoragePoolDefParseFile(inxml))) + if (!(dev = virStoragePoolDefParseFile(inxml, 0))) goto fail; if (!(actual = virStoragePoolDefFormat(dev))) diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c index b795f83aee..aa55e4dbad 100644 --- a/tests/storagevolxml2argvtest.c +++ b/tests/storagevolxml2argvtest.c @@ -53,7 +53,7 @@ testCompareXMLToArgvFiles(bool shouldFail, virStoragePoolDefPtr inputpool = NULL; virStoragePoolObjPtr obj = NULL; - if (!(def = virStoragePoolDefParseFile(poolxml))) + if (!(def = virStoragePoolDefParseFile(poolxml, 0))) goto cleanup; if (!(obj = virStoragePoolObjNew())) { @@ -63,7 +63,7 @@ testCompareXMLToArgvFiles(bool shouldFail, virStoragePoolObjSetDef(obj, def); if (inputpoolxml) { - if (!(inputpool = virStoragePoolDefParseFile(inputpoolxml))) + if (!(inputpool = virStoragePoolDefParseFile(inputpoolxml, 0))) goto cleanup; } diff --git a/tests/storagevolxml2xmltest.c b/tests/storagevolxml2xmltest.c index 7bac4974ae..0ff25451bc 100644 --- a/tests/storagevolxml2xmltest.c +++ b/tests/storagevolxml2xmltest.c @@ -25,7 +25,7 @@ testCompareXMLToXMLFiles(const char *poolxml, const char *inxml, virStoragePoolDefPtr pool = NULL; virStorageVolDefPtr dev = NULL; - if (!(pool = virStoragePoolDefParseFile(poolxml))) + if (!(pool = virStoragePoolDefParseFile(poolxml, 0))) goto fail; if (!(dev = virStorageVolDefParseFile(pool, inxml, flags))) -- 2.17.1
https://bugzilla.redhat.com/show_bug.cgi?id=1107420 Add a new define/create flag VIR_STORAGE_POOL_DEF_PARSE_VALIDATE_NAME to disallow new storage pools to be defined/created using a name comprised entirely of spaces. Alter the storagepoolxml2xmltest to add a parse failure scenario and a test in order to prove the failure occurs. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 9 +++- src/conf/storage_conf.h | 7 +++ src/storage/storage_driver.c | 6 ++- .../pool-dir-whitespace-name.xml | 18 ++++++++ tests/storagepoolxml2xmltest.c | 45 +++++++++++++++---- 5 files changed, 73 insertions(+), 12 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-dir-whitespace-name.xml diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e5d35cd254..e8bbe4ea54 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -681,7 +681,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt, char *uuid = NULL; char *target_path = NULL; - virCheckFlags(0, NULL); + virCheckFlags(VIR_STORAGE_POOL_DEF_PARSE_VALIDATE_NAME, NULL); if (VIR_ALLOC(ret) < 0) return NULL; @@ -729,6 +729,13 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt, goto error; } + if ((flags & VIR_STORAGE_POOL_DEF_PARSE_VALIDATE_NAME) && + virStringIsEmpty(ret->name)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("name must contain at least one non blank character")); + goto error; + } + uuid = virXPathString("string(./uuid)", ctxt); if (uuid == NULL) { if (virUUIDGenerate(ret->uuid) < 0) { diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index d6886ad6ca..31851643e9 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -235,6 +235,13 @@ struct _virStoragePoolSourceList { virStoragePoolSourcePtr sources; }; +typedef enum { + /* Perform extra name validation on new storage pool names which + * will cause failure to parse the XML. Initially just that a + * name cannot be all white space. */ + VIR_STORAGE_POOL_DEF_PARSE_VALIDATE_NAME = 1 << 0, +} virStoragePoolDefParseFlags; + virStoragePoolDefPtr virStoragePoolDefParseXML(xmlXPathContextPtr ctxt, unsigned int flags); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 491c4fab9b..8d7a7b399c 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -690,6 +690,7 @@ storagePoolCreateXML(virConnectPtr conn, virStorageBackendPtr backend; virObjectEventPtr event = NULL; char *stateFile = NULL; + unsigned int parse_flags = VIR_STORAGE_POOL_DEF_PARSE_VALIDATE_NAME; unsigned int build_flags = 0; virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD | @@ -699,7 +700,7 @@ storagePoolCreateXML(virConnectPtr conn, VIR_EXCLUSIVE_FLAGS_RET(VIR_STORAGE_POOL_BUILD_OVERWRITE, VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, NULL); - if (!(newDef = virStoragePoolDefParseString(xml, 0))) + if (!(newDef = virStoragePoolDefParseString(xml, parse_flags))) goto cleanup; if (virStoragePoolCreateXMLEnsureACL(conn, newDef) < 0) @@ -787,10 +788,11 @@ storagePoolDefineXML(virConnectPtr conn, virStoragePoolDefPtr def; virStoragePoolPtr pool = NULL; virObjectEventPtr event = NULL; + unsigned int parse_flags = VIR_STORAGE_POOL_DEF_PARSE_VALIDATE_NAME; virCheckFlags(0, NULL); - if (!(newDef = virStoragePoolDefParseString(xml, 0))) + if (!(newDef = virStoragePoolDefParseString(xml, parse_flags))) goto cleanup; if (virXMLCheckIllegalChars("name", newDef->name, "\n") < 0) diff --git a/tests/storagepoolxml2xmlin/pool-dir-whitespace-name.xml b/tests/storagepoolxml2xmlin/pool-dir-whitespace-name.xml new file mode 100644 index 0000000000..024505df03 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-dir-whitespace-name.xml @@ -0,0 +1,18 @@ +<pool type='dir'> + <name> </name> + <uuid>70a7eb15-6c34-ee9c-bf57-69e8e5ff3fb2</uuid> + <capacity>0</capacity> + <allocation>0</allocation> + <available>0</available> + <source> + </source> + <target> + <path>///var/////lib/libvirt/images//</path> + <permissions> + <mode>0700</mode> + <owner>-1</owner> + <group>-1</group> + <label>some_label_t</label> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 84f2bfb9ec..7893e79da8 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -17,14 +17,24 @@ #define VIR_FROM_THIS VIR_FROM_NONE static int -testCompareXMLToXMLFiles(const char *inxml, const char *outxml) +testCompareXMLToXMLFiles(const char *inxml, + const char *outxml, + bool expect_parse_fail) { char *actual = NULL; int ret = -1; virStoragePoolDefPtr dev = NULL; - - if (!(dev = virStoragePoolDefParseFile(inxml, 0))) + unsigned int parse_flags = VIR_STORAGE_POOL_DEF_PARSE_VALIDATE_NAME; + + if (!(dev = virStoragePoolDefParseFile(inxml, parse_flags))) { + if (expect_parse_fail) { + VIR_TEST_DEBUG("Got expected parse failure msg='%s'", + virGetLastErrorMessage()); + virResetLastError(); + ret = 0; + } goto fail; + } if (!(actual = virStoragePoolDefFormat(dev))) goto fail; @@ -40,21 +50,28 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml) return ret; } + +typedef struct test_params { + const char *name; + bool expect_parse_fail; +} test_params; + static int testCompareXMLToXMLHelper(const void *data) { int result = -1; char *inxml = NULL; char *outxml = NULL; + const test_params *tp = data; if (virAsprintf(&inxml, "%s/storagepoolxml2xmlin/%s.xml", - abs_srcdir, (const char*)data) < 0 || + abs_srcdir, tp->name) < 0 || virAsprintf(&outxml, "%s/storagepoolxml2xmlout/%s.xml", - abs_srcdir, (const char*)data) < 0) { + abs_srcdir, tp->name) < 0) { goto cleanup; } - result = testCompareXMLToXMLFiles(inxml, outxml); + result = testCompareXMLToXMLFiles(inxml, outxml, tp->expect_parse_fail); cleanup: VIR_FREE(inxml); @@ -68,13 +85,23 @@ mymain(void) { int ret = 0; +#define DO_TEST_FULL(name, expect_parse_fail) \ + do { \ + test_params tp = {name, expect_parse_fail}; \ + if (virTestRun("Storage Pool XML-2-XML " name, \ + testCompareXMLToXMLHelper, &tp) < 0) \ + ret = -1; \ + } while (0) + #define DO_TEST(name) \ - if (virTestRun("Storage Pool XML-2-XML " name, \ - testCompareXMLToXMLHelper, (name)) < 0) \ - ret = -1 + DO_TEST_FULL(name, false); + +#define DO_TEST_PARSE_FAIL(name) \ + DO_TEST_FULL(name, true); DO_TEST("pool-dir"); DO_TEST("pool-dir-naming"); + DO_TEST_PARSE_FAIL("pool-dir-whitespace-name"); DO_TEST("pool-fs"); DO_TEST("pool-logical"); DO_TEST("pool-logical-nopath"); -- 2.17.1
Add a @flags argument when parsing the network definition via the virNetworkDefParse{XML|Node|String|File} API's as this will allow us to in the future make parsing decisions based on the @flags. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/network_conf.c | 27 +++++++++++++++++---------- src/conf/network_conf.h | 12 ++++++++---- src/conf/virnetworkobj.c | 4 ++-- src/esx/esx_network_driver.c | 2 +- src/network/bridge_driver.c | 4 ++-- src/qemu/qemu_process.c | 2 +- src/test/test_driver.c | 6 +++--- src/vbox/vbox_network.c | 2 +- tests/networkxml2conftest.c | 2 +- tests/networkxml2firewalltest.c | 2 +- tests/networkxml2xmltest.c | 2 +- tests/networkxml2xmlupdatetest.c | 2 +- 13 files changed, 40 insertions(+), 29 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 42b55f01d4..705575fe92 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30004,7 +30004,7 @@ virDomainNetResolveActualType(virDomainNetDefPtr iface) if (!(xml = virNetworkGetXMLDesc(net, 0))) goto cleanup; - if (!(def = virNetworkDefParseString(xml))) + if (!(def = virNetworkDefParseString(xml, 0))) goto cleanup; switch ((virNetworkForwardType) def->forward.type) { diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c08456ba96..34d132e506 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -259,7 +259,7 @@ virNetworkDefCopy(virNetworkDefPtr def, unsigned int flags) /* deep copy with a format/parse cycle */ if (!(xml = virNetworkDefFormat(def, flags))) goto cleanup; - newDef = virNetworkDefParseString(xml); + newDef = virNetworkDefParseString(xml, 0); cleanup: VIR_FREE(xml); return newDef; @@ -1584,7 +1584,8 @@ virNetworkForwardDefParseXML(const char *networkName, virNetworkDefPtr -virNetworkDefParseXML(xmlXPathContextPtr ctxt) +virNetworkDefParseXML(xmlXPathContextPtr ctxt, + unsigned int flags) { virNetworkDefPtr def; char *tmp = NULL; @@ -1603,6 +1604,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) xmlNodePtr vlanNode; xmlNodePtr metadataNode = NULL; + virCheckFlags(0, NULL); + if (VIR_ALLOC(def) < 0) return NULL; @@ -2016,14 +2019,15 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) static virNetworkDefPtr virNetworkDefParse(const char *xmlStr, - const char *filename) + const char *filename, + unsigned int flags) { xmlDocPtr xml; virNetworkDefPtr def = NULL; int keepBlanksDefault = xmlKeepBlanksDefault(0); if ((xml = virXMLParse(filename, xmlStr, _("(network_definition)")))) { - def = virNetworkDefParseNode(xml, xmlDocGetRootElement(xml)); + def = virNetworkDefParseNode(xml, xmlDocGetRootElement(xml), flags); xmlFreeDoc(xml); } @@ -2033,22 +2037,25 @@ virNetworkDefParse(const char *xmlStr, virNetworkDefPtr -virNetworkDefParseString(const char *xmlStr) +virNetworkDefParseString(const char *xmlStr, + unsigned int flags) { - return virNetworkDefParse(xmlStr, NULL); + return virNetworkDefParse(xmlStr, NULL, flags); } virNetworkDefPtr -virNetworkDefParseFile(const char *filename) +virNetworkDefParseFile(const char *filename, + unsigned int flags) { - return virNetworkDefParse(NULL, filename); + return virNetworkDefParse(NULL, filename, flags); } virNetworkDefPtr virNetworkDefParseNode(xmlDocPtr xml, - xmlNodePtr root) + xmlNodePtr root, + unsigned int flags) { xmlXPathContextPtr ctxt = NULL; virNetworkDefPtr def = NULL; @@ -2068,7 +2075,7 @@ virNetworkDefParseNode(xmlDocPtr xml, } ctxt->node = root; - def = virNetworkDefParseXML(ctxt); + def = virNetworkDefParseXML(ctxt, flags); cleanup: xmlXPathFreeContext(ctxt); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 54c8ed1c4c..6373b783c6 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -294,17 +294,21 @@ virNetworkDefPtr virNetworkDefCopy(virNetworkDefPtr def, unsigned int flags); virNetworkDefPtr -virNetworkDefParseXML(xmlXPathContextPtr ctxt); +virNetworkDefParseXML(xmlXPathContextPtr ctxt, + unsigned int flags); virNetworkDefPtr -virNetworkDefParseString(const char *xmlStr); +virNetworkDefParseString(const char *xmlStr, + unsigned int flags); virNetworkDefPtr -virNetworkDefParseFile(const char *filename); +virNetworkDefParseFile(const char *filename, + unsigned int flags); virNetworkDefPtr virNetworkDefParseNode(xmlDocPtr xml, - xmlNodePtr root); + xmlNodePtr root, + unsigned int flags); char * virNetworkDefFormat(const virNetworkDef *def, diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index b13e5a7b03..d7357007d3 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -885,7 +885,7 @@ virNetworkLoadState(virNetworkObjListPtr nets, /* parse the definition first */ ctxt->node = node; - if (!(def = virNetworkDefParseXML(ctxt))) + if (!(def = virNetworkDefParseXML(ctxt, 0))) goto error; if (STRNEQ(name, def->name)) { @@ -998,7 +998,7 @@ virNetworkLoadConfig(virNetworkObjListPtr nets, if ((autostart = virFileLinkPointsTo(autostartLink, configFile)) < 0) goto error; - if (!(def = virNetworkDefParseFile(configFile))) + if (!(def = virNetworkDefParseFile(configFile, 0))) goto error; if (STRNEQ(name, def->name)) { diff --git a/src/esx/esx_network_driver.c b/src/esx/esx_network_driver.c index 31bceb7bff..5e2b2b50f7 100644 --- a/src/esx/esx_network_driver.c +++ b/src/esx/esx_network_driver.c @@ -298,7 +298,7 @@ esxNetworkDefineXML(virConnectPtr conn, const char *xml) return NULL; /* Parse network XML */ - def = virNetworkDefParseString(xml); + def = virNetworkDefParseString(xml, 0); if (!def) return NULL; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f92cc61e47..eef0d83c10 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3609,7 +3609,7 @@ networkCreateXML(virConnectPtr conn, virNetworkPtr net = NULL; virObjectEventPtr event = NULL; - if (!(newDef = virNetworkDefParseString(xml))) + if (!(newDef = virNetworkDefParseString(xml, 0))) goto cleanup; if (virNetworkCreateXMLEnsureACL(conn, newDef) < 0) @@ -3661,7 +3661,7 @@ networkDefineXML(virConnectPtr conn, virNetworkPtr net = NULL; virObjectEventPtr event = NULL; - if (!(def = virNetworkDefParseString(xml))) + if (!(def = virNetworkDefParseString(xml, 0))) goto cleanup; if (virNetworkDefineXMLEnsureACL(conn, def) < 0) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c4e33723d1..ccf6c662c9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4541,7 +4541,7 @@ qemuProcessGetNetworkAddress(const char *netname, if (!xml) goto cleanup; - netdef = virNetworkDefParseString(xml); + netdef = virNetworkDefParseString(xml, 0); if (!netdef) goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 1a42a4f74b..9826ac3fee 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -966,7 +966,7 @@ testParseNetworks(testDriverPtr privconn, if (!node) goto error; - def = virNetworkDefParseNode(ctxt->doc, node); + def = virNetworkDefParseNode(ctxt->doc, node, 0); if (!def) goto error; @@ -3462,7 +3462,7 @@ testNetworkCreateXML(virConnectPtr conn, const char *xml) virNetworkPtr net = NULL; virObjectEventPtr event = NULL; - if ((newDef = virNetworkDefParseString(xml)) == NULL) + if ((newDef = virNetworkDefParseString(xml, 0)) == NULL) goto cleanup; if (!(obj = virNetworkObjAssignDef(privconn->networks, newDef, @@ -3498,7 +3498,7 @@ testNetworkDefineXML(virConnectPtr conn, virNetworkPtr net = NULL; virObjectEventPtr event = NULL; - if ((newDef = virNetworkDefParseString(xml)) == NULL) + if ((newDef = virNetworkDefParseString(xml, 0)) == NULL) goto cleanup; if (!(obj = virNetworkObjAssignDef(privconn->networks, newDef, 0))) diff --git a/src/vbox/vbox_network.c b/src/vbox/vbox_network.c index 50a7a56303..eb47abaf00 100644 --- a/src/vbox/vbox_network.c +++ b/src/vbox/vbox_network.c @@ -375,7 +375,7 @@ vboxNetworkDefineCreateXML(virConnectPtr conn, const char *xml, bool start) PRUnichar *networkNameUtf16 = NULL; char *networkNameUtf8 = NULL; IHostNetworkInterface *networkInterface = NULL; - virNetworkDefPtr def = virNetworkDefParseString(xml); + virNetworkDefPtr def = virNetworkDefParseString(xml, 0); virNetworkIPDefPtr ipdef = NULL; unsigned char uuid[VIR_UUID_BUFLEN]; vboxIID vboxnetiid; diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 8e7751e36b..569763f0e6 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -29,7 +29,7 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr char *pidfile = NULL; dnsmasqContext *dctx = NULL; - if (!(def = virNetworkDefParseFile(inxml))) + if (!(def = virNetworkDefParseFile(inxml, 0))) goto fail; if (!(obj = virNetworkObjNew())) diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c index 242b645767..49f0300a29 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -55,7 +55,7 @@ static int testCompareXMLToArgvFiles(const char *xml, virCommandSetDryRun(&buf, NULL, NULL); - if (!(def = virNetworkDefParseFile(xml))) + if (!(def = virNetworkDefParseFile(xml, 0))) goto cleanup; if (networkAddFirewallRules(def) < 0) diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index eb7db766fa..7828995df1 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -33,7 +33,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml, testCompareNetXML2XMLResult result = TEST_COMPARE_NET_XML2XML_RESULT_SUCCESS; virNetworkDefPtr dev = NULL; - if (!(dev = virNetworkDefParseFile(inxml))) { + if (!(dev = virNetworkDefParseFile(inxml, 0))) { result = TEST_COMPARE_NET_XML2XML_RESULT_FAIL_PARSE; goto cleanup; } diff --git a/tests/networkxml2xmlupdatetest.c b/tests/networkxml2xmlupdatetest.c index b4b44ceff3..dfb21c0064 100644 --- a/tests/networkxml2xmlupdatetest.c +++ b/tests/networkxml2xmlupdatetest.c @@ -30,7 +30,7 @@ testCompareXMLToXMLFiles(const char *netxml, const char *updatexml, if (virTestLoadFile(updatexml, &updateXmlData) < 0) goto error; - if (!(def = virNetworkDefParseFile(netxml))) + if (!(def = virNetworkDefParseFile(netxml, 0))) goto fail; if (virNetworkDefUpdateSection(def, command, section, parentIndex, -- 2.17.1
https://bugzilla.redhat.com/show_bug.cgi?id=1107420 Add a new define/create flag VIR_NETWORK_DEF_PARSE_VALIDATE_NAME to disallow new networks to be defined/created using a name comprised entirely of spaces. Alter the networkxml2xmltest to add a test in order to prove the failure occurs. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/network_conf.c | 9 ++++++++- src/conf/network_conf.h | 7 +++++++ src/network/bridge_driver.c | 6 ++++-- tests/networkxml2xmlin/network-whitespace-name.xml | 6 ++++++ tests/networkxml2xmltest.c | 4 +++- 5 files changed, 28 insertions(+), 4 deletions(-) create mode 100644 tests/networkxml2xmlin/network-whitespace-name.xml diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 34d132e506..2139f61c82 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1604,7 +1604,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt, xmlNodePtr vlanNode; xmlNodePtr metadataNode = NULL; - virCheckFlags(0, NULL); + virCheckFlags(VIR_NETWORK_DEF_PARSE_VALIDATE_NAME, NULL); if (VIR_ALLOC(def) < 0) return NULL; @@ -1619,6 +1619,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt, if (virXMLCheckIllegalChars("name", def->name, "/") < 0) goto error; + if ((flags & VIR_NETWORK_DEF_PARSE_VALIDATE_NAME) && + virStringIsEmpty(def->name)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("name must contain at least one non blank character")); + goto error; + } + /* Extract network uuid */ tmp = virXPathString("string(./uuid[1])", ctxt); if (!tmp) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 6373b783c6..53c187b075 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -293,6 +293,13 @@ enum { virNetworkDefPtr virNetworkDefCopy(virNetworkDefPtr def, unsigned int flags); +typedef enum { + /* Perform extra name validation on new network names which + * will cause failure to parse the XML. Initially just that a + * name cannot be all white space. */ + VIR_NETWORK_DEF_PARSE_VALIDATE_NAME = 1 << 0, +} virNetworkDefParseFlags; + virNetworkDefPtr virNetworkDefParseXML(xmlXPathContextPtr ctxt, unsigned int flags); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index eef0d83c10..33e28c3666 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3608,8 +3608,9 @@ networkCreateXML(virConnectPtr conn, virNetworkDefPtr def; virNetworkPtr net = NULL; virObjectEventPtr event = NULL; + unsigned int parse_flags = VIR_NETWORK_DEF_PARSE_VALIDATE_NAME; - if (!(newDef = virNetworkDefParseString(xml, 0))) + if (!(newDef = virNetworkDefParseString(xml, parse_flags))) goto cleanup; if (virNetworkCreateXMLEnsureACL(conn, newDef) < 0) @@ -3660,8 +3661,9 @@ networkDefineXML(virConnectPtr conn, virNetworkObjPtr obj = NULL; virNetworkPtr net = NULL; virObjectEventPtr event = NULL; + unsigned int parse_flags = VIR_NETWORK_DEF_PARSE_VALIDATE_NAME; - if (!(def = virNetworkDefParseString(xml, 0))) + if (!(def = virNetworkDefParseString(xml, parse_flags))) goto cleanup; if (virNetworkDefineXMLEnsureACL(conn, def) < 0) diff --git a/tests/networkxml2xmlin/network-whitespace-name.xml b/tests/networkxml2xmlin/network-whitespace-name.xml new file mode 100644 index 0000000000..31d54985b4 --- /dev/null +++ b/tests/networkxml2xmlin/network-whitespace-name.xml @@ -0,0 +1,6 @@ +<network> + <name> </name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="virbr0"/> + <mac address='12:34:56:78:9A:BC'/> +</network> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 7828995df1..42063f9904 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -32,8 +32,9 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml, int ret; testCompareNetXML2XMLResult result = TEST_COMPARE_NET_XML2XML_RESULT_SUCCESS; virNetworkDefPtr dev = NULL; + unsigned int parse_flags = VIR_NETWORK_DEF_PARSE_VALIDATE_NAME; - if (!(dev = virNetworkDefParseFile(inxml, 0))) { + if (!(dev = virNetworkDefParseFile(inxml, parse_flags))) { result = TEST_COMPARE_NET_XML2XML_RESULT_FAIL_PARSE; goto cleanup; } @@ -160,6 +161,7 @@ mymain(void) DO_TEST_PARSE_ERROR("passthrough-duplicate"); DO_TEST("metadata"); DO_TEST("set-mtu"); + DO_TEST_PARSE_ERROR("network-whitespace-name"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.17.1
https://bugzilla.redhat.com/show_bug.cgi?id=1107420 Add a new define/create flag VIR_DOMAIN_DEF_PARSE_VALIDATE_NAME to disallow new domains to be defined/created using a name comprised entirely of spaces. Alter the qemuxml2argvtest to add a test in order to prove the failure occurs. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 7 ++++++ src/conf/domain_conf.h | 4 +++ src/lxc/lxc_driver.c | 6 +++-- src/qemu/qemu_driver.c | 6 +++-- tests/qemuxml2argvdata/name-whitespace.xml | 29 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 5 +++- 6 files changed, 52 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/name-whitespace.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 705575fe92..6620ff89af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19278,6 +19278,13 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } + if ((flags & VIR_DOMAIN_DEF_PARSE_VALIDATE_NAME) && + virStringIsEmpty(def->name)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("name must contain at least one non blank character")); + goto error; + } + /* Extract domain uuid. If both uuid and sysinfo/system/entry/uuid * exist, they must match; and if only the latter exists, it can * also serve as the uuid. */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c1dfa37fdf..7eb020d6d7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2996,6 +2996,10 @@ typedef enum { * post parse callbacks before starting. Failure of the post parse callback * is recorded as def->postParseFail */ VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL = 1 << 12, + /* Perform extra name validation on new domain names which + * will cause failure to parse the XML. Initially just that a + * name cannot be all white space. */ + VIR_DOMAIN_DEF_PARSE_VALIDATE_NAME = 1 << 13, } virDomainDefParseFlags; typedef enum { diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 8867645cdc..5d1082292d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -418,7 +418,8 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) virDomainDefPtr oldDef = NULL; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCapsPtr caps = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_VALIDATE_NAME; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -1163,7 +1164,8 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, virObjectEventPtr event = NULL; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCapsPtr caps = NULL; - unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_VALIDATE_NAME; virCheckFlags(VIR_DOMAIN_START_AUTODESTROY | VIR_DOMAIN_START_VALIDATE, NULL); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fb0d4a8c7a..37f10d286e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1710,7 +1710,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, unsigned int start_flags = VIR_QEMU_PROCESS_START_COLD; virCapsPtr caps = NULL; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE | + VIR_DOMAIN_DEF_PARSE_VALIDATE_NAME; virCheckFlags(VIR_DOMAIN_START_PAUSED | VIR_DOMAIN_START_AUTODESTROY | @@ -7381,7 +7382,8 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, virQEMUDriverConfigPtr cfg; virCapsPtr caps = NULL; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; + VIR_DOMAIN_DEF_PARSE_ABI_UPDATE | + VIR_DOMAIN_DEF_PARSE_VALIDATE_NAME; virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); diff --git a/tests/qemuxml2argvdata/name-whitespace.xml b/tests/qemuxml2argvdata/name-whitespace.xml new file mode 100644 index 0000000000..e143c7c770 --- /dev/null +++ b/tests/qemuxml2argvdata/name-whitespace.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name> </name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>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> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 84117a3e63..e92bcd0df6 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -494,7 +494,8 @@ testCompareXMLToArgv(const void *data) if (!(vm = virDomainObjNew(driver.xmlopt))) goto cleanup; - parseFlags |= VIR_DOMAIN_DEF_PARSE_INACTIVE; + parseFlags |= VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_VALIDATE_NAME; if (!(vm->def = virDomainDefParseFile(xml, driver.caps, driver.xmlopt, NULL, parseFlags))) { if (flags & FLAG_EXPECT_PARSE_ERROR) @@ -822,6 +823,8 @@ mymain(void) DO_TEST_PARSE_ERROR("minimal-no-memory", NONE); DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP); + DO_TEST_PARSE_ERROR("name-whitespace", NONE); + DO_TEST_CAPS_LATEST("genid"); DO_TEST_CAPS_LATEST("genid-auto"); -- 2.17.1
Split/rename secretXMLParseNode to virSecretDefParseNode and virSecretDefParseXML in order to follow other drivers usage and naming of the same functionality. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/secret_conf.c | 56 ++++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 21 deletions(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 7a2e4b28aa..faf44bccce 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -121,29 +121,14 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, return 0; } + static virSecretDefPtr -secretXMLParseNode(xmlDocPtr xml, xmlNodePtr root) +virSecretDefParseXML(xmlXPathContextPtr ctxt) { - xmlXPathContextPtr ctxt = NULL; virSecretDefPtr def = NULL, ret = NULL; char *prop = NULL; char *uuidstr = NULL; - if (!virXMLNodeNameEqual(root, "secret")) { - virReportError(VIR_ERR_XML_ERROR, - _("unexpected root element <%s>, " - "expecting <secret>"), - root->name); - goto cleanup; - } - - ctxt = xmlXPathNewContext(xml); - if (ctxt == NULL) { - virReportOOMError(); - goto cleanup; - } - ctxt->node = root; - if (VIR_ALLOC(def) < 0) goto cleanup; @@ -195,17 +180,46 @@ secretXMLParseNode(xmlDocPtr xml, xmlNodePtr root) if (virXPathNode("./usage", ctxt) != NULL && virSecretDefParseUsage(ctxt, def) < 0) goto cleanup; - ret = def; - def = NULL; + VIR_STEAL_PTR(ret, def); cleanup: VIR_FREE(prop); VIR_FREE(uuidstr); virSecretDefFree(def); - xmlXPathFreeContext(ctxt); return ret; } + +static virSecretDefPtr +virSecretDefParseNode(xmlDocPtr xml, + xmlNodePtr root) +{ + xmlXPathContextPtr ctxt = NULL; + virSecretDefPtr def = NULL; + + if (!virXMLNodeNameEqual(root, "secret")) { + virReportError(VIR_ERR_XML_ERROR, + _("unexpected root element <%s>, " + "expecting <secret>"), + root->name); + goto cleanup; + } + + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) { + virReportOOMError(); + goto cleanup; + } + ctxt->node = root; + + def = virSecretDefParseXML(ctxt); + + cleanup: + xmlXPathFreeContext(ctxt); + return def; +} + + static virSecretDefPtr virSecretDefParse(const char *xmlStr, const char *filename) @@ -214,7 +228,7 @@ virSecretDefParse(const char *xmlStr, virSecretDefPtr ret = NULL; if ((xml = virXMLParse(filename, xmlStr, _("(definition_of_secret)")))) { - ret = secretXMLParseNode(xml, xmlDocGetRootElement(xml)); + ret = virSecretDefParseNode(xml, xmlDocGetRootElement(xml)); xmlFreeDoc(xml); } -- 2.17.1
Add a @flags argument when parsing the secret definition via the virSecretDefParse{XML|Node|String|File} API's as this will allow us to in the future make parsing decisions based on the @flags. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/secret_conf.c | 25 ++++++++++++++++--------- src/conf/secret_conf.h | 9 +++++++-- src/conf/virsecretobj.c | 2 +- src/secret/secret_driver.c | 2 +- tests/secretxml2xmltest.c | 2 +- 5 files changed, 26 insertions(+), 14 deletions(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index faf44bccce..d98a4f2442 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -123,12 +123,15 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt, static virSecretDefPtr -virSecretDefParseXML(xmlXPathContextPtr ctxt) +virSecretDefParseXML(xmlXPathContextPtr ctxt, + unsigned int flags) { virSecretDefPtr def = NULL, ret = NULL; char *prop = NULL; char *uuidstr = NULL; + virCheckFlags(0, NULL); + if (VIR_ALLOC(def) < 0) goto cleanup; @@ -192,7 +195,8 @@ virSecretDefParseXML(xmlXPathContextPtr ctxt) static virSecretDefPtr virSecretDefParseNode(xmlDocPtr xml, - xmlNodePtr root) + xmlNodePtr root, + unsigned int flags) { xmlXPathContextPtr ctxt = NULL; virSecretDefPtr def = NULL; @@ -212,7 +216,7 @@ virSecretDefParseNode(xmlDocPtr xml, } ctxt->node = root; - def = virSecretDefParseXML(ctxt); + def = virSecretDefParseXML(ctxt, flags); cleanup: xmlXPathFreeContext(ctxt); @@ -222,13 +226,14 @@ virSecretDefParseNode(xmlDocPtr xml, static virSecretDefPtr virSecretDefParse(const char *xmlStr, - const char *filename) + const char *filename, + unsigned int flags) { xmlDocPtr xml; virSecretDefPtr ret = NULL; if ((xml = virXMLParse(filename, xmlStr, _("(definition_of_secret)")))) { - ret = virSecretDefParseNode(xml, xmlDocGetRootElement(xml)); + ret = virSecretDefParseNode(xml, xmlDocGetRootElement(xml), flags); xmlFreeDoc(xml); } @@ -236,15 +241,17 @@ virSecretDefParse(const char *xmlStr, } virSecretDefPtr -virSecretDefParseString(const char *xmlStr) +virSecretDefParseString(const char *xmlStr, + unsigned int flags) { - return virSecretDefParse(xmlStr, NULL); + return virSecretDefParse(xmlStr, NULL, flags); } virSecretDefPtr -virSecretDefParseFile(const char *filename) +virSecretDefParseFile(const char *filename, + unsigned int flags) { - return virSecretDefParse(NULL, filename); + return virSecretDefParse(NULL, filename, flags); } static int diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 19f612b8fe..2a19629f54 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -38,8 +38,13 @@ struct _virSecretDef { }; void virSecretDefFree(virSecretDefPtr def); -virSecretDefPtr virSecretDefParseString(const char *xml); -virSecretDefPtr virSecretDefParseFile(const char *filename); + +virSecretDefPtr virSecretDefParseString(const char *xml, + unsigned int flags); + +virSecretDefPtr virSecretDefParseFile(const char *filename, + unsigned int flags); + char *virSecretDefFormat(const virSecretDef *def); # define VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL \ diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 48ce3c8141..9527d7570e 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -909,7 +909,7 @@ virSecretLoad(virSecretObjListPtr secrets, virSecretDefPtr def = NULL; virSecretObjPtr obj = NULL; - if (!(def = virSecretDefParseFile(path))) + if (!(def = virSecretDefParseFile(path, 0))) goto cleanup; if (virSecretLoadValidateUUID(def, file) < 0) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 3c3557947e..5e1f82a314 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -218,7 +218,7 @@ secretDefineXML(virConnectPtr conn, virCheckFlags(0, NULL); - if (!(def = virSecretDefParseString(xml))) + if (!(def = virSecretDefParseString(xml, 0))) return NULL; if (virSecretDefineXMLEnsureACL(conn, def) < 0) diff --git a/tests/secretxml2xmltest.c b/tests/secretxml2xmltest.c index 306f64c236..573edd6012 100644 --- a/tests/secretxml2xmltest.c +++ b/tests/secretxml2xmltest.c @@ -15,7 +15,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml) int ret = -1; virSecretDefPtr secret = NULL; - if (!(secret = virSecretDefParseFile(inxml))) + if (!(secret = virSecretDefParseFile(inxml, 0))) goto fail; if (!(actual = virSecretDefFormat(secret))) -- 2.17.1
https://bugzilla.redhat.com/show_bug.cgi?id=1107420 Add a new define/create flag VIR_SECRET_DEF_PARSE_VALIDATE_USAGE_ID to disallow new secrets to be defined/created using a usage id comprised entirely of spaces. Alter the secretxml2xmltest to add a parse failure scenario and a test in order to prove the failure occurs. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/secret_conf.c | 10 ++++++- src/conf/secret_conf.h | 7 +++++ src/secret/secret_driver.c | 3 +- .../usage-whitespace-invalid.xml | 7 +++++ tests/secretxml2xmltest.c | 30 +++++++++++++++---- 5 files changed, 49 insertions(+), 8 deletions(-) create mode 100644 tests/secretxml2xmlin/usage-whitespace-invalid.xml diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index d98a4f2442..9a77327014 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -130,7 +130,7 @@ virSecretDefParseXML(xmlXPathContextPtr ctxt, char *prop = NULL; char *uuidstr = NULL; - virCheckFlags(0, NULL); + virCheckFlags(VIR_SECRET_DEF_PARSE_VALIDATE_USAGE_ID, NULL); if (VIR_ALLOC(def) < 0) goto cleanup; @@ -183,6 +183,14 @@ virSecretDefParseXML(xmlXPathContextPtr ctxt, if (virXPathNode("./usage", ctxt) != NULL && virSecretDefParseUsage(ctxt, def) < 0) goto cleanup; + + if (def->usage_id && (flags & VIR_SECRET_DEF_PARSE_VALIDATE_USAGE_ID) && + virStringIsEmpty(def->usage_id)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("usage name must contain at least one non blank character")); + goto cleanup; + } + VIR_STEAL_PTR(ret, def); cleanup: diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 2a19629f54..b7aa70d785 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -39,6 +39,13 @@ struct _virSecretDef { void virSecretDefFree(virSecretDefPtr def); +typedef enum { + /* Perform extra name validation on new secret usage ids which + * will cause failure to parse the XML. Initially just that a + * name cannot be all white space. */ + VIR_SECRET_DEF_PARSE_VALIDATE_USAGE_ID = 1 << 0, +} virSecretDefParseFlags; + virSecretDefPtr virSecretDefParseString(const char *xml, unsigned int flags); diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 5e1f82a314..2a66481d37 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -215,10 +215,11 @@ secretDefineXML(virConnectPtr conn, virSecretDefPtr backup = NULL; virSecretDefPtr def; virObjectEventPtr event = NULL; + unsigned int parse_flags = VIR_SECRET_DEF_PARSE_VALIDATE_USAGE_ID; virCheckFlags(0, NULL); - if (!(def = virSecretDefParseString(xml, 0))) + if (!(def = virSecretDefParseString(xml, parse_flags))) return NULL; if (virSecretDefineXMLEnsureACL(conn, def) < 0) diff --git a/tests/secretxml2xmlin/usage-whitespace-invalid.xml b/tests/secretxml2xmlin/usage-whitespace-invalid.xml new file mode 100644 index 0000000000..7611bd53d1 --- /dev/null +++ b/tests/secretxml2xmlin/usage-whitespace-invalid.xml @@ -0,0 +1,7 @@ +<secret ephemeral='no' private='yes'> + <uuid>f52a81b2-424e-490c-823d-6bd4235bc572</uuid> + <description>Ceph secret</description> + <usage type='ceph'> + <name> </name> + </usage> +</secret> diff --git a/tests/secretxml2xmltest.c b/tests/secretxml2xmltest.c index 573edd6012..3c624d8319 100644 --- a/tests/secretxml2xmltest.c +++ b/tests/secretxml2xmltest.c @@ -9,14 +9,24 @@ #define VIR_FROM_THIS VIR_FROM_NONE static int -testCompareXMLToXMLFiles(const char *inxml, const char *outxml) +testCompareXMLToXMLFiles(const char *inxml, + const char *outxml, + bool expect_parse_fail) { char *actual = NULL; int ret = -1; virSecretDefPtr secret = NULL; - - if (!(secret = virSecretDefParseFile(inxml, 0))) + unsigned int parse_flags = VIR_SECRET_DEF_PARSE_VALIDATE_USAGE_ID; + + if (!(secret = virSecretDefParseFile(inxml, parse_flags))) { + if (expect_parse_fail) { + VIR_TEST_DEBUG("Got expected parse failure msg='%s'", + virGetLastErrorMessage()); + virResetLastError(); + ret = 0; + } goto fail; + } if (!(actual = virSecretDefFormat(secret))) goto fail; @@ -35,6 +45,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml) struct testInfo { const char *name; bool different; + bool expect_fail; }; static int @@ -54,7 +65,7 @@ testCompareXMLToXMLHelper(const void *data) goto cleanup; } - result = testCompareXMLToXMLFiles(inxml, outxml); + result = testCompareXMLToXMLFiles(inxml, outxml, info->expect_fail); cleanup: VIR_FREE(inxml); @@ -68,19 +79,26 @@ mymain(void) { int ret = 0; -#define DO_TEST(name) \ +#define DO_TEST_FULL(name, different, parse_fail) \ do { \ - const struct testInfo info = {name, false}; \ + const struct testInfo info = {name, different, parse_fail}; \ if (virTestRun("Secret XML->XML " name, \ testCompareXMLToXMLHelper, &info) < 0) \ ret = -1; \ } while (0) +#define DO_TEST(name) \ + DO_TEST_FULL(name, false, false) + +#define DO_TEST_PARSE_FAIL(name) \ + DO_TEST_FULL(name, false, true) + DO_TEST("ephemeral-usage-volume"); DO_TEST("usage-volume"); DO_TEST("usage-ceph"); DO_TEST("usage-iscsi"); DO_TEST("usage-tls"); + DO_TEST_PARSE_FAIL("usage-whitespace-invalid"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.17.1
Add a @flags argument when parsing the nwfilter definition via the virNWFilterDefParse{XML|Node|String|File} API's as this will allow us to in the future make parsing decisions based on the @flags. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 25 ++++++++++++++++--------- src/conf/nwfilter_conf.h | 9 ++++++--- src/conf/virnwfilterobj.c | 2 +- src/nwfilter/nwfilter_driver.c | 2 +- tests/nwfilterxml2firewalltest.c | 2 +- tests/nwfilterxml2xmltest.c | 2 +- 6 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 120ca5ec14..c1867fb946 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2602,7 +2602,8 @@ virNWFilterIsAllowedChain(const char *chainname) static virNWFilterDefPtr -virNWFilterDefParseXML(xmlXPathContextPtr ctxt) +virNWFilterDefParseXML(xmlXPathContextPtr ctxt, + unsigned int flags) { virNWFilterDefPtr ret; xmlNodePtr curr = ctxt->node; @@ -2613,6 +2614,8 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt) int chain_priority; const char *name_prefix; + virCheckFlags(0, NULL); + if (VIR_ALLOC(ret) < 0) return NULL; @@ -2734,7 +2737,8 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt) virNWFilterDefPtr virNWFilterDefParseNode(xmlDocPtr xml, - xmlNodePtr root) + xmlNodePtr root, + unsigned int flags) { xmlXPathContextPtr ctxt = NULL; virNWFilterDefPtr def = NULL; @@ -2753,7 +2757,7 @@ virNWFilterDefParseNode(xmlDocPtr xml, } ctxt->node = root; - def = virNWFilterDefParseXML(ctxt); + def = virNWFilterDefParseXML(ctxt, flags); cleanup: xmlXPathFreeContext(ctxt); @@ -2763,13 +2767,14 @@ virNWFilterDefParseNode(xmlDocPtr xml, static virNWFilterDefPtr virNWFilterDefParse(const char *xmlStr, - const char *filename) + const char *filename, + unsigned int flags) { virNWFilterDefPtr def = NULL; xmlDocPtr xml; if ((xml = virXMLParse(filename, xmlStr, _("(nwfilter_definition)")))) { - def = virNWFilterDefParseNode(xml, xmlDocGetRootElement(xml)); + def = virNWFilterDefParseNode(xml, xmlDocGetRootElement(xml), flags); xmlFreeDoc(xml); } @@ -2778,16 +2783,18 @@ virNWFilterDefParse(const char *xmlStr, virNWFilterDefPtr -virNWFilterDefParseString(const char *xmlStr) +virNWFilterDefParseString(const char *xmlStr, + unsigned int flags) { - return virNWFilterDefParse(xmlStr, NULL); + return virNWFilterDefParse(xmlStr, NULL, flags); } virNWFilterDefPtr -virNWFilterDefParseFile(const char *filename) +virNWFilterDefParseFile(const char *filename, + unsigned int flags) { - return virNWFilterDefParse(NULL, filename); + return virNWFilterDefParse(NULL, filename, flags); } diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 9f8ad51bf2..5ffdc07fab 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -561,7 +561,8 @@ virNWFilterDeleteDef(const char *configDir, virNWFilterDefPtr virNWFilterDefParseNode(xmlDocPtr xml, - xmlNodePtr root); + xmlNodePtr root, + unsigned int flags); char * virNWFilterDefFormat(const virNWFilterDef *def); @@ -571,10 +572,12 @@ virNWFilterSaveConfig(const char *configDir, virNWFilterDefPtr def); virNWFilterDefPtr -virNWFilterDefParseString(const char *xml); +virNWFilterDefParseString(const char *xml, + unsigned int flags); virNWFilterDefPtr -virNWFilterDefParseFile(const char *filename); +virNWFilterDefParseFile(const char *filename, + unsigned int flags); void virNWFilterWriteLockFilterUpdates(void); diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 0136a0d56c..3993be3874 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -503,7 +503,7 @@ virNWFilterObjListLoadConfig(virNWFilterObjListPtr nwfilters, if (!(configFile = virFileBuildPath(configDir, name, ".xml"))) goto error; - if (!(def = virNWFilterDefParseFile(configFile))) + if (!(def = virNWFilterDefParseFile(configFile, 0))) goto error; if (STRNEQ(name, def->name)) { diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index ac3a964388..d850a66b28 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -556,7 +556,7 @@ nwfilterDefineXML(virConnectPtr conn, nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); - if (!(def = virNWFilterDefParseString(xml))) + if (!(def = virNWFilterDefParseString(xml, 0))) goto cleanup; if (virNWFilterDefineXMLEnsureACL(conn, def) < 0) diff --git a/tests/nwfilterxml2firewalltest.c b/tests/nwfilterxml2firewalltest.c index 043b7d170e..786761a04e 100644 --- a/tests/nwfilterxml2firewalltest.c +++ b/tests/nwfilterxml2firewalltest.c @@ -275,7 +275,7 @@ virNWFilterDefToInst(const char *xml, { size_t i; int ret = -1; - virNWFilterDefPtr def = virNWFilterDefParseFile(xml); + virNWFilterDefPtr def = virNWFilterDefParseFile(xml, 0); if (!def) return -1; diff --git a/tests/nwfilterxml2xmltest.c b/tests/nwfilterxml2xmltest.c index 89f809ca8f..0c79afa8ee 100644 --- a/tests/nwfilterxml2xmltest.c +++ b/tests/nwfilterxml2xmltest.c @@ -29,7 +29,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml, virResetLastError(); - if (!(dev = virNWFilterDefParseFile(inxml))) { + if (!(dev = virNWFilterDefParseFile(inxml, 0))) { if (expect_error) { virResetLastError(); goto done; -- 2.17.1
https://bugzilla.redhat.com/show_bug.cgi?id=1107420 Add a new define/create flag VIR_NWFILTER_DEF_PARSE_VALIDATE_NAME to disallow new nwfilters to be defined/created using a name comprised entirely of spaces. Alter the nwfilterxml2xmltest to add a test in order to prove the failure occurs. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/nwfilter_conf.c | 9 ++++++++- src/conf/nwfilter_conf.h | 7 +++++++ src/nwfilter/nwfilter_driver.c | 3 ++- tests/nwfilterxml2xmlin/name-whitespace-invalid.xml | 4 ++++ tests/nwfilterxml2xmltest.c | 7 ++++++- 5 files changed, 27 insertions(+), 3 deletions(-) create mode 100644 tests/nwfilterxml2xmlin/name-whitespace-invalid.xml diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index c1867fb946..4f99f88dca 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2614,7 +2614,7 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt, int chain_priority; const char *name_prefix; - virCheckFlags(0, NULL); + virCheckFlags(VIR_NWFILTER_DEF_PARSE_VALIDATE_NAME, NULL); if (VIR_ALLOC(ret) < 0) return NULL; @@ -2626,6 +2626,13 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt, goto cleanup; } + if ((flags & VIR_NWFILTER_DEF_PARSE_VALIDATE_NAME) && + virStringIsEmpty(ret->name)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("name must contain at least one non blank character")); + goto cleanup; + } + chain_pri_s = virXPathString("string(./@priority)", ctxt); if (chain_pri_s) { if (virStrToLong_i(chain_pri_s, NULL, 10, &chain_priority) < 0) { diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 5ffdc07fab..2a7eabbf91 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -559,6 +559,13 @@ int virNWFilterDeleteDef(const char *configDir, virNWFilterDefPtr def); +typedef enum { + /* Perform extra name validation on new nwfilter names which + * will cause failure to parse the XML. Initially just that a + * name cannot be all white space. */ + VIR_NWFILTER_DEF_PARSE_VALIDATE_NAME = 1 << 0, +} virNWFilterDefParseFlags; + virNWFilterDefPtr virNWFilterDefParseNode(xmlDocPtr xml, xmlNodePtr root, diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index d850a66b28..3529dfa519 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -546,6 +546,7 @@ nwfilterDefineXML(virConnectPtr conn, virNWFilterObjPtr obj = NULL; virNWFilterDefPtr objdef; virNWFilterPtr nwfilter = NULL; + unsigned int parse_flags = VIR_NWFILTER_DEF_PARSE_VALIDATE_NAME; if (!driver->privileged) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -556,7 +557,7 @@ nwfilterDefineXML(virConnectPtr conn, nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); - if (!(def = virNWFilterDefParseString(xml, 0))) + if (!(def = virNWFilterDefParseString(xml, parse_flags))) goto cleanup; if (virNWFilterDefineXMLEnsureACL(conn, def) < 0) diff --git a/tests/nwfilterxml2xmlin/name-whitespace-invalid.xml b/tests/nwfilterxml2xmlin/name-whitespace-invalid.xml new file mode 100644 index 0000000000..452847ae93 --- /dev/null +++ b/tests/nwfilterxml2xmlin/name-whitespace-invalid.xml @@ -0,0 +1,4 @@ +<filter name=' '> + <uuid>83011800-f663-96d6-8841-fd836b4318c6</uuid> + <filterref filter=' '/> +</filter> diff --git a/tests/nwfilterxml2xmltest.c b/tests/nwfilterxml2xmltest.c index 0c79afa8ee..de63ab1a91 100644 --- a/tests/nwfilterxml2xmltest.c +++ b/tests/nwfilterxml2xmltest.c @@ -26,11 +26,14 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml, char *actual = NULL; int ret = -1; virNWFilterDefPtr dev = NULL; + unsigned int parse_flags = VIR_NWFILTER_DEF_PARSE_VALIDATE_NAME; virResetLastError(); - if (!(dev = virNWFilterDefParseFile(inxml, 0))) { + if (!(dev = virNWFilterDefParseFile(inxml, parse_flags))) { if (expect_error) { + VIR_TEST_DEBUG("Got expected parse failure msg='%s'", + virGetLastErrorMessage()); virResetLastError(); goto done; } @@ -149,6 +152,8 @@ mymain(void) DO_TEST("ipset-test", false); + DO_TEST("name-whitespace-invalid", true); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.17.1
https://bugzilla.redhat.com/show_bug.cgi?id=1107420 Add a new define/create flag VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE_NAME to disallow new snapshots to be defined/created using a name comprised entirely of spaces. Alter the domainsnapshotxml2xmltest to add a test in order to prove the failure occurs. Signed-off-by: John Ferlan <jferlan@redhat.com> --- Looks like the original send had a network glitch right at this patch so just making sure 11/11 makes it to the list (and hopefully as a reply to the 0/11 cover). src/conf/snapshot_conf.c | 7 ++++ src/conf/snapshot_conf.h | 1 + src/qemu/qemu_driver.c | 3 +- .../name_whitespace.xml | 3 ++ tests/domainsnapshotxml2xmltest.c | 38 ++++++++++++++----- 5 files changed, 42 insertions(+), 10 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlin/name_whitespace.xml diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index adba149241..2897a7edf5 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -235,6 +235,13 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, goto cleanup; } + if ((flags & VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE_NAME) && + virStringIsEmpty(def->name)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + ("name must contain at least one non blank character")); + goto cleanup; + } + def->description = virXPathString("string(./description)", ctxt); if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 20a42bd572..2599c6b57f 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -101,6 +101,7 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_DISKS = 1 << 1, VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 2, VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE = 1 << 3, + VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE_NAME = 1 << 4, } virDomainSnapshotParseFlags; virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 37f10d286e..b29faffee1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15279,7 +15279,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, virDomainSnapshotDefPtr def = NULL; bool update_current = true; bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE; - unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | + VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE_NAME; virDomainSnapshotObjPtr other = NULL; int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; bool align_match = true; diff --git a/tests/domainsnapshotxml2xmlin/name_whitespace.xml b/tests/domainsnapshotxml2xmlin/name_whitespace.xml new file mode 100644 index 0000000000..901bcce62f --- /dev/null +++ b/tests/domainsnapshotxml2xmlin/name_whitespace.xml @@ -0,0 +1,3 @@ +<domainsnapshot> + <name> </name> +</domainsnapshot> diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index 5ea0f325de..046e8f7525 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -74,14 +74,16 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml, const char *uuid, bool internal, - bool redefine) + bool redefine, + bool expect_parse_fail) { char *inXmlData = NULL; char *outXmlData = NULL; char *actual = NULL; int ret = -1; virDomainSnapshotDefPtr def = NULL; - unsigned int flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | + VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE_NAME; if (internal) flags |= VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL; @@ -97,8 +99,15 @@ testCompareXMLToXMLFiles(const char *inxml, if (!(def = virDomainSnapshotDefParseString(inXmlData, driver.caps, driver.xmlopt, - flags))) + flags))) { + if (expect_parse_fail) { + VIR_TEST_DEBUG("Got expected parse failure msg='%s'", + virGetLastErrorMessage()); + virResetLastError(); + ret = 0; + } goto cleanup; + } if (!(actual = virDomainSnapshotDefFormat(uuid, def, driver.caps, driver.xmlopt, @@ -135,6 +144,7 @@ struct testInfo { const char *uuid; bool internal; bool redefine; + bool expect_parse_fail; }; @@ -144,7 +154,8 @@ testCompareXMLToXMLHelper(const void *data) const struct testInfo *info = data; return testCompareXMLToXMLFiles(info->inxml, info->outxml, info->uuid, - info->internal, info->redefine); + info->internal, info->redefine, + info->expect_parse_fail); } @@ -168,11 +179,13 @@ mymain(void) } -# define DO_TEST(prefix, name, inpath, outpath, uuid, internal, redefine) \ +# define DO_TEST(prefix, name, inpath, outpath, uuid, internal, redefine, \ + expect_parse_fail) \ do { \ const struct testInfo info = {abs_srcdir "/" inpath "/" name ".xml", \ abs_srcdir "/" outpath "/" name ".xml", \ - uuid, internal, redefine}; \ + uuid, internal, redefine, \ + expect_parse_fail}; \ if (virTestRun("SNAPSHOT XML-2-XML " prefix " " name, \ testCompareXMLToXMLHelper, &info) < 0) \ ret = -1; \ @@ -181,18 +194,23 @@ mymain(void) # define DO_TEST_IN(name, uuid) DO_TEST("in->in", name,\ "domainsnapshotxml2xmlin",\ "domainsnapshotxml2xmlin",\ - uuid, false, false) + uuid, false, false, false) + +# define DO_TEST_IN_PARSE_FAIL(name, uuid) DO_TEST("in->in", name,\ + "domainsnapshotxml2xmlin",\ + "domainsnapshotxml2xmlin",\ + uuid, false, false, true) # define DO_TEST_OUT(name, uuid, internal) DO_TEST("out->out", name,\ "domainsnapshotxml2xmlout",\ "domainsnapshotxml2xmlout",\ - uuid, internal, true) + uuid, internal, true, false) # define DO_TEST_INOUT(name, uuid, internal, redefine) \ DO_TEST("in->out", name,\ "domainsnapshotxml2xmlin",\ "domainsnapshotxml2xmlout",\ - uuid, internal, redefine) + uuid, internal, redefine, false) /* Unset or set all envvars here that are copied in qemudBuildCommandLine * using ADD_ENV_COPY, otherwise these tests may fail due to unexpected @@ -219,6 +237,8 @@ mymain(void) DO_TEST_IN("description_only", NULL); DO_TEST_IN("name_only", NULL); + DO_TEST_IN_PARSE_FAIL("name_whitespace", NULL); + cleanup: if (testSnapshotXMLVariableLineRegex) regfree(testSnapshotXMLVariableLineRegex); -- 2.17.1
On Mon, Jul 30, 2018 at 02:46:37PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1107420
As unusual as the case may be, having a name of all white space has been allowed. This leads to the obvious problem of how a future usage would be able to "utilize" that resource since it's not "simple" to determine what combination of spaces and tabs are being used for the name.
So if the user wants it difficult, they can do so. What do we gain by forbidding it?
46 files changed, 418 insertions(+), 131 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlin/name_whitespace.xml create mode 100644 tests/networkxml2xmlin/network-whitespace-name.xml create mode 100644 tests/nwfilterxml2xmlin/name-whitespace-invalid.xml create mode 100644 tests/qemuxml2argvdata/name-whitespace.xml create mode 100644 tests/secretxml2xmlin/usage-whitespace-invalid.xml create mode 100644 tests/storagepoolxml2xmlin/pool-dir-whitespace-name.xml
(apart from extra 300 lines) Jano
On 07/31/2018 04:28 AM, Ján Tomko wrote:
On Mon, Jul 30, 2018 at 02:46:37PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1107420
As unusual as the case may be, having a name of all white space has been allowed. This leads to the obvious problem of how a future usage would be able to "utilize" that resource since it's not "simple" to determine what combination of spaces and tabs are being used for the name.
So if the user wants it difficult, they can do so. What do we gain by forbidding it?
46 files changed, 418 insertions(+), 131 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlin/name_whitespace.xml create mode 100644 tests/networkxml2xmlin/network-whitespace-name.xml create mode 100644 tests/nwfilterxml2xmlin/name-whitespace-invalid.xml create mode 100644 tests/qemuxml2argvdata/name-whitespace.xml create mode 100644 tests/secretxml2xmlin/usage-whitespace-invalid.xml create mode 100644 tests/storagepoolxml2xmlin/pool-dir-whitespace-name.xml
(apart from extra 300 lines)
Jano
Since when has the number of insertions/deletions become the determining factor of what is and isn't a problem? I could drop the tests and shave a few off. Skipping a few comments along the way may pick up a few more. As long as one "knows" to list certain objects using 'virsh xxx-list --uuid', then they can manipulate the white space named object once they figure out which UUID is associated with their white space named resource. Still snapshot's don't provide that, so they'd be a problem. At least nwfilter and secrets are in your face with the UUID. Domains, networks, and pools just make you work a bit harder. If you don't think this is a problem, then grab/own the bz and close it. John
participants (2)
-
John Ferlan -
Ján Tomko