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.
> src/util/virnetdevopenvswitch.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index b903ae4..cdbc5ef 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/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;
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
Yes, the old code risked freeing an uninitialized pointer.
>
> virMacAddrFormat(macaddr, macaddrstr);
> virUUIDFormat(ovsport->interfaceID, ifuuidstr);
> @@ -79,13 +79,12 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
> ovsport->profileID) < 0)
> goto out_of_memory;
> }
> - 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...
>
> /* Trunk port first */
> - if (virtVlan->trunk) {
> - virBufferAddLit(buf, "trunk=");
> + if (virtVlan->trunk == true) {
'cond == true' as a condition is a pointless change, 'cond' is sufficient.
> @@ -135,7 +134,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
>
> ret = 0;
> cleanup:
> - VIR_FREE(buf);
> + if (virBufferUse(&buf) > 0)
> + virBufferFreeAndReset(&buf);
...and this change fixes both a potential memory leak, where the old
VIR_FREE(buf) risked leaking the contents of buf, and the potential
free(garbage).
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:
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