On 08/29/2012 10:32 PM, Eric Blake wrote:
> On 08/29/2012 11:44 AM, Kyle Mestery wrote:
>> Fixup buffer usage when handling VLANs. Also fix the logic
>> used to determine if the virNetDevVlanPtr is valid or not.
>> Fixes crashes in the latest code when using Open vSwitch
>> virtualports.
>>
>> Signed-off-by: Kyle Mestery <kmestery@xxxxxxxxx>
>> ---
>
> It's hard to see the real fix amongst the churn.
>
>> - virBufferPtr buf;
>> + virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> Yes, the old code risked freeing an uninitialized pointer.
>
>> - if (virtVlan) {
>> - if (VIR_ALLOC(buf) < 0)
>> - goto out_of_memory;
>> +
>> + if (virtVlan && virtVlan->nTags > 0) {
>
> This line also impacts behavior, unrelated to the change between 'buf'
> being heap-allocated vs. stack-allocated...
That said, I _like_ using stack-allocated instead of heap-allocated, but
that means that I think there's two separate issues (virtVlan->nTags vs.
virBuffer usage cleanup), so maybe this deserves two patches instead of
one...
> Note that the new code is wrong as well - you should unconditionally
> free a virBufferPtr's contents, rather than conditionally freeing it
> based on virBufferUse() returning non-zero.
>
> That is, I think the real patch here would be simply this (untested)
> version:
This 3-liner to fix the bug at hand would be the first patch, then
another to convert to stack-allocation with no semantic changes.
>
> diff --git i/src/util/virnetdevopenvswitch.c
> w/src/util/virnetdevopenvswitch.c
> index b903ae4..e8b9f4a 100644
> --- i/src/util/virnetdevopenvswitch.c
> +++ w/src/util/virnetdevopenvswitch.c
> @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname,
> const char *ifname,
> char *ifaceid_ex_id = NULL;
> char *profile_ex_id = NULL;
> char *vmid_ex_id = NULL;
> - virBufferPtr buf;
> + virBufferPtr buf = NULL;
>
> virMacAddrFormat(macaddr, macaddrstr);
> virUUIDFormat(ovsport->interfaceID, ifuuidstr);
> @@ -79,7 +79,7 @@ int virNetDevOpenvswitchAddPort(const char *brname,
> const char *ifname,
> ovsport->profileID) < 0)
> goto out_of_memory;
> }
> - if (virtVlan) {
> + if (virtVlan && virtVlan->tag[0]) {
> if (VIR_ALLOC(buf) < 0)
> goto out_of_memory;
>
> @@ -135,6 +135,7 @@ int virNetDevOpenvswitchAddPort(const char *brname,
> const char *ifname,
>
> ret = 0;
> cleanup:
> + virBufferFreeAndReset(&buf);
> VIR_FREE(buf);
> VIR_FREE(attachedmac_ex_id);
> VIR_FREE(ifaceid_ex_id);
>
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list