[libvirt] [PATCH v4 0/3] Forbid new-line char in name of networks

v3: http://www.redhat.com/archives/libvir-list/2016-October/msg00627.html Differences in v4: * function to check string moved from src/util/virstring to src/util/virxml Sławek Kapłoński (3): util: Add function to check if string contains some illegal chars Use new util function to check network name Forbid new-line char in name of new networks src/conf/network_conf.c | 5 +---- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 3 +++ src/util/virxml.c | 28 ++++++++++++++++++++++++++++ src/util/virxml.h | 3 +++ 5 files changed, 36 insertions(+), 4 deletions(-) -- 2.10.0

This new function can be used to check if e.g. name of XML node don't contains forbidden chars like "/" or "\n". --- src/libvirt_private.syms | 1 + src/util/virxml.c | 28 ++++++++++++++++++++++++++++ src/util/virxml.h | 3 +++ 3 files changed, 32 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55b6a24..f91f179 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2678,6 +2678,7 @@ virUUIDParse; # util/virxml.h virXMLChildElementCount; virXMLExtractNamespaceXML; +virXMLNodeHasIllegalChars; virXMLNodeSanitizeNamespaces; virXMLNodeToString; virXMLParseHelper; diff --git a/src/util/virxml.c b/src/util/virxml.c index 03bd784..b0d0a94 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -462,6 +462,34 @@ virXPathLongLong(const char *xpath, return ret; } + +/** + * virXMLNodeHasIllegalChars: checks if string contains at least one of + * forbidden characters + * + * @node_name: Name of checked node + * @node: string to check + * @chars: illegal chars to check + * + * If node contains any of illegal chars VIR_ERR_XML_DETAIL error will be + * reported. + * + * Returns: 0 if string don't contains any of given characters, -1 otherwise + */ +int +virXMLNodeHasIllegalChars(const char *node_name, + const char *node, + const char *chars) +{ + if (strpbrk(node, chars)) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid char in %s: 0x%02x"), node_name, *chars); + return -1; + } + return 0; +} + + /** * virXMLPropString: * @node: XML dom node pointer diff --git a/src/util/virxml.h b/src/util/virxml.h index 7a0a1da..4425cf8 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -71,6 +71,9 @@ xmlNodePtr virXPathNode(const char *xpath, int virXPathNodeSet(const char *xpath, xmlXPathContextPtr ctxt, xmlNodePtr **list); +int virXMLNodeHasIllegalChars(const char *node_name, + const char *node, + const char *chars); char * virXMLPropString(xmlNodePtr node, const char *name); long virXMLChildElementCount(xmlNodePtr node); -- 2.10.0

On 20.10.2016 04:57, Sławek Kapłoński wrote:
This new function can be used to check if e.g. name of XML node don't contains forbidden chars like "/" or "\n". --- src/libvirt_private.syms | 1 + src/util/virxml.c | 28 ++++++++++++++++++++++++++++ src/util/virxml.h | 3 +++ 3 files changed, 32 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55b6a24..f91f179 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2678,6 +2678,7 @@ virUUIDParse; # util/virxml.h virXMLChildElementCount; virXMLExtractNamespaceXML; +virXMLNodeHasIllegalChars; virXMLNodeSanitizeNamespaces; virXMLNodeToString; virXMLParseHelper; diff --git a/src/util/virxml.c b/src/util/virxml.c index 03bd784..b0d0a94 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -462,6 +462,34 @@ virXPathLongLong(const char *xpath, return ret; }
+ +/** + * virXMLNodeHasIllegalChars: checks if string contains at least one of + * forbidden characters + * + * @node_name: Name of checked node + * @node: string to check + * @chars: illegal chars to check + * + * If node contains any of illegal chars VIR_ERR_XML_DETAIL error will be + * reported. + * + * Returns: 0 if string don't contains any of given characters, -1 otherwise + */ +int +virXMLNodeHasIllegalChars(const char *node_name, + const char *node, + const char *chars) +{ + if (strpbrk(node, chars)) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid char in %s: 0x%02x"), node_name, *chars);
Almost. So for instance when I call this function like this: virXMLNodeHasIllegalChars("name", "some string\n", "\r\t\n"); I will see the following error message: "invalid char in name: \r" I'd expect to see "invalid char in name: \n". But that's okay. I can fix it before pushing. Also, virXMLNode.* may suggest that it takes xmlNodePtr which is not the case. I will rename this before pushing. ACK Michal

New util function virXMLNodeHasIllegalChars is now used to test if parsed network contains illegal char '/' in it's name. --- src/conf/network_conf.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa39776..bd934f1 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2117,11 +2117,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; } - if (strchr(def->name, '/')) { - virReportError(VIR_ERR_XML_ERROR, - _("name %s cannot contain '/'"), def->name); + if (virXMLNodeHasIllegalChars("name", def->name, "/")) goto error; - } /* Extract network uuid */ tmp = virXPathString("string(./uuid[1])", ctxt); -- 2.10.0

On 20.10.2016 04:57, Sławek Kapłoński wrote:
New util function virXMLNodeHasIllegalChars is now used to test if parsed network contains illegal char '/' in it's name. --- src/conf/network_conf.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa39776..bd934f1 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2117,11 +2117,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; }
- if (strchr(def->name, '/')) { - virReportError(VIR_ERR_XML_ERROR, - _("name %s cannot contain '/'"), def->name); + if (virXMLNodeHasIllegalChars("name", def->name, "/")) goto error; - }
/* Extract network uuid */ tmp = virXPathString("string(./uuid[1])", ctxt);
ACK (after I reflect the rename made in 1/3 obviously). Michal

New line character in name of network is now forbidden because it mess virsh output and can be confusing for users. Validation of name is done in network driver, after parsing XML to avoid problems with dissappeared network which was already created with new-line char in name. Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064 --- src/network/bridge_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b2af482..a5a116a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2973,6 +2973,9 @@ networkValidate(virNetworkDriverStatePtr driver, bool bandwidthAllowed = true; bool usesInterface = false, usesAddress = false; + if (virXMLNodeHasIllegalChars("name", def->name, "\n")) + return -1; + /* Only the three L3 network types that are configured by libvirt * need to have a bridge device name / mac address provided */ -- 2.10.0

On 20.10.2016 04:57, Sławek Kapłoński wrote:
New line character in name of network is now forbidden because it mess virsh output and can be confusing for users. Validation of name is done in network driver, after parsing XML to avoid problems with dissappeared network which was already created with new-line char in name.
Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064 --- src/network/bridge_driver.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b2af482..a5a116a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2973,6 +2973,9 @@ networkValidate(virNetworkDriverStatePtr driver, bool bandwidthAllowed = true; bool usesInterface = false, usesAddress = false;
+ if (virXMLNodeHasIllegalChars("name", def->name, "\n")) + return -1; + /* Only the three L3 network types that are configured by libvirt * need to have a bridge device name / mac address provided */
ACK here too. Michal

On 20.10.2016 04:57, Sławek Kapłoński wrote:
v3: http://www.redhat.com/archives/libvir-list/2016-October/msg00627.html
Differences in v4: * function to check string moved from src/util/virstring to src/util/virxml
Sławek Kapłoński (3): util: Add function to check if string contains some illegal chars Use new util function to check network name Forbid new-line char in name of new networks
src/conf/network_conf.c | 5 +---- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 3 +++ src/util/virxml.c | 28 ++++++++++++++++++++++++++++ src/util/virxml.h | 3 +++ 5 files changed, 36 insertions(+), 4 deletions(-)
I've fixed all the small nits I've found and raised in the review and pushed this. Congratulations on your first libvirt contribution! Michal

Hello, -- Best regards / Pozdrawiam Sławek Kapłoński slawek@kaplonski.pl On Thu, 20 Oct 2016, Michal Privoznik wrote:
On 20.10.2016 04:57, Sławek Kapłoński wrote:
v3: http://www.redhat.com/archives/libvir-list/2016-October/msg00627.html
Differences in v4: * function to check string moved from src/util/virstring to src/util/virxml
Sławek Kapłoński (3): util: Add function to check if string contains some illegal chars Use new util function to check network name Forbid new-line char in name of new networks
src/conf/network_conf.c | 5 +---- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 3 +++ src/util/virxml.c | 28 ++++++++++++++++++++++++++++ src/util/virxml.h | 3 +++ 5 files changed, 36 insertions(+), 4 deletions(-)
I've fixed all the small nits I've found and raised in the review and pushed this.
Congratulations on your first libvirt contribution!
Thanks a lot. I hope it will be not the last one :)
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Michal Privoznik
-
Sławek Kapłoński