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

[Laine Stump]

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.