[libvirt] [PATCH 0/2] Forbid new-line char in name of networks
*** BLURB HERE *** Sławek Kapłoński (2): util: Add function to check if string contains some chars Forbid new-line char in name of networks src/conf/network_conf.c | 5 +---- src/util/virxml.c | 18 ++++++++++++++++++ src/util/virxml.h | 4 ++++ 3 files changed, 23 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 new-line. --- src/util/virxml.c | 18 ++++++++++++++++++ src/util/virxml.h | 4 ++++ 2 files changed, 22 insertions(+) diff --git a/src/util/virxml.c b/src/util/virxml.c index 03bd784..450487e 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -890,6 +890,24 @@ virXMLChildElementCount(xmlNodePtr node) return ret; } +/** + * virXMLCheckString: checks if string contains at least one of + * forbidden characters + * + * Returns: 0 if string don't contains any of given characters, -1 otherwise + */ +int virXMLCheckString(const char *nodeName, char *str, + const char *forbiddenChars) +{ + char *c; + c = strpbrk(str, forbiddenChars); + if (c) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid char in %s: %c"), nodeName, *c); + return -1; + } + return 0; +} /** * virXMLNodeToString: convert an XML node ptr to an XML string diff --git a/src/util/virxml.h b/src/util/virxml.h index 7a0a1da..676bf5e 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -75,6 +75,10 @@ char * virXMLPropString(xmlNodePtr node, const char *name); long virXMLChildElementCount(xmlNodePtr node); +int virXMLCheckString(const char *nodeName, + char *str, + const char *forbiddenChars); + /* Internal function; prefer the macros below. */ xmlDocPtr virXMLParseHelper(int domcode, const char *filename, -- 2.10.0
On 11.10.2016 04:15, 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 new-line. --- src/util/virxml.c | 18 ++++++++++++++++++ src/util/virxml.h | 4 ++++ 2 files changed, 22 insertions(+)
diff --git a/src/util/virxml.c b/src/util/virxml.c index 03bd784..450487e 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -890,6 +890,24 @@ virXMLChildElementCount(xmlNodePtr node) return ret; }
+/** + * virXMLCheckString: checks if string contains at least one of + * forbidden characters + * + * Returns: 0 if string don't contains any of given characters, -1 otherwise + */ +int virXMLCheckString(const char *nodeName, char *str, + const char *forbiddenChars) +{ + char *c; + c = strpbrk(str, forbiddenChars); + if (c) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid char in %s: %c"), nodeName, *c); + return -1; + } + return 0; +}
/** * virXMLNodeToString: convert an XML node ptr to an XML string diff --git a/src/util/virxml.h b/src/util/virxml.h index 7a0a1da..676bf5e 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -75,6 +75,10 @@ char * virXMLPropString(xmlNodePtr node, const char *name); long virXMLChildElementCount(xmlNodePtr node);
+int virXMLCheckString(const char *nodeName, + char *str, + const char *forbiddenChars); + /* Internal function; prefer the macros below. */ xmlDocPtr virXMLParseHelper(int domcode, const char *filename,
Looking good, however you forgot to add the symbol to src/libvirt_private.syms. Without it, other parts of the code might not be able to use this function if linker decides to optimize the symbol out. Michal
On 10/10/2016 04:15 PM, Sławek Kapłoński wrote:
This new function can be used to check if e.g. name of XML node don't
s/don't/doesn't/
contains forbidden chars like "/" or new-line.
s/new-line/'\n'
--- src/util/virxml.c | 18 ++++++++++++++++++ src/util/virxml.h | 4 ++++ 2 files changed, 22 insertions(+)
Couple of other nits... There are a few other string manipulation API's in src/util/virstring.c whether these changes belong there is a matter of interpretation. Certainly there's quite a few places where you might also get some ideas especially with respect to handling these types of things from other parts of libvirt... If you went with a common virstring function, it could check check and return the 0 or -1 allowing/forcing the caller to provide the error message...
diff --git a/src/util/virxml.c b/src/util/virxml.c index 03bd784..450487e 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -890,6 +890,24 @@ virXMLChildElementCount(xmlNodePtr node) return ret; }
We've been using 2 blank lines between functions lately.
+/** + * virXMLCheckString: checks if string contains at least one of + * forbidden characters
More recent comments would be: * virXMLCheckString: * @nodename: <description> * @str: <description> * @forbiddenChars: <description> * * <Function description> *
+ * + * Returns: 0 if string don't contains any of given characters, -1 otherwise
s/don't/doesn't
+ */ +int virXMLCheckString(const char *nodeName, char *str, + const char *forbiddenChars)
We've been using: int virXMLCheckString(const char *nodeName, char *str, const char *forbiddenChars) I would think 'str' is const too since you're not changing it.
+{ + char *c; + c = strpbrk(str, forbiddenChars); + if (c) {
if ((c = strpbrk(str, forbiddenChars))) {
+ virReportError(VIR_ERR_XML_DETAIL, + _("invalid char in %s: %c"), nodeName, *c);
The _( needs to be lined up below the VIR, e.g. virReportError(VIR_ERR_XML_DETAIL, _("invalid char in ..."), ...); [the ... args could be on the second line] Now if the *c is '\n' (or any other non-printable), then printing won't do much good without some form of escaping. Alternatively the hex value could be printed... You may want to look at {virStringHas|virStringStrip}ControlChars too for some more thoughts.
+ return -1; + } + return 0; +}
/** * virXMLNodeToString: convert an XML node ptr to an XML string diff --git a/src/util/virxml.h b/src/util/virxml.h index 7a0a1da..676bf5e 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -75,6 +75,10 @@ char * virXMLPropString(xmlNodePtr node, const char *name); long virXMLChildElementCount(xmlNodePtr node);
+int virXMLCheckString(const char *nodeName, + char *str, + const char *forbiddenChars);
I see you're just following existing practices in the module that's good. John
+ /* Internal function; prefer the macros below. */ xmlDocPtr virXMLParseHelper(int domcode, const char *filename,
New line character in name of network is now forbidden because it mess virsh output and can be confusing for users. Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064 --- 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..a7f802b 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 (virXMLCheckString("name", def->name, "/\n")) goto error; - } /* Extract network uuid */ tmp = virXPathString("string(./uuid[1])", ctxt); -- 2.10.0
On 11.10.2016 04:15, 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.
Closes-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=818064 --- 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..a7f802b 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 (virXMLCheckString("name", def->name, "/\n")) goto error; - }
/* Extract network uuid */ tmp = virXPathString("string(./uuid[1])", ctxt);
Almost. As Peter pointed out, failure to parse XML (and this is causing failure) results in libvirt not knowing anything about the object in the XML. For instance, if somebody already has a network with '\n' in its name, after this is applied libvirt stops reporting the network and leaves it behind. With no chance for users to adjust their definition. What we should do here is to: 1) still deny '/' in network name (as this was never allowed), 2) deny networks with any other forbidden characters (like '\n' for instance) in network startup process. Michal
On 11.10.2016 04:15, Sławek Kapłoński wrote:
*** BLURB HERE ***
Sławek Kapłoński (2): util: Add function to check if string contains some chars Forbid new-line char in name of networks
src/conf/network_conf.c | 5 +---- src/util/virxml.c | 18 ++++++++++++++++++ src/util/virxml.h | 4 ++++ 3 files changed, 23 insertions(+), 4 deletions(-)
Usually, we prefix new versions of the patches with 'PATCH v2 ...' for version 2, 'PATCH v3 ...' for version 3, and so on. And on the top of that, we describe what are the changes compared to previous version(s). This is a good example: https://www.redhat.com/archives/libvir-list/2016-October/msg00306.html Michal
participants (3)
-
John Ferlan -
Michal Privoznik -
Sławek Kapłoński