[libvirt] [PATCH v3 0/2] Forbid new-line char in name of networks
v2: http://www.redhat.com/archives/libvir-list/2016-October/msg00451.html Differences in v3: * function to check string moved from src/util/virxml to src/util/virstring * validation if name of network contains \n char moved from parsing XML to functions responsible for create/define new networks Sławek Kapłoński (2): util: Add function to check if string contains some chars Forbid new-line char in name of new networks src/conf/network_conf.c | 2 +- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 6 ++++++ src/util/virstring.c | 9 +++++++++ src/util/virstring.h | 1 + 5 files changed, 18 insertions(+), 1 deletion(-) -- 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/conf/network_conf.c | 2 +- src/libvirt_private.syms | 1 + src/util/virstring.c | 9 +++++++++ src/util/virstring.h | 1 + 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa39776..949d138 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2117,7 +2117,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; } - if (strchr(def->name, '/')) { + if (virStringHasChars(def->name, "/")) { virReportError(VIR_ERR_XML_ERROR, _("name %s cannot contain '/'"), def->name); goto error; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55b6a24..6f41234 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2435,6 +2435,7 @@ virStringEncodeBase64; virStringFreeList; virStringFreeListCount; virStringGetFirstWithPrefix; +virStringHasChars; virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; diff --git a/src/util/virstring.c b/src/util/virstring.c index 4a70620..7799d87 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -975,6 +975,15 @@ virStringStripIPv6Brackets(char *str) } +bool +virStringHasChars(const char *str, const char *chars) +{ + if (strpbrk(str, chars)) + return true; + return false; +} + + static const char control_chars[] = "\x01\x02\x03\x04\x05\x06\x07" "\x08" /* \t \n */ "\x0B\x0C" /* \r */ "\x0E\x0F" diff --git a/src/util/virstring.h b/src/util/virstring.h index 8854d5f..7f2c96d 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -272,6 +272,7 @@ char *virStringReplace(const char *haystack, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); void virStringStripIPv6Brackets(char *str); +bool virStringHasChars(const char *str, const char *chars); bool virStringHasControlChars(const char *str); void virStringStripControlChars(char *str); -- 2.10.0
On 14.10.2016 04:53, 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/conf/network_conf.c | 2 +- src/libvirt_private.syms | 1 + src/util/virstring.c | 9 +++++++++ src/util/virstring.h | 1 + 4 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa39776..949d138 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2117,7 +2117,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; }
- if (strchr(def->name, '/')) { + if (virStringHasChars(def->name, "/")) { virReportError(VIR_ERR_XML_ERROR, _("name %s cannot contain '/'"), def->name);
Usually, in one patch we introduce a function and then in a subsequent one we switch the rest of the code into using it. But okay, I can live with this too.
goto error; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55b6a24..6f41234 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2435,6 +2435,7 @@ virStringEncodeBase64; virStringFreeList; virStringFreeListCount; virStringGetFirstWithPrefix; +virStringHasChars; virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; diff --git a/src/util/virstring.c b/src/util/virstring.c index 4a70620..7799d87 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -975,6 +975,15 @@ virStringStripIPv6Brackets(char *str) }
+bool +virStringHasChars(const char *str, const char *chars) +{ + if (strpbrk(str, chars)) + return true; + return false; +}
This, however, makes not much sense. I mean, this function has no added value to pain strpbrk(). What I suggested in one of previous reviews was that this function should report an error too. In that case, it will immediately have added value and thus it will be handy to use it. Perhaps you are afraid that error message might change in some cases? That's okay, we don't make any promises about error messages.
+ + static const char control_chars[] = "\x01\x02\x03\x04\x05\x06\x07" "\x08" /* \t \n */ "\x0B\x0C" /* \r */ "\x0E\x0F" diff --git a/src/util/virstring.h b/src/util/virstring.h index 8854d5f..7f2c96d 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -272,6 +272,7 @@ char *virStringReplace(const char *haystack, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
void virStringStripIPv6Brackets(char *str); +bool virStringHasChars(const char *str, const char *chars); bool virStringHasControlChars(const char *str); void virStringStripControlChars(char *str);
Michal
Hello, Thx for review. Please read my answear inline. -- Best regards / Pozdrawiam Sławek Kapłoński slawek@kaplonski.pl On Tue, 18 Oct 2016, Michal Privoznik wrote:
On 14.10.2016 04:53, 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/conf/network_conf.c | 2 +- src/libvirt_private.syms | 1 + src/util/virstring.c | 9 +++++++++ src/util/virstring.h | 1 + 4 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa39776..949d138 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2117,7 +2117,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; }
- if (strchr(def->name, '/')) { + if (virStringHasChars(def->name, "/")) { virReportError(VIR_ERR_XML_ERROR, _("name %s cannot contain '/'"), def->name);
Usually, in one patch we introduce a function and then in a subsequent one we switch the rest of the code into using it. But okay, I can live with this too.
goto error; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55b6a24..6f41234 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2435,6 +2435,7 @@ virStringEncodeBase64; virStringFreeList; virStringFreeListCount; virStringGetFirstWithPrefix; +virStringHasChars; virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; diff --git a/src/util/virstring.c b/src/util/virstring.c index 4a70620..7799d87 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -975,6 +975,15 @@ virStringStripIPv6Brackets(char *str) }
+bool +virStringHasChars(const char *str, const char *chars) +{ + if (strpbrk(str, chars)) + return true; + return false; +}
This, however, makes not much sense. I mean, this function has no added value to pain strpbrk(). What I suggested in one of previous reviews was that this function should report an error too. In that case, it will immediately have added value and thus it will be handy to use it. Perhaps you are afraid that error message might change in some cases? That's okay, we don't make any promises about error messages.
I agree that in fact it don't have too much sense but I'm not sure that it would be good to report error here. First of all, it could be that in some cases it could be used to check if function have required chars so there is no easy way to check which result is error in fact. Second thing (maybe here I'm wrong) places where I wanted to use this function are reporting error VIR_ERR_XML_ERROR and I'm not sure it is good place to report such error in "string lib". So maybe better solution would be just use strbprk (or strchr) in src/network/bridge_driver.c to check if name contains \n char and not introduce any new function to such check? What You think about that?
+ + static const char control_chars[] = "\x01\x02\x03\x04\x05\x06\x07" "\x08" /* \t \n */ "\x0B\x0C" /* \r */ "\x0E\x0F" diff --git a/src/util/virstring.h b/src/util/virstring.h index 8854d5f..7f2c96d 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -272,6 +272,7 @@ char *virStringReplace(const char *haystack, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
void virStringStripIPv6Brackets(char *str); +bool virStringHasChars(const char *str, const char *chars); bool virStringHasControlChars(const char *str); void virStringStripControlChars(char *str);
Michal
On 19.10.2016 03:55, Sławek Kapłoński wrote:
On 14.10.2016 04:53, 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/conf/network_conf.c | 2 +- src/libvirt_private.syms | 1 + src/util/virstring.c | 9 +++++++++ src/util/virstring.h | 1 + 4 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa39776..949d138 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2117,7 +2117,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; }
- if (strchr(def->name, '/')) { + if (virStringHasChars(def->name, "/")) { virReportError(VIR_ERR_XML_ERROR, _("name %s cannot contain '/'"), def->name);
Usually, in one patch we introduce a function and then in a subsequent one we switch the rest of the code into using it. But okay, I can live with this too.
goto error; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55b6a24..6f41234 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2435,6 +2435,7 @@ virStringEncodeBase64; virStringFreeList; virStringFreeListCount; virStringGetFirstWithPrefix; +virStringHasChars; virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; diff --git a/src/util/virstring.c b/src/util/virstring.c index 4a70620..7799d87 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -975,6 +975,15 @@ virStringStripIPv6Brackets(char *str) }
+bool +virStringHasChars(const char *str, const char *chars) +{ + if (strpbrk(str, chars)) + return true; + return false; +}
This, however, makes not much sense. I mean, this function has no added value to pain strpbrk(). What I suggested in one of previous reviews was that this function should report an error too. In that case, it will immediately have added value and thus it will be handy to use it. Perhaps you are afraid that error message might change in some cases? That's okay, we don't make any promises about error messages.
I agree that in fact it don't have too much sense but I'm not sure that it would be good to report error here. First of all, it could be that in some cases it could be used to check if function have required chars so
Tue, 18 Oct 2016, Michal Privoznik wrote: there is no easy way to check which result is error in fact.
Well, even if we did want to check for that, strpbrk() is no help there. I mean, you cannot use it to check if a string contains only allowed characters and nothing more.
Second thing (maybe here I'm wrong) places where I wanted to use this function are reporting error VIR_ERR_XML_ERROR and I'm not sure it is good place to report such error in "string lib".
That's why I initially suggested for this function to be in virxml.c (and thus have a slightly different name to reflect the submodule it is in).
So maybe better solution would be just use strbprk (or strchr) in src/network/bridge_driver.c to check if name contains \n char and not introduce any new function to such check? What You think about that?
Well, then again - I don't think we should limit ourselves to network driver really. Other drivers suffer from the same problem, don't they? I mean, sure - we can just use strpbrk() here and copy it all over the place to different drivers, but I think having a small function just for that would be more convenient. Moreover, we already have some small, one purpose functions in virxml, for instance: virXMLPickShellSafeComment, virXMLSaveFile, virXMLPropString, and others. Michal
Hello, -- Best regards / Pozdrawiam Sławek Kapłoński slawek@kaplonski.pl On Wed, 19 Oct 2016, Michal Privoznik wrote:
On 19.10.2016 03:55, Sławek Kapłoński wrote:
On 14.10.2016 04:53, 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/conf/network_conf.c | 2 +- src/libvirt_private.syms | 1 + src/util/virstring.c | 9 +++++++++ src/util/virstring.h | 1 + 4 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa39776..949d138 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2117,7 +2117,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; }
- if (strchr(def->name, '/')) { + if (virStringHasChars(def->name, "/")) { virReportError(VIR_ERR_XML_ERROR, _("name %s cannot contain '/'"), def->name);
Usually, in one patch we introduce a function and then in a subsequent one we switch the rest of the code into using it. But okay, I can live with this too.
goto error; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55b6a24..6f41234 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2435,6 +2435,7 @@ virStringEncodeBase64; virStringFreeList; virStringFreeListCount; virStringGetFirstWithPrefix; +virStringHasChars; virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; diff --git a/src/util/virstring.c b/src/util/virstring.c index 4a70620..7799d87 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -975,6 +975,15 @@ virStringStripIPv6Brackets(char *str) }
+bool +virStringHasChars(const char *str, const char *chars) +{ + if (strpbrk(str, chars)) + return true; + return false; +}
This, however, makes not much sense. I mean, this function has no added value to pain strpbrk(). What I suggested in one of previous reviews was that this function should report an error too. In that case, it will immediately have added value and thus it will be handy to use it. Perhaps you are afraid that error message might change in some cases? That's okay, we don't make any promises about error messages.
I agree that in fact it don't have too much sense but I'm not sure that it would be good to report error here. First of all, it could be that in some cases it could be used to check if function have required chars so
Tue, 18 Oct 2016, Michal Privoznik wrote: there is no easy way to check which result is error in fact.
Well, even if we did want to check for that, strpbrk() is no help there. I mean, you cannot use it to check if a string contains only allowed characters and nothing more.
Second thing (maybe here I'm wrong) places where I wanted to use this function are reporting error VIR_ERR_XML_ERROR and I'm not sure it is good place to report such error in "string lib".
That's why I initially suggested for this function to be in virxml.c (and thus have a slightly different name to reflect the submodule it is in).
So maybe better solution would be just use strbprk (or strchr) in src/network/bridge_driver.c to check if name contains \n char and not introduce any new function to such check? What You think about that?
Well, then again - I don't think we should limit ourselves to network driver really. Other drivers suffer from the same problem, don't they? I mean, sure - we can just use strpbrk() here and copy it all over the place to different drivers, but I think having a small function just for that would be more convenient. Moreover, we already have some small, one purpose functions in virxml, for instance: virXMLPickShellSafeComment, virXMLSaveFile, virXMLPropString, and others.
Ok, that makes sense. I will add this function to virxml.c then and will rename it. Then it should be fine to report error message also in such function. Once again, thx for help with that.
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 | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b2af482..df85884 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2973,6 +2973,12 @@ networkValidate(virNetworkDriverStatePtr driver, bool bandwidthAllowed = true; bool usesInterface = false, usesAddress = false; + if (virStringHasChars(def->name, "\n")) { + virReportError(VIR_ERR_XML_ERROR, + _("name %s cannot contain '\\n'"), def->name); + 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 14.10.2016 04:53, 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 | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b2af482..df85884 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2973,6 +2973,12 @@ networkValidate(virNetworkDriverStatePtr driver, bool bandwidthAllowed = true; bool usesInterface = false, usesAddress = false;
+ if (virStringHasChars(def->name, "\n")) { + virReportError(VIR_ERR_XML_ERROR, + _("name %s cannot contain '\\n'"), def->name); + return -1; + } + /* Only the three L3 network types that are configured by libvirt * need to have a bridge device name / mac address provided */
Good, you found the best place to have this check. Impressive. But if we go with my suggestion, this can be reduced to: if (virStringHasChars(def->name, "\n")) return -1; Also, any plans on turning some other checks in other drivers into using this wrapper? Michal
On 14.10.2016 04:53, Sławek Kapłoński wrote:
v2: http://www.redhat.com/archives/libvir-list/2016-October/msg00451.html
Differences in v3: * function to check string moved from src/util/virxml to src/util/virstring * validation if name of network contains \n char moved from parsing XML to functions responsible for create/define new networks
Sławek Kapłoński (2): util: Add function to check if string contains some chars Forbid new-line char in name of new networks
src/conf/network_conf.c | 2 +- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 6 ++++++ src/util/virstring.c | 9 +++++++++ src/util/virstring.h | 1 + 5 files changed, 18 insertions(+), 1 deletion(-)
Looks more like it. I mean, we are nearly there. See my comments to each patch. Michal
participants (2)
-
Michal Privoznik -
Sławek Kapłoński