[libvirt] [PATCH] network: fix crash when portgroup has no name
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=879473 The name attribute is required for portgroup elements (yes, the RNG specifies that), and there is code in libvirt that assumes it is non-null. Unfortunately, the portgroup parsing function wasn't checking for lack of portgroup. One adverse result of this was that attempts to update a network by adding a portgroup with no name would cause libvirtd to segfault. For example: virsh net-update default add portgroup "<portgroup default='yes'/>" This patch causes virNetworkPortGroupParseXML to fail if no name is specified, thus avoiding any later problems. --- src/conf/network_conf.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 228951d..6ce2e63 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1175,6 +1175,12 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def, /* grab raw data from XML */ def->name = virXPathString("string(./@name)", ctxt); + if (!def->name) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing required name attribute in portgroup")); + goto error; + } + isDefault = virXPathString("string(./@default)", ctxt); def->isDefault = isDefault && STRCASEEQ(isDefault, "yes"); -- 1.7.11.7
On 11/28/2012 06:08 AM, Laine Stump wrote:
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=879473
The name attribute is required for portgroup elements (yes, the RNG specifies that), and there is code in libvirt that assumes it is non-null. Unfortunately, the portgroup parsing function wasn't checking for lack of portgroup. One adverse result of this was that attempts to update a network by adding a portgroup with no name would cause libvirtd to segfault. For example:
virsh net-update default add portgroup "<portgroup default='yes'/>"
This patch causes virNetworkPortGroupParseXML to fail if no name is specified, thus avoiding any later problems.
Looking at the code, I see it's required on more places, yes. And according to the documentation the name is needed.
--- src/conf/network_conf.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 228951d..6ce2e63 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1175,6 +1175,12 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def,
/* grab raw data from XML */ def->name = virXPathString("string(./@name)", ctxt); + if (!def->name) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing required name attribute in portgroup")); + goto error; + } + isDefault = virXPathString("string(./@default)", ctxt); def->isDefault = isDefault && STRCASEEQ(isDefault, "yes");
Just a question; there's a similar check for (!def->name), for networks particularly, and that one uses VIR_ERR_NO_NAME (specified as a error for missing _domain_ name). Should one of these be changed (in a separate patch, of course)? Anyway, ACK for this one, Martin
On 11/28/2012 04:19 AM, Martin Kletzander wrote:
On 11/28/2012 06:08 AM, Laine Stump wrote:
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=879473
The name attribute is required for portgroup elements (yes, the RNG specifies that), and there is code in libvirt that assumes it is non-null. Unfortunately, the portgroup parsing function wasn't checking for lack of portgroup. One adverse result of this was that attempts to update a network by adding a portgroup with no name would cause libvirtd to segfault. For example:
virsh net-update default add portgroup "<portgroup default='yes'/>"
This patch causes virNetworkPortGroupParseXML to fail if no name is specified, thus avoiding any later problems. Looking at the code, I see it's required on more places, yes. And according to the documentation the name is needed.
--- src/conf/network_conf.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 228951d..6ce2e63 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1175,6 +1175,12 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def,
/* grab raw data from XML */ def->name = virXPathString("string(./@name)", ctxt); + if (!def->name) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing required name attribute in portgroup")); + goto error; + } + isDefault = virXPathString("string(./@default)", ctxt); def->isDefault = isDefault && STRCASEEQ(isDefault, "yes");
Just a question; there's a similar check for (!def->name), for networks particularly, and that one uses VIR_ERR_NO_NAME (specified as a error for missing _domain_ name). Should one of these be changed (in a separate patch, of course)?
According to the comment with the definition of VIR_ERR_NO_NAME, it's intended only for when the name is missing from a domain. The error printed out though is "missing name information" (with optional "in %s"). So if you look at the comment, it seems that it shouldn't be used for network (or node device) name, but if you look at the message generated, it looks like it could be used for any missing name. Personally I don't see the use of having a separate error code for missing name vs. missing [anything else]. To me it's just like any other XML error.
On 11/28/2012 05:55 PM, Laine Stump wrote:
On 11/28/2012 04:19 AM, Martin Kletzander wrote:
On 11/28/2012 06:08 AM, Laine Stump wrote:
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=879473
The name attribute is required for portgroup elements (yes, the RNG specifies that), and there is code in libvirt that assumes it is non-null. Unfortunately, the portgroup parsing function wasn't checking for lack of portgroup. One adverse result of this was that attempts to update a network by adding a portgroup with no name would cause libvirtd to segfault. For example:
virsh net-update default add portgroup "<portgroup default='yes'/>"
This patch causes virNetworkPortGroupParseXML to fail if no name is specified, thus avoiding any later problems. Looking at the code, I see it's required on more places, yes. And according to the documentation the name is needed.
--- src/conf/network_conf.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 228951d..6ce2e63 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1175,6 +1175,12 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def,
/* grab raw data from XML */ def->name = virXPathString("string(./@name)", ctxt); + if (!def->name) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing required name attribute in portgroup")); + goto error; + } + isDefault = virXPathString("string(./@default)", ctxt); def->isDefault = isDefault && STRCASEEQ(isDefault, "yes");
Just a question; there's a similar check for (!def->name), for networks particularly, and that one uses VIR_ERR_NO_NAME (specified as a error for missing _domain_ name). Should one of these be changed (in a separate patch, of course)?
According to the comment with the definition of VIR_ERR_NO_NAME, it's intended only for when the name is missing from a domain. The error printed out though is "missing name information" (with optional "in %s").
So if you look at the comment, it seems that it shouldn't be used for network (or node device) name, but if you look at the message generated, it looks like it could be used for any missing name. Personally I don't see the use of having a separate error code for missing name vs. missing [anything else]. To me it's just like any other XML error.
That's exactly what I meant, so you assured me that I haven't misunderstood the second appearance of that. Thanks, Martin
participants (2)
-
Laine Stump -
Martin Kletzander