[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]