[libvirt] [PATCH] Forbid new-line char in name of networks
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 | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa39776..f9c537f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2123,6 +2123,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; } + if (strchr(def->name, '\n')) { + virReportError( + VIR_ERR_XML_ERROR, + _("name %s cannot contain new-line character"), def->name); + goto error; + } + /* Extract network uuid */ tmp = virXPathString("string(./uuid[1])", ctxt); if (!tmp) { -- 2.10.0
On 08.10.2016 23:38, 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 | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa39776..f9c537f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2123,6 +2123,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; }
+ if (strchr(def->name, '\n')) { + virReportError( + VIR_ERR_XML_ERROR, + _("name %s cannot contain new-line character"), def->name); + goto error; + } + /* Extract network uuid */ tmp = virXPathString("string(./uuid[1])", ctxt); if (!tmp) {
Okay, this makes sense. We can put reasonable restrictions on object names. However, I think while we are at this we can fix the same problem in other areas of the code. For instance, storage pools and volumes suffer from the same problem. Now, instead of copying this chunk all over the places, what about doing this more wisely? For instance: char *c; ... /* Parse name from XML */ if ((c = strpbrk(def->name, "/\n")) { /* Insert your favourite character here ^^ */ virReportError(VIR_ERR_XML_DETAIL, _("invalid char in name: %c"), *c); return -1; /* goto cleanup */ } This way, we can just add new character to the string when somebody files a bug. Moreover, if we'd wrap those couple of lines into a function, we could just do: if (!virXMLCheckName(def->name, "/\n")) return -1; Let me know if you have any troubles writing this, or if you disagree. Michal
On Mon, Oct 10, 2016 at 04:53:15 +0200, Michal Privoznik wrote:
On 08.10.2016 23:38, 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 | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa39776..f9c537f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2123,6 +2123,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; }
+ if (strchr(def->name, '\n')) { + virReportError( + VIR_ERR_XML_ERROR, + _("name %s cannot contain new-line character"), def->name);
Please note that this is not our expected style of breaking up arguments.
+ goto error; + } + /* Extract network uuid */ tmp = virXPathString("string(./uuid[1])", ctxt); if (!tmp) {
Okay, this makes sense. We can put reasonable restrictions on object names. However, I think while we are at this we can fix the same problem in other areas of the code. For instance, storage pools and volumes suffer from the same problem. Now, instead of copying this chunk all over the places, what about doing this more wisely?
For instance:
char *c; ... /* Parse name from XML */
if ((c = strpbrk(def->name, "/\n")) { /* Insert your favourite character here ^^ */ virReportError(VIR_ERR_XML_DETAIL, _("invalid char in name: %c"), *c); return -1; /* goto cleanup */ }
This way, we can just add new character to the string when somebody files a bug. Moreover, if we'd wrap those couple of lines into a function, we could just do:
if (!virXMLCheckName(def->name, "/\n")) return -1;
Let me know if you have any troubles writing this, or if you disagree.
The problem with making the parser stricter is that it's prone to create regressions for people that might have used such configuration. As this was previously working (except for the broken virsh interface) we should limit such names only when defining a network/vm/whatever. Peter
Hello, Ok. Thx for review. I will try today make new function for it and also will look for how arguments should be splitted in multiple lines. -- Best regards / Pozdrawiam Sławek Kapłoński slawek@kaplonski.pl On Mon, 10 Oct 2016, Peter Krempa wrote:
On Mon, Oct 10, 2016 at 04:53:15 +0200, Michal Privoznik wrote:
On 08.10.2016 23:38, 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 | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index aa39776..f9c537f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2123,6 +2123,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) goto error; }
+ if (strchr(def->name, '\n')) { + virReportError( + VIR_ERR_XML_ERROR, + _("name %s cannot contain new-line character"), def->name);
Please note that this is not our expected style of breaking up arguments.
+ goto error; + } + /* Extract network uuid */ tmp = virXPathString("string(./uuid[1])", ctxt); if (!tmp) {
Okay, this makes sense. We can put reasonable restrictions on object names. However, I think while we are at this we can fix the same problem in other areas of the code. For instance, storage pools and volumes suffer from the same problem. Now, instead of copying this chunk all over the places, what about doing this more wisely?
For instance:
char *c; ... /* Parse name from XML */
if ((c = strpbrk(def->name, "/\n")) { /* Insert your favourite character here ^^ */ virReportError(VIR_ERR_XML_DETAIL, _("invalid char in name: %c"), *c); return -1; /* goto cleanup */ }
This way, we can just add new character to the string when somebody files a bug. Moreover, if we'd wrap those couple of lines into a function, we could just do:
if (!virXMLCheckName(def->name, "/\n")) return -1;
Let me know if you have any troubles writing this, or if you disagree.
The problem with making the parser stricter is that it's prone to create regressions for people that might have used such configuration.
As this was previously working (except for the broken virsh interface) we should limit such names only when defining a network/vm/whatever.
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Michal Privoznik -
Peter Krempa -
Sławek Kapłoński