Re: [libvirt] [PATCH] network: fix crash when portgroup has no name

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)?